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

Added DevelopmentDependency=true to Equals.Fody project #356

Closed
wants to merge 1 commit into from
Closed

Added DevelopmentDependency=true to Equals.Fody project #356

wants to merge 1 commit into from

Conversation

HavenDV
Copy link

@HavenDV HavenDV commented Jul 19, 2021

This change automatically installs private assets after the package is installed.
Will be:

<PackageReference Include="Equals.Fody" Version="4.0.1">
   <PrivateAssets>all</PrivateAssets>
   <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>

Instead:

<PackageReference Include="Equals.Fody" Version="4.0.1" />

@SimonCropp
Copy link
Member

did u smoke test this?

@HavenDV
Copy link
Author

HavenDV commented Jul 19, 2021

I see the following warning after this change, but I find it more clearly emphasizes the nature of the package and makes it easier for the user to remove IncludeAssets after this warning.
MSBUILD : warning FodyPackageReference: Fody: The package reference for Equals.Fody is missing the 'compile' part in the IncludeAssets setting; it's recommended to completely remove IncludeAssets

@HavenDV
Copy link
Author

HavenDV commented Jul 19, 2021

https://github.com/NuGet/Home/wiki/DevelopmentDependency-support-for-PackageReference
This property is officially supported, and is often used in new code generators. I have also used it and I guarantee it works.

@ltrzesniewski
Copy link
Member

This will by default remove the reference to the Equals assembly (which will prevent the usage of the [Equals] attribute for instance). So it's not a good change IMO.

That's why Fody weavers don't set the DevelopmentDependency to true. I'd like to have a way to only set PrivateAssets="all", but it doesn't exist ATM.

@tom-englert
Copy link
Member

Also this should be the same for all weavers, not just this one!

@HavenDV
Copy link
Author

HavenDV commented Jul 19, 2021

NuGet/Home#10746
Yes, I think it is worth waiting for the solution to this problem and then applying it to all Weavers.
Voting for this will probably raise the priority.

@SimonCropp
Copy link
Member

could we use dev dependency, but re add the ref in an msbuild imort?

@ltrzesniewski
Copy link
Member

ltrzesniewski commented Jul 19, 2021

The main problem I see with that is that we'd need to pick the correct TFM for the Reference item. I'm not sure if plugging into the SDK for this would be easy.

I thought about updating the package's IncludeAssets metadata item to an empty string in the package's targets file, but that doesn't strike me as a great idea either. On the other hand, that's pretty easy to try...

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.

4 participants