-
Notifications
You must be signed in to change notification settings - Fork 258
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
When a ProjectReference and PackageReference in different TFMs have the same identity/version, Restore throws "An item with the same key has already been added. [...]" #10368
Comments
Known issue that @erdembayar is actively working on. |
It's duplicate of Issue6795. |
Looking at that PR, it seems like a different issue. It mentions "Prefer project reference over package reference, so remove the the package reference.". This isn't what I'd expect in this situation. I would expect that (in this example) when building <ItemGroup Condition="'$(TargetFramework)' == 'net5.0'">
<ProjectReference Include="../System.Security.Cryptography.Cng/System.Security.Cryptography.Cng.csproj" />
</ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp3.1'">
<PackageReference Include="System.Security.Cryptography.Cng" Version="5.0.0-rc.2.20475.5" />
</ItemGroup> It sounds like that PR might actually make things worse by silently transforming the |
Project over package is the expectation for restore in general. I think this scenario is slightly different from the linked one, and as such I am re-opening it. |
Not quite sure my expectations are clear... restating just in case:
They seem very isolated from the project author point of view... I don't know why one would ever have to be considered "over" another. Why do they have a chance to collide and force NuGet to choose something that only exists in the "wrong" At the moment this feels like an arbitrary limitation (maybe of the |
Absolutely agree with @dagood on this one. Filed #10617 which was closed as a dup of this one but which includes a very simple repro. Note that in the other issue, the error is slightly different:
This is causing unreasonable pain in dotnet/runtime in which we often reference via @nkolev92 could we please raise the priority of this bug? |
@ViktorHofer |
I don't understand how that applies to the current issue. @erdembayar, can you give some more details? What will the behavior be with Viktor's repro project, or my repro project? The PR description mentions this currently:
This makes me concerned, because as a user, "Project over package" seems unrelated to this issue and (if it's forced in the way that comes to mind) would make the situation even worse than it is now. (See my comments above.) |
Oh, that PR is the same one that this issue was mistakenly closed as a duplicate of once before. #10368 (comment)
It seems to me like the actual status is that there has been no progress towards solving this issue, and there has been progress towards a fix for a different issue that will make this issue worse. (Significantly harder to detect/diagnose.) |
I agree with Davis and his comment above which is a perfect summary of how I would expect NuGet to behave in a situation where a ProjectReference and a PackageReference to the same identity is present but both references being conditioned on a different TFM (inner build). |
@ViktorHofer |
* Runtime specific and doc file packaging fixes * Replace all remaining pkgprojs with NuGet Pack task * Avoid NuGet/Home/issues/10368 * Update PackageValidation package to latest
…6712) * Runtime specific and doc file packaging fixes * Replace all remaining pkgprojs with NuGet Pack task * Avoid NuGet/Home/issues/10368 * Update PackageValidation package to latest Commit migrated from dotnet/runtime@503ae51
As this still affects us in dotnet/runtime and makes it impossible to use ProjectReferences in multi-targeting projects without bringing in potentially vulnerable prebuilts, it would be good if could get some traction on this issue. Any idea where this issue originates? I would be willing to help but I have no idea where to start. |
@ViktorHofer |
Looking into this again as the issue is still impacting us in dotnet/runtime when using ProjectReferences and PackageReferences in different TFMs in the same project. When projects reference
I looked into this more closely and here's what I got so far. Example 1 - Direct dependenciesThe following scenario behaves as expected: main.csproj <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>netstandard2.0;net8.0</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\System.Numerics.Vectors\System.Numerics.Vectors.csproj" Condition="'$(TargetFramework)' == 'net8.0'" />
<PackageReference Include="System.Numerics.Vectors" Version="4.4.0" Condition="'$(TargetFramework)' == 'netstandard2.0'" />
</ItemGroup>
</Project> System.Numerics.Vectors.csproj <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>net8.0</TargetFramework>
</PropertyGroup>
</Project> The ProjectReference is correctly identified as project dependency in the graph walker because the LibraryRange's Example 2 - Transitive dependency onlyThe following scenario also behaves as expected: <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="System.Memory" Version="4.5.5" />
</ItemGroup>
</Project> The direct dependency is resolved as package because the In this case example, a ProjectReference for So far so good... Example 3 - Matching transitive and direct dependencyThe following scenario doesn't behave as expected: main.csproj <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>netstandard2.0;net8.0</TargetFrameworks>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\System.Numerics.Vectors\System.Numerics.Vectors.csproj" Condition="'$(TargetFramework)' == 'net8.0'" />
<!-- System.Memory transitively references System.Numerics.Vectors for netstandard2.0. -->
<PackageReference Include="System.Memory" Version="4.5.5" Condition="'$(TargetFramework)' == 'netstandard2.0'" />
</ItemGroup>
</Project> System.Numerics.Vectors.csproj <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>net8.0</TargetFramework>
</PropertyGroup>
</Project>
First, the direct dependency for the net8.0 TFM is resolved as a project because the Now the Now, the transitive dependency Here's the code in question: https://github.com/NuGet/NuGet.Client/blob/41e98d33e40293d8276015d7a01e4d90610fa9d8/src/NuGet.Core/NuGet.DependencyResolver.Core/ResolverUtility.cs#L161 The tricky part (which I haven't yet figured out) is to make this code aware of which projects (in the |
Fixes NuGet/Home#10368 When a transitive package dependency has the same identity as a direct project dependency that is differenced by a different inner build (TFM), the dependency is incorrectly resolved as a project. The resolver logic invoked by the dependency walker only knows about which projects are referenced but not by which TFM. To correctly isolate inner builds from each other and avoid incorrectly promoting a project dependency, pass the nearest project dependencies to the dependency resolving logic.
I came up with a "solution" and pushed a draft PR: NuGet/NuGet.Client#5397 Note that I had to touch public API ( With these changes, restoring the project tree from the above "Example 3" now works. I'm happy to jump on a call with experts if that helps to explain the problem and the current solution in more detail, cc @nkolev92 @zivkan. |
* Runtime specific and doc file packaging fixes * Replace all remaining pkgprojs with NuGet Pack task * Avoid NuGet/Home/issues/10368 * Update PackageValidation package to latest
There are numerous benefits in using ProjectReferences consistently in all libraries: 1. An upfront "libs" build isn't required anymore and sfx libraries can now directly be built from a fresh clone (with dotnet build or inside VS). I.e. `dotnet.cmd pack src/libraries/System.Text.Json/src/` is now possible from a fresh clone. 2. Because of 1), we can now add a solution file for the whole sfx that can directly be opened and worked with from a fresh clone. 3. The overall root build is faster. Without this change, the build order was sfx-ref -> (sfx-src & sfx-gen) so the shared framework reference projects first had to be built and only then the sfx src and gen projects could be built. Now with this change, everything gets built in parallel. 4. Using P2Ps means that we now follow the common and well supported msbuild and SDK path instead of repo customization. The downside of doing this is that the dependency graph gets bigger, meaning that more projects get incrementally built when doing a "dotnet build". This is nothing new and the SDK team recommends to pass the "--no-dependencies" flag to "dotnet build" if incrementally (no-op) building the additional dependency nodes is noticeable. This is less of a concern inside VS as that has a "fast up-to-date check" feature that doesn't even attempt to build projects that didn't change. For VS, really the only noticeable change is that the solution explorer now lists more projects and that when opening a solution, more projects need to be evaluated. But, that should be fast enough when using an up-to-date version of VS. - A few observations that make the change more involved: There's a NuGet client bug that requires a few workarounds: NuGet/Home#10368 Because of that, direct package refs to System.Numerics.Vectors and in a few cases to System.Memory are required. We should fix the NuGet tooling issue to eventually get rid of the workarounds introduced with this commit. There was already a PR in NuGet.Client open but it was closed because of staleness. - System.Data.Common.csproj is a weird project as it references CoreLib and reference assemblies. I had to disable transitive project references in order for type universes to not clash and explicitly set CompileUsingReferenceAssemblies=true as that gets set to false when the library explicitly references CoreLib.
There are numerous benefits in using ProjectReferences consistently in all libraries: 1. An upfront "libs" build isn't required anymore and sfx libraries can now directly be built from a fresh clone (with dotnet build or inside VS). I.e. `dotnet.cmd pack src/libraries/System.Text.Json/src/` is now possible from a fresh clone. 2. Because of 1), we can now add a solution file for the whole sfx that can directly be opened and worked with from a fresh clone. 3. The overall root build is faster. Without this change, the build order was sfx-ref -> (sfx-src & sfx-gen) so the shared framework reference projects first had to be built and only then the sfx src and gen projects could be built. Now with this change, everything gets built in parallel. 4. Using P2Ps means that we now follow the common and well supported msbuild and SDK path instead of repo customization. The downside of doing this is that the dependency graph gets bigger, meaning that more projects get incrementally built when doing a "dotnet build". This is nothing new and the SDK team recommends to pass the "--no-dependencies" flag to "dotnet build" if incrementally (no-op) building the additional dependency nodes is noticeable. This is less of a concern inside VS as that has a "fast up-to-date check" feature that doesn't even attempt to build projects that didn't change. For VS, really the only noticeable change is that the solution explorer now lists more projects and that when opening a solution, more projects need to be evaluated. But, that should be fast enough when using an up-to-date version of VS. - A few observations that make the change more involved: There's a NuGet client bug that requires a few workarounds: NuGet/Home#10368 Because of that, direct package refs to System.Numerics.Vectors and in a few cases to System.Memory are required. We should fix the NuGet tooling issue to eventually get rid of the workarounds introduced with this commit. There was already a PR in NuGet.Client open but it was closed because of staleness. - System.Data.Common.csproj is a weird project as it references CoreLib and reference assemblies. I had to disable transitive project references in order for type universes to not clash and explicitly set CompileUsingReferenceAssemblies=true as that gets set to false when the library explicitly references CoreLib.
There are numerous benefits in using ProjectReferences consistently in all libraries: 1. An upfront "libs" build isn't required anymore and sfx libraries can now directly be built from a fresh clone (with dotnet build or inside VS). I.e. `dotnet.cmd pack src/libraries/System.Text.Json/src/` is now possible from a fresh clone. 2. Because of 1), we can now add a solution file for the whole sfx that can directly be opened and worked with from a fresh clone. 3. The overall root build is faster. Without this change, the build order was sfx-ref -> (sfx-src & sfx-gen) so the shared framework reference projects first had to be built and only then the sfx src and gen projects could be built. Now with this change, everything gets built in parallel. 4. Using P2Ps means that we now follow the common and well supported msbuild and SDK path instead of repo customization. The downside of doing this is that the dependency graph gets bigger, meaning that more projects get incrementally built when doing a "dotnet build". This is nothing new and the SDK team recommends to pass the "--no-dependencies" flag to "dotnet build" if incrementally (no-op) building the additional dependency nodes is noticeable. This is less of a concern inside VS as that has a "fast up-to-date check" feature that doesn't even attempt to build projects that didn't change. For VS, really the only noticeable change is that the solution explorer now lists more projects and that when opening a solution, more projects need to be evaluated. But, that should be fast enough when using an up-to-date version of VS. - A few observations that make the change more involved: There's a NuGet client bug that requires a few workarounds: NuGet/Home#10368 Because of that, as a workaround, PackageId had to be set to a different string for S.Numerics.Vectors and System.Memory. We should fix the NuGet tooling issue to eventually get rid of the workarounds introduced with this commit. There was already a PR in NuGet.Client open but it was closed because of staleness. - System.Data.Common.csproj is a weird project as it references CoreLib and reference assemblies. I had to disable transitive project references in order for type universes to not clash and explicitly set CompileUsingReferenceAssemblies=true as that gets set to false when the library explicitly references CoreLib.
Fixes: NuGet/Home#10368 Re-submit of NuGet#5452 NuGet gets confused with transitive project and package dependencies with the same identity (name) in a multi-targeting project and selects the wrong type (project instead of package). Projects are expected to be preferred over packages, but only when the version matches. If the version doesn't match, projects shouldn't be selected in frameworks which don't declare a ProjectReference to those projects.
* Runtime specific and doc file packaging fixes * Replace all remaining pkgprojs with NuGet Pack task * Avoid NuGet/Home/issues/10368 * Update PackageValidation package to latest
There are numerous benefits in using ProjectReferences consistently in all libraries: 1. An upfront "libs" build isn't required anymore and sfx libraries can now directly be built from a fresh clone (with dotnet build or inside VS). I.e. `dotnet.cmd pack src/libraries/System.Text.Json/src/` is now possible from a fresh clone. 2. Because of 1), we can now add a solution file for the whole sfx that can directly be opened and worked with from a fresh clone. 3. The overall root build is faster. Without this change, the build order was sfx-ref -> (sfx-src & sfx-gen) so the shared framework reference projects first had to be built and only then the sfx src and gen projects could be built. Now with this change, everything gets built in parallel. 4. Using P2Ps means that we now follow the common and well supported msbuild and SDK path instead of repo customization. The downside of doing this is that the dependency graph gets bigger, meaning that more projects get incrementally built when doing a "dotnet build". This is nothing new and the SDK team recommends to pass the "--no-dependencies" flag to "dotnet build" if incrementally (no-op) building the additional dependency nodes is noticeable. This is less of a concern inside VS as that has a "fast up-to-date check" feature that doesn't even attempt to build projects that didn't change. For VS, really the only noticeable change is that the solution explorer now lists more projects and that when opening a solution, more projects need to be evaluated. But, that should be fast enough when using an up-to-date version of VS. - A few observations that make the change more involved: There's a NuGet client bug that requires a few workarounds: NuGet/Home#10368 Because of that, as a workaround, PackageId had to be set to a different string for S.Numerics.Vectors and System.Memory. We should fix the NuGet tooling issue to eventually get rid of the workarounds introduced with this commit. There was already a PR in NuGet.Client open but it was closed because of staleness. - System.Data.Common.csproj is a weird project as it references CoreLib and reference assemblies. I had to disable transitive project references in order for type universes to not clash and explicitly set CompileUsingReferenceAssemblies=true as that gets set to false when the library explicitly references CoreLib.
Details about Problem
Full repro details at https://github.com/dagood/repro/tree/repro-restore-ref-same-key. I used the
mcr.microsoft.com/dotnet/sdk:5.0
Docker image to minimally repro, but this also repros the same on my Windows machine.System.Security.Cryptography.Cng.csproj
System.Security.Cryptography.Pkcs.csproj
As far as I know I haven't seen this work before.
We hit it in source-build while trying to build dotnet/runtime: dotnet/source-build#1845 (comment)
This might not be supported--it doesn't make a whole lot of sense to me for a project to intentionally do this--but the error message should be improved at least! 😄 It took a while to figure out what the message was complaining about, work around it in source-build, and write this small repro.
Detailed repro steps so we can see the same problem
Clone https://github.com/dagood/repro/tree/repro-restore-ref-same-key
(
git clone -b repro-restore-ref-same-key https://github.com/dagood/repro && cd repro
)Run
dotnet build ./System.Security.Cryptography.Pkcs/System.Security.Cryptography.Pkcs.csproj /bl
See this error:
/usr/share/dotnet/sdk/5.0.100/NuGet.targets(131,5): error : An item with the same key has already been added. Key: (System.Security.Cryptography.Cng, 5.0.0-rc.2.20475.5) [/work/System.Security.Cryptograp hy.Pkcs/System.Security.Cryptography.Pkcs.csproj]
Open
msbuild.binlog
to see full stack trace:Verbose Logs
Binlog: repro-restore-ref-same-key.zip
The text was updated successfully, but these errors were encountered: