Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add System.Collections.Immutable and System.Runtime to msbuild dependencies #10

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

acioc
Copy link
Collaborator

@acioc acioc commented Mar 12, 2020

To make https://github.com/dafny-lang/dafny/pull/559/files simpler for customers, I wanted to go ahead and add the two new dependencies directly to msbuild.

I'm not sure of a great way to force dafny-lang/dafny to use a local version for testing though...

acioc added a commit to aws/aws-encryption-sdk that referenced this pull request Mar 13, 2020
As a result of dafny-lang/dafny#559 we need to add 2 additional dependencies if we are running the latest version of Dafny locally. This will likely be reverted once we add them to dafny-lang/dafny.msbuild#10 and update our version of dafny.msbuild to use the latest version
@@ -21,6 +21,8 @@
<PackageReference Include="Microsoft.Build.Framework" Version="15.1.1012" PrivateAssets="All" />
<PackageReference Include="Microsoft.Build.Utilities.Core" Version="15.1.1012" PrivateAssets="All" />
<PackageReference Include="System.Linq.Parallel" Version="4.3.0" PrivateAssets="All" />
<PackageReference Include="System.Collections.Immutable" Version="1.7.0" PrivateAssets="All" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Watch out for the difference between a dependency of dafny.msbuild itself, which shouldn’t be transitively a dependency of a project using this build tool, and one that IS intended to be like these ones. It’s possible the existing dependencies are getting picked up when they shouldn’t be. :)

Copy link
Collaborator Author

@acioc acioc Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, is the recommendation that these new dependencies should be listed as IncludeAssets, so they are properly picked up and usable by customers of dafny.msbuild, or is it that these dependencies don't belong here at all, since they're not required for dafny.msbuild to actually build.

I wanted to add these somewhere that customers could automatically pick up, otherwise anyone depending on dafny.msbuild, otherwise all dafny.msbuild customers would need the additionally dependencies manually entered.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might be getting a spurious build pass here, because this project's build doesn't lock down the Dafny commit like our others. It might have built with a snapshot of Dafny before you actually made the dependency required?

Copy link
Collaborator Author

@acioc acioc Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few notes here...

  1. The way this was written wasn't actually doing anything. Per .NET documentation, we should use IncludeAssets to pass these. This also wasn't actually working as expected, though, as it appeared to make assets consumable, but still required the child to reference that asset (via PackageReference).
  2. The reason this was working was because we were targeting netcoreapp3.0 in TestProject.csproj. If we target netstandard2.0 instead, tests can't run (since it's not a runnable app), but we can resolve the dependency as long as we add a package reference to System.Collections.Immutable. If we don't add the package reference, this fails for the expected reason The type or namespace name 'Immutable' does not exist in the namespace 'System.Collections'
  3. It appears PackageReference is required in TestProject for System.Collections.Immutable regardless of whether or not it's added in dafny.msbuild.csproj (or whether it's PrivateAssets/IncludeAssets in dafny.msbuild.csproj). Adding guidance.
  4. I haven't experimented much with this, but I saw https://github.com/NuGet/Home/wiki/Allow-package--authors-to-define-build-assets-transitive-behavior This is actually a bit different, since it basically says that it compiles but fails at runtime vs it fails to compile (our case)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding 2, it occurs to me that it would be good to have another test project that doesn't include tests and the xunit dependencies. That would allow it to target netstandard2.0 and avoid potentially bringing in the System.Collections.Immutable dependency via xunit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants