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

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

Open
dagood opened this issue Dec 8, 2020 · 16 comments · May be fixed by NuGet/NuGet.Client#5985
Assignees
Labels
Category:Quality Week Issues that should be considered for quality week Functionality:Restore Priority:2 Issues for the current backlog. Style:PackageReference Type:Bug

Comments

@dagood
Copy link

dagood commented Dec 8, 2020

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.

$ dotnet --info
.NET SDK (reflecting any global.json):
 Version:   5.0.100
 Commit:    5044b93829

Runtime Environment:
 OS Name:     debian
 OS Version:  10
 OS Platform: Linux
 RID:         debian.10-x64
 Base Path:   /usr/share/dotnet/sdk/5.0.100/

Host (useful for support):
  Version: 5.0.0
  Commit:  cf258a14b7

.NET SDKs installed:
  5.0.100 [/usr/share/dotnet/sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 5.0.0 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 5.0.0 [/usr/share/dotnet/shared/Microsoft.NETCore.App]

System.Security.Cryptography.Cng.csproj

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net5.0</TargetFramework>
    <Version>5.0.0-rc.2.20475.5</Version>
  </PropertyGroup>
</Project>

System.Security.Cryptography.Pkcs.csproj

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFrameworks>net5.0;netcoreapp3.1</TargetFrameworks>
  </PropertyGroup>
  <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>
</Project>

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

  1. 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)

  2. Run dotnet build ./System.Security.Cryptography.Pkcs/System.Security.Cryptography.Pkcs.csproj /bl

  3. 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]

  4. Open msbuild.binlog to see full stack trace:

System.ArgumentException: An item with the same key has already been added. Key: (System.Security.Cryptography.Cng, 5.0.0-rc.2.20475.5)
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
   at System.Linq.Enumerable.ToDictionary[TSource,TKey](List`1 source, Func`2 keySelector, IEqualityComparer`1 comparer)
   at System.Linq.Enumerable.ToDictionary[TSource,TKey](IEnumerable`1 source, Func`2 keySelector, IEqualityComparer`1 comparer)
   at System.Linq.Enumerable.ToDictionary[TSource,TKey](IEnumerable`1 source, Func`2 keySelector)
   at NuGet.Commands.LockFileBuilder.CreateLockFile(LockFile previousLockFile, PackageSpec project, IEnumerable`1 targetGraphs, IReadOnlyList`1 localRepositories, RemoteWalkContext context)
   at NuGet.Commands.RestoreCommand.ExecuteAsync(CancellationToken token)
   at NuGet.Commands.RestoreRunner.ExecuteAsync(RestoreSummaryRequest summaryRequest, CancellationToken token)
   at NuGet.Commands.RestoreRunner.ExecuteAndCommitAsync(RestoreSummaryRequest summaryRequest, CancellationToken token)
   at NuGet.Commands.RestoreRunner.CompleteTaskAsync(List`1 restoreTasks)
   at NuGet.Commands.RestoreRunner.RunAsync(IEnumerable`1 restoreRequests, RestoreArgs restoreContext, CancellationToken token)
   at NuGet.Commands.RestoreRunner.RunAsync(RestoreArgs restoreContext, CancellationToken token)
   at NuGet.Build.Tasks.BuildTasksUtility.RestoreAsync(DependencyGraphSpec dependencyGraphSpec, Boolean interactive, Boolean recursive, Boolean noCache, Boolean ignoreFailedSources, Boolean disableParallel, Boolean force, Boolean forceEvaluate, Boolean hideWarningsAndErrors, Boolean restorePC, Boolean cleanupAssetsForUnsupportedProjects, ILogger log, CancellationToken cancellationToken)
   at NuGet.Build.Tasks.RestoreTask.ExecuteAsync(ILogger log)

Verbose Logs

Binlog: repro-restore-ref-same-key.zip

@nkolev92
Copy link
Member

nkolev92 commented Dec 8, 2020

Known issue that @erdembayar is actively working on.
Maybe he can find the dup.

@erdembayar
Copy link
Contributor

Known issue that @erdembayar is actively working on.
Maybe he can find the dup.

It's duplicate of Issue6795.
Draft PR for fix is here:
NuGet/NuGet.Client#3789

@nkolev92 nkolev92 added Functionality:Restore Resolution:Duplicate This issue appears to be a Duplicate of another issue Type:Bug labels Dec 8, 2020
@nkolev92 nkolev92 closed this as completed Dec 8, 2020
@dagood
Copy link
Author

dagood commented Dec 8, 2020

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 net5.0, it selects the project reference. When building netcoreapp3.1, it selects the package reference. That's based on these conditions:

  <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 PackageReference into a ProjectReference when that isn't what you'd expect from the MSBuild code. 😕

@nkolev92
Copy link
Member

nkolev92 commented Feb 4, 2021

It sounds like that PR might actually make things worse by silently transforming the PackageReference into a ProjectReference when that isn't what you'd expect from the MSBuild code

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.

@nkolev92 nkolev92 reopened this Feb 4, 2021
@nkolev92 nkolev92 added Priority:2 Issues for the current backlog. Style:PackageReference Pipeline:Backlog and removed Resolution:Duplicate This issue appears to be a Duplicate of another issue labels Feb 4, 2021
@dagood
Copy link
Author

dagood commented Feb 5, 2021

Not quite sure my expectations are clear... restating just in case:

  • When TargetFramework == net5.0, there's a ProjectReference.
  • When TargetFramework == netcoreapp3.1, there's a PackageReference.

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" TargetFramework?

At the moment this feels like an arbitrary limitation (maybe of the project.assets.json format?) rather than something intentional. The scenario is pretty wild, so I understand we might run into a problem like this, but if this is intentional, it shakes up something I thought I understood about NuGet, so I'd like to know more.

@ViktorHofer
Copy link

ViktorHofer commented Mar 10, 2021

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:

C:\temp\nugetnodes\parent>dotnet restore
  Determining projects to restore...
C:\temp\nugetnodes\parent\parent.csproj : error NU1201: Project System.Numerics.Vectors is not compatible with netstandard2.0 (.NETStandard,Version=v2.0). Project System.Numerics.Vectors supports: net5.0 (.NETCoreApp,Version=v5.0)
  Restored C:\temp\nugetnodes\child1\child1.csproj (in 276 ms).
  Failed to restore C:\temp\nugetnodes\parent\parent.csproj (in 276 ms).

This is causing unreasonable pain in dotnet/runtime in which we often reference via <PackageReference /> and <ProjectReference /> based on the TFM but IMHO is a bug that any customer could easily encounter when multi-targeting and conditionally referencing based on the inner-build TFM.

@nkolev92 could we please raise the priority of this bug?

@ViktorHofer
Copy link

This is still causing unreasonable pain. @nkolev92 @rrelyea have you had a chance to take a look?

@erdembayar
Copy link
Contributor

erdembayar commented Jun 14, 2021

@ViktorHofer
We already have fix for this. NuGet/NuGet.Client#3789
But this is hot code path so need perf analysis testing to make sure new change is doesn't add weight. Due other urgent priorities I didn't finish that part. I'll try to finish in next 2-3 week, sorry for delay.

@dagood
Copy link
Author

dagood commented Jun 14, 2021

We already have fix for this. NuGet/NuGet.Client#3789

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:

Since we already have project over package preference I currently implemented this approach in my code.

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.)

@dagood
Copy link
Author

dagood commented Jun 14, 2021

Oh, that PR is the same one that this issue was mistakenly closed as a duplicate of once before. #10368 (comment)

It's duplicate of Issue6795.
Draft PR for fix is here:
NuGet/NuGet.Client#3789

I think this scenario is slightly different from the linked one, and as such I am re-opening it.

We already have fix for this. NuGet/NuGet.Client#3789

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.)

@ViktorHofer
Copy link

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).

@erdembayar
Copy link
Contributor

@ViktorHofer
Sorry, it was different issue. As you can see you already upvoted this issue.

ViktorHofer added a commit to ViktorHofer/runtime that referenced this issue Aug 4, 2021
ViktorHofer added a commit to dotnet/runtime that referenced this issue Aug 5, 2021
* 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
ViktorHofer added a commit to dotnet/winforms that referenced this issue Dec 5, 2022
…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
@ViktorHofer
Copy link

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.

cc @aortiz-msft @ericstj

@erdembayar
Copy link
Contributor

I would be willing to help but I have no idea where to start.

@ViktorHofer
I believe NuGet/NuGet.Client#3789 is very close one to what you're trying to do. Possible need to add TFM check before selecting the package vs project reference, tbh I haven't looked back at that code since then, so I don't know if it needs more changes other than TFM check.

@ViktorHofer
Copy link

ViktorHofer commented Sep 2, 2023

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 System.Numerics.Vectors.csproj from one TFM and another TFM package references System.Memory which transitively package references System.Numerics.Vectors, S.N.V incorrectly gets resolved as a P2P. I.e.: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Encodings.Web/src/System.Text.Encodings.Web.csproj#L67-L75

Error: C:\temp\tfmnugetoverlap\main\main.csproj : error NU1201: Project System.Numerics.Vectors is not compatible with netstandard2.0 (.NETStandard,Version=v2.0). Project System.Numerics.Vectors supports: net8.0 (.NETCoreApp,Version=v8.0)


I looked into this more closely and here's what I got so far.

Example 1 - Direct dependencies

The 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 TypeConstraint is Project (for the net8.0 TFM) and the TypeConstraint for the package LibraryRange is Package (for the netstandard2.0 TFM).

Example 2 - Transitive dependency only

The 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 TypeConstraint is Package. The transitive dependency (System.Numerics.Vectors/4.4.0)'s TypeConstraint is resolved as PackageProjectExternal because that's what NuGet does for transitives to allow P2Ps to win over PackageReferences.

In this case example, a ProjectReference for System.Numerics.Vectors doesn't exist and hence the transitive dependency is resolved as a package.

So far so good...

Example 3 - Matching transitive and direct dependency

The 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>

Error: C:\temp\tfmnugetoverlap\main\main.csproj : error NU1201: Project System.Numerics.Vectors is not compatible with netstandard2.0 (.NETStandard,Version=v2.0). Project System.Numerics.Vectors supports: net8.0 (.NETCoreApp,Version=v8.0)

First, the direct dependency for the net8.0 TFM is resolved as a project because the TypeConstraint is Project. That's expected and correct.

Now the netstandard2.0 TFM has a direct dependency to System.Memory which is resolved as package as the TypeConstraint of the dependency is Package. Still correct.

Now, the transitive dependency System.Numerics.Vectors is calculated. That dependency has a TypeConstraint of PackageProjectExternal (same as in example 2 above - because that's how NuGet implements P2Ps over PackageReferences). Now, because a ProjectReference with the same identity exists in the graph (even though not for the current evaluated TFM), the dependency is incorrectly resolved as a project.

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 projectProviders instance (?)) apply and which don't.

ViktorHofer added a commit to ViktorHofer/NuGet.Client that referenced this issue Sep 3, 2023
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.
@ViktorHofer
Copy link

ViktorHofer commented Sep 3, 2023

I came up with a "solution" and pushed a draft PR: NuGet/NuGet.Client#5397

Note that I had to touch public API (ResolverUtility) as the current API doesn't provide any information about the library's referencee.

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.

@nkolev92 nkolev92 self-assigned this Nov 1, 2023
@nkolev92 nkolev92 added the Category:Quality Week Issues that should be considered for quality week label Nov 6, 2023
carlossanlop pushed a commit to carlossanlop/maintenance-packages that referenced this issue Feb 5, 2024
* 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
ViktorHofer added a commit to ViktorHofer/runtime that referenced this issue Aug 17, 2024
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.
ViktorHofer added a commit to ViktorHofer/runtime that referenced this issue Aug 17, 2024
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.
ViktorHofer added a commit to ViktorHofer/runtime that referenced this issue Aug 20, 2024
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.
ViktorHofer added a commit to ViktorHofer/NuGet.Client that referenced this issue Aug 21, 2024
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.
carlossanlop pushed a commit to carlossanlop/maintenance-packages that referenced this issue Aug 28, 2024
* 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
ViktorHofer added a commit to ViktorHofer/runtime that referenced this issue Sep 3, 2024
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.
ViktorHofer added a commit to ViktorHofer/runtime that referenced this issue Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment