-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
1.0 Cryptography native shim is being published in a 2.0 app #2836
Comments
I believe the presence of the old shim library is innocuous but @bartonjs would need to confirm. This would have been trimmed if the new Microsoft.NETCore.Targets package was referenced, since that would wack all the RID-split package dependencies. I was trying to address this with dotnet/corefx#18341 and dotnet/core-setup#2053 but that isn't flowing through Core-Setup for some reason. To workaround this folks can reference the latest Microsoft.NETCore.Targets package. I'm going to look into why this isn't flowing through core-setup and also see what the additional dependencies/conflicts are costing at restore/build to see if its worth fixing for 2.0. |
Correct. It's just dead-weight. None of the code in 2.0 will ever call it. |
Do we need this for 2.0 or should we move it out? |
I need a shiproom read on that. The side effects are dead-weight in additional downloads and one additional unused file showing up in the app when old packages are referenced with new. Its a case of "could be better but nothing is broken". |
@ericstj did it happen in 2.0 or do we need to do more work for 2.1 / 2.0.x (servicing)? |
It did not happen in 2.0. I don't think it meets the bar for servicing but if folks want to do it we can. It's just adding a reference to the Microsoft.NETCore.App package in core-setup. It will help folks by avoiding unnecessary nuget package downloads when building standalone applications that have a mix of old (1.x) and new packages in the package graph. |
Moving to 2.1 then. |
@ericstj is this something we want to target in 2.1? |
I think it has some benefits in 2.1 if we have the runway to make the change and test it. It's essentially adding a reference to NETCore.Targets (latest) to the 2.1 framework package. |
Yes we still have time to get it in for 2.1, can you please put together a PR for it? |
Steps to reproduce
dotnet restore -r osx.10.12-x64
dotnet publish -r osx.10.12-x64
Alternatively if you call
dotnet store -r osx.10.12-x64
on a manifest that references Newtonsoft.Json/9.0.1, you get the same behavior in the outputted runtime store.Expected behavior
No "1.0" native libraries should be published in the publish folder (or in the runtime store). This is a "2.0" app, so all the framework assemblies should be our "2.0" assemblies.
Actual behavior
There is a 1.0 crypto native shim library being published:
This library is not required, and doesn't work with our 2.0 framework.
Technically, nothing should try and load the assembly, so I'm not sure what, if any, downstream effects this will have. But the assembly should not be getting published.
Environment data
Notes
The reason this is happening is because
Newtonsoft.Json/9.0.1
doesn't referenceNETStandard.Library
. Instead, it references the individual System.XXX packages it requires. Thus a bunch of our .NET Core 1.0 packages are being referenced. Normally, this is OK because we've implemented conflict resolution to resolve when 1.0 and 2.0 assemblies are named the same. However, between 1.0 and 1.1 we renamed/refactored this native Cryptography assembly. So the 2.0 framework doesn't have this assembly any more, so there is no conflict to resolve. That means there is nothing stopping this 1.0 assembly from being published.This doesn't happen when publishing a self-contained app for Windows - only on Linux and macOS - since we don't have native shims on Windows.
/cc @bartonjs @ericstj
The text was updated successfully, but these errors were encountered: