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

Should netstandard2.0 library publish trim packages? #1855

Closed
dasMulli opened this issue Jan 14, 2018 · 10 comments
Closed

Should netstandard2.0 library publish trim packages? #1855

dasMulli opened this issue Jan 14, 2018 · 10 comments
Labels
Milestone

Comments

@dasMulli
Copy link
Contributor

When building a .NET Framework or .NET Core 2.0 app and ending up with a package reference to a pre-2.0 System.* pacakage, it is removed from the publish output.

The same does not happen for .NET Standard libraries, at least for those that shipped "outside" of NETStandard.Library 1.6.0/1.6.1, but are considered inbox in NETStandard.Library 2.0.0+ for netstandard2.0.
While this is probably fine for <2.0, performing a similar trimming could be beneficial for for >= 2.0 libraries, since all platforms that can load netstandard 2.0 libraries should already carry the necessary assemblies (or the app has binding redirects) to load <= netstandard2.0 libraries.

The scenarios this affects are plugin architectures that try to load assemblies from "plugin" folders that aren't known during build time. This also includes MSBuild tasks.
The danger here is logic like "for all DLLs.." in code or in VS, referencing all published assemblies via the UI, which results in Reference+HintPath items that aren't trimmed (!) and end up in the build/publish output.

Example:

  1. Create a .NET Standard 2.0 library
  2. Add a package reference to System.Text.RegularExpressions (alt: Newtonsoft.Json 10.0.3)
  3. dotnet publish
  4. see System.Text.RegularExpressions.dll in publish output.

Question: Is there a benefit in leaving this as it is or would it be better to trim the these packages for netstandard2.0 publish?

@dasMulli
Copy link
Contributor Author

Been hitting this while trying to make net46+netstandard2.0 build tasks but ended up targeting net+netcoreapp anyway, but may become more interesting in the future. but this came up on SO again: https://stackoverflow.com/questions/48244136

@dsplaisted
Copy link
Member

@ericstj What do you think? Should we be filtering out the assets from the pre-2.0 .NET Standard implementation packages when targeting .NET Standard?

@ericstj
Copy link
Member

ericstj commented Jan 19, 2018

/cc @terrajobst that'd be ideal. One thing that could be used is to see if it's removed by conflict resolution.

@dasMulli
Copy link
Contributor Author

@ericstj wouldn't this require NETStandard.Library to define $(PackageConflictPreferredPackages) and @(PackageConflictPlatformManifests) for the build?

@ericstj
Copy link
Member

ericstj commented Jan 23, 2018

True, I hadn't fully understood the question originally. PlatformManifest is probably the right thing to use. It could be done on the side, doesn't necessarily need to be in NETStandard.Library package.

That said, the bigger problem here is that the whole premise is busted: NETStandard libraries themselves aren't publishable/runnable since they may depend on packages with framework specific implementations and selecting at runtime between frameworks isn't something that any framework understands nor is it persisted by NuGet.

@dasMulli
Copy link
Contributor Author

dasMulli commented Jan 26, 2018

Reflecting on this for a while, I think this absolutely makes sense. The set of scenarios enabled by publishing(!) for a netstandard TFM is very limited and it may lead to confusion when something doesn't work.

However it is a bit weird that publishing for a netstandard TFM works without warnings/errors.. because it technically works, but isn't really useful.

Maybe a warning could be added to indicate that the output may not be usable at all? or an error?.. maybe cc @nguerrera @KathleenDollard
There are warnings already for tfm+rid combinations that don't make sense, maybe a warning/error for a tfm on publish that doesn't seem reasonable would make sense to have as well

@KathleenDollard
Copy link

How would we word such a warning? I'm not sure what we say now when we mean "what you did isn't sensible, are you sure it's what you wanted?"

Also, I'm not sure how we would do a warning here without backward compatibly issues.

@idg10
Copy link

idg10 commented Apr 26, 2018

You say that trimming does occur "When building a .NET Framework or .NET Core 2.0 app" but I've got a project (with a <project Sdk="Microsoft.Net.Sdk"> style .csproj) that builds a <TargetFramework>net462</TargetFramework> exe file, and it doesn't seem to trim this stuff. I've got 106 DLLs with names start System. in my build output. And the same happens if I run dotnet publish - no trimming seems to occur.

Furthermore, the conflict resolution behaviour downstream of this is causing me a serious problem. For reasons outside of my control, I have a DLL named System.Collections.Specialized which is not related to the one that gets added by the .NET standard build machinery. The one I'm using has a different public key token. But apparently the build system ignores that, and decides to give me the System.Collections.Specialized assembly from .NET Standard instead on the grounds that it has a higher version number. Naturally, this causes an assembly load failure due to this being a completely unrelated assembly (different public key token, different version number).

JL03-Yue pushed a commit that referenced this issue Mar 19, 2024
Set minor version to help distinguish builds.
Copy link
Contributor

github-actions bot commented May 1, 2024

Due to lack of recent activity, this issue has been labeled as 'stale'. It will be closed if no further activity occurs within 30 more days. Any new comment will remove the label.

@github-actions github-actions bot added the stale label May 1, 2024
Copy link
Contributor

This issue will now be closed since it has been labeled 'stale' without activity for 30 days.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants