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

Add dependencies from Microsoft.NETCore.App to Microsoft.NETCore.Targets and NETStandard.Library #3574

Closed
dsplaisted opened this issue May 2, 2019 · 30 comments
Assignees
Milestone

Comments

@dsplaisted
Copy link
Member

We should add the following package dependencies to the "Empty" Microsoft.NETCore.App package (see #3528):

  • Microsoft.NETCore.Targets
  • NETStandard.Library version 2.0.3
    • This will prune the 1.x contracts from the dependency graph if they are coming from a package that has a dependency on NETStandard.Library 1.6.1 (or 1.6.0).
    • .NET Standard 2.1 uses a targeting pack, so we don't have a 2.1 package to depend on
    • We could create an "empty" 2.1 NETStandard.Library package if there's a reason to do so, but at this point I'm not sure there is
@dsplaisted
Copy link
Member Author

FYI @ericstj @nguerrera

@ericstj
Copy link
Member

ericstj commented May 2, 2019

/cc @wtgodbe on the NETStandard.Library suggestion. We may want to make an empty one. I'll submit a change to empty MS.NETCore.Targets.

@nguerrera
Copy link
Contributor

I think we should definitely make the empty netstandard 2.1 as well. Let's be consistent here in how these stubs work. I think it will be very confusing to see an actual reference to non-empty netstandard.library 2.0.x injected. It will also cause too much to be downloaded in cases with no legacy refs.

@wtgodbe
Copy link
Member

wtgodbe commented May 2, 2019

I think I'm missing some context here, why do we need to ship an empty Microsoft.NETCore.App? If we empty out NetStandard.Library, is the intention that the change will be permanent, and other repos should start consuming Microsoft.Private.Standard?

@nguerrera
Copy link
Contributor

I don't know what Microsoft.Private.Standard is. NETStandard.Library.Ref is the real payload ("targeting pack"). The empty package is a way to short circuit things in nuget while we're currently lacking any other mechanism.

@wtgodbe
Copy link
Member

wtgodbe commented May 2, 2019

@nguerrera Microsoft.Private.Standard contains essentially the same bits as Netstandard.Library - I believe Core-Setup currently depends on Microsoft.Private.Standard to get the netstandard bits that it injects into the targeting pack (@dagood can you confirm?). However if we empty out Netstandard.Library, that would break other repos that depend on that package (like corefx), unless we intend for those repos to switch over to Microsoft.Private.Standard. I'm missing some information here, so I'm not sure what the motivation is for this request, or what the guidance is for repos that depend on the packages that are going to become empty.

@dagood
Copy link
Member

dagood commented May 2, 2019

other repos should start consuming Microsoft.Private.Standard?

I believe the near-term goal is that Core-Setup is the only place referencing dotnet/standard bits, and it does so through the Microsoft.Private.Standard transport package. That's redisted as NETStandard.Library.Ref that should be used everywhere else. (Repos may need to use a new enough SDK to be able to use NETStandard.Library.Ref?)

We're still depending on NETStandard.Library here, but it should just be a matter of changing the PackageReference:

https://github.com/dotnet/core-setup/blob/29f2b13923b066173b1a156516a3f5f8763cb276/src/pkg/projects/netstandard/src/netstandard.depproj#L10

Long-term, dotnet/standard will produce NETStandard.Library.Ref using shared tooling. (And the workaround stub NETStandard.Library.)

@wtgodbe
Copy link
Member

wtgodbe commented May 2, 2019

For the moment, should we transition CoreFx to Microsoft.Private.Standard, or to NETStandard.Library.Ref? https://github.com/dotnet/corefx/blob/master/eng/Version.Details.xml#L41

@dagood
Copy link
Member

dagood commented May 2, 2019

Here's the issue for the "empty" Microsoft.NETCore.App implementation that might help with context: https://github.com/dotnet/core-setup/issues/5663.

(I don't know about the migration though.)

@ericstj
Copy link
Member

ericstj commented May 22, 2019

/cc @wtgodbe

@dsplaisted
Copy link
Member Author

@dagood Hold off on this for now, currently we are thinking of not relying on the Microsoft.NETCore.App package.

@joperezr
Copy link
Member

joperezr commented Aug 2, 2019

Customers are reporting hitting this issue with the latest sdk, and don't like the manual workaround of having to add a reference to the Targets package. This is a regression, @dsplaisted do we plan to fix this for 3.0?

@dagood
Copy link
Member

dagood commented Aug 2, 2019

In case this is weighed against implementation difficulty (although so far it hasn't sounded like it), it's trivial to add workaround package dependencies, they just need to be added to this itemgroup:

https://github.com/dotnet/core-setup/blob/ccf0024ba099103a5b759613c48b8cd46af8754a/src/pkg/projects/netcoreapp/pkg/workaround/Microsoft.NETCore.App.pkgproj#L20-L22

@dsplaisted
Copy link
Member Author

No, we don't plan to fix this for 3.0. We only had one report of the issue, and we analyzed the packages on NuGet.org to try to figure out how common the issue would be. The data seemed to indicate that the issue would be quite rare.

We are not using the Microsoft.NETCore.App package anymore, so simply adding these dependencies as @dagood suggests won't fix the issue. Referencing the Microsoft.NETCore.App package had a number of issues, and the one that finally tipped us over to abandoning it was that we had a heuristic to only reference it if there were other PackageReferences, but that didn't work if a project had no direct PackageReferences, but transitive PackageReferences through other ProjectReferences.

So to fix the issue, what we would need would be a feature in NuGet: NuGet/Home#7344. Because the impact of the issue appeared to be limited, we opted to go with documentation for now.

cc @rrelyea @nguerrera @livarcocc

@dsplaisted
Copy link
Member Author

@joperezr Can you point us to where customers are hitting this? I've still only seen the one original bug report.

@joperezr
Copy link
Member

joperezr commented Aug 2, 2019

Sure, this is the report I got in the iot repo since the customer thought it was related to the package built in that repository: dotnet/iot#646 (comment)

@joperezr
Copy link
Member

joperezr commented Aug 2, 2019

Do note that the only thing that the user did in order to repro this was:

dotnet new mvc
dotnet restore -r linux-arm

That alone would get you a repro since the problem happens when <PackageReference Include="Microsoft.AspNetCore.Blazor.Server" Version="3.0.0-preview7.19365.7" /> is added to your project and tries to restore for a linux RID

@dsplaisted
Copy link
Member Author

In that case I think something in the MVC or Blazor package graph needs to be updated, or this may be a different issue.

The issue happens when you have contract packages (such as System.IO.FileSystem, etc) from both the 1.0 and 1.1 bands of .NET Core in your package graph. For vanilla 3.0 projects, we shouldn't have any 1.x packages in the graph.

@joperezr
Copy link
Member

joperezr commented Aug 2, 2019

whether the fix goes on the blazor packages or on the sdk, I do think that something must be fixed for 3.0 since I think that it is pretty broken that you can hit this problem so easily. For reference, here is the error message which has the package tree dependency graph:

F:\scratch\temptest\temptest.csproj : error NU1605: Detected package downgrade: System.IO.FileSystem.Primitives from 4.3.0 to 4.0.1. Reference the package directly from the project to select a different version.
F:\scratch\temptest\temptest.csproj : error NU1605:  temptest -> Microsoft.AspNetCore.Blazor.Server 3.0.0-preview7.19365.7 -> Mono.Cecil 0.10.1 -> System.IO.FileSystem 4.0.1 -> runtime.unix.System.IO.FileSystem 4.3.0 -> System.IO.FileSystem.Primitives (>= 4.3.0)
F:\scratch\temptest\temptest.csproj : error NU1605:  temptest -> Microsoft.AspNetCore.Blazor.Server 3.0.0-preview7.19365.7 -> Mono.Cecil 0.10.1 -> System.IO.FileSystem.Primitives (>= 4.0.1)

@nguerrera
Copy link
Contributor

See also dotnet/sdk#3044 (comment)

@nguerrera
Copy link
Contributor

A nestandard2.0 target should be added to Mono.Cecil and Blazor should depend on that.

cc @sbomer

@nguerrera
Copy link
Contributor

It looks like the code was already moved to netstandard2.0, but no release was pushed to nuget?

jbevain/cecil#599 (comment)

cc @jbevain

@nguerrera
Copy link
Contributor

nguerrera commented Aug 4, 2019

@davidfowl Can you help push 1.x assets out of dotnet new mvc graph?

@nguerrera
Copy link
Contributor

nguerrera commented Aug 5, 2019

dotnet new mvc
dotnet restore -r linux-arm

I cannot reproduce this in preview 8.

This didn't trigger it either:

dotnet new blazorserver
dotnet restore -r linux-arm

Neither of these have package references.

@dsplaisted
Copy link
Member Author

I don't think this happens with default projects. The binlog with the failure is linked here. Based on that, the following is a minimal repro:

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>netcoreapp3.0</TargetFramework>
    <RuntimeIdentifier>linux-arm</RuntimeIdentifier>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.Blazor.Server" Version="3.0.0-preview7.19365.7" />
    <PackageReference Include="System.Device.Gpio" Version="0.1.0-prerelease.19376.1" />
  </ItemGroup>

</Project>

For the failure to occur, there need to be dependencies to both 1.0 and 1.1 band contract packages. The Blazor package is bringing in Mono.Cecil which is bringing in 1.0 contracts such as System.IO.FileSystem 4.0.1. The GPIO package is depending on several 1.1 contracts, such as System.Runtime.InteropServices.WindowsRuntime 4.3.0.

As Nick suggests, an updated Mono.Cecil package with a .NET Standard 2.0 version of the library would fix this. Updating the GPIO package so that it doesn't depend on the contract packages should also work if that's possible.

@eerhardt
Copy link
Member

eerhardt commented Aug 9, 2019

@ericstj and @joperezr pointed me to this issue.

I hit this today upgrading ML.NET to 3.0. Restoring this project:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netcoreapp3.0</TargetFramework>
    <RuntimeIdentifier>win-x64</RuntimeIdentifier>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="BenchmarkDotNet" Version="0.11.3" />
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.8.0" />
  </ItemGroup>
</Project>

with the 3.0.100-preview8-013437 SDK results in:

F:\DotNetTest\ThreeOhBug\ThreeOhBug.csproj : error NU1605: Detected package downgrade: Microsoft.Win32.Primitives from 4.3.0 to 4.0.1. Reference the package directly from the project to select a different version.
F:\DotNetTest\ThreeOhBug\ThreeOhBug.csproj : error NU1605:  ThreeOhBug -> Microsoft.NET.Test.Sdk 15.8.0 -> Microsoft.TestPlatform.TestHost 15.8.0 -> Microsoft.TestPlatform.ObjectModel 15.8.0 -> NETStandard.Library 1.6.0 -> System.Net.Primitives 4.0.11 -> runtime.win.System.Net.Primitives 4.3.0 -> Microsoft.Win32.Primitives (>= 4.3.0)
F:\DotNetTest\ThreeOhBug\ThreeOhBug.csproj : error NU1605:  ThreeOhBug -> Microsoft.NET.Test.Sdk 15.8.0 -> Microsoft.TestPlatform.TestHost 15.8.0 -> Microsoft.TestPlatform.ObjectModel 15.8.0 -> NETStandard.Library 1.6.0 -> Microsoft.Win32.Primitives (>= 4.0.1)
F:\DotNetTest\ThreeOhBug\ThreeOhBug.csproj : error NU1605: Detected package downgrade: System.Net.Primitives from 4.3.0 to 4.0.11. Reference the package directly from the project to select a different version.
F:\DotNetTest\ThreeOhBug\ThreeOhBug.csproj : error NU1605:  ThreeOhBug -> Microsoft.NET.Test.Sdk 15.8.0 -> Microsoft.TestPlatform.TestHost 15.8.0 -> Microsoft.TestPlatform.ObjectModel 15.8.0 -> NETStandard.Library 1.6.0 -> System.Net.Sockets 4.1.0 -> runtime.win.System.Net.Sockets 4.3.0 -> System.Net.Primitives (>= 4.3.0)
F:\DotNetTest\ThreeOhBug\ThreeOhBug.csproj : error NU1605:  ThreeOhBug -> Microsoft.NET.Test.Sdk 15.8.0 -> Microsoft.TestPlatform.TestHost 15.8.0 -> Microsoft.TestPlatform.ObjectModel 15.8.0 -> NETStandard.Library 1.6.0 -> System.Net.Primitives (>= 4.0.11)
F:\DotNetTest\ThreeOhBug\ThreeOhBug.csproj : error NU1605: Detected package downgrade: Microsoft.Win32.Primitives from 4.3.0 to 4.0.1. Reference the package directly from the project to select a different version.
F:\DotNetTest\ThreeOhBug\ThreeOhBug.csproj : error NU1605:  ThreeOhBug -> Microsoft.NET.Test.Sdk 15.8.0 -> Microsoft.TestPlatform.TestHost 15.8.0 -> Microsoft.TestPlatform.ObjectModel 15.8.0 -> NETStandard.Library 1.6.0 -> System.Net.Sockets 4.1.0 -> runtime.win.System.Net.Sockets 4.3.0 -> System.Security.Principal.Windows 4.3.0 -> Microsoft.Win32.Primitives (>= 4.3.0)
F:\DotNetTest\ThreeOhBug\ThreeOhBug.csproj : error NU1605:  ThreeOhBug -> Microsoft.NET.Test.Sdk 15.8.0 -> Microsoft.TestPlatform.TestHost 15.8.0 -> Microsoft.TestPlatform.ObjectModel 15.8.0 -> NETStandard.Library 1.6.0 -> Microsoft.Win32.Primitives (>= 4.0.1)

If I literally change any line in the .csproj, it works:

  • change TFM to netcoreapp2.1 - works
  • remove RuntimeIdentifier - works
    remove either PackageReference - works

@eerhardt
Copy link
Member

eerhardt commented Aug 9, 2019

Re-opening this issue so it gets noticed.

@eerhardt eerhardt reopened this Aug 9, 2019
@nguerrera
Copy link
Contributor

nguerrera commented Aug 9, 2019

I don't think this issue can track a fix. We can't use an empty package for reasons that have been discussed.

@nguerrera
Copy link
Contributor

Well, we can have the empty packages, but we can't auto reference them in the SDK.

@nguerrera
Copy link
Contributor

nguerrera commented Aug 9, 2019

dotnet/sdk#3044 tracks the work on the SDK to make these scenarios just work, which after trying many workarounds and failing, is now blocked on NuGet/Home#7344

Both are still open so we should focus there rather than on this workaround issue that won't work.

Please link any reports on either of those issues.

@msftgits msftgits transferred this issue from dotnet/core-setup Jan 30, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 30, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants