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

Allow NuGet dependencies to exist with duplicate names but different dependency types #9642

Merged

Conversation

DavidBoike
Copy link
Contributor

@DavidBoike DavidBoike commented Apr 30, 2024

NuGetUpdater.Cli.exe discover was throwing "An item with teh same key has already been added" in the situation where a centrally-managed Directory.Build.props file would have 2 references for the same package based on a condition such as the repository using central package management.

This is an example that would make the tool throw from my company's centrally-managed Directory.Build.props file:

<ItemGroup Condition="'$(ManagePackageVersionsCentrally)' == 'true'">
  <GlobalPackageReference Include="Particular.Analyzers" Version="$(ParticularAnalyzersVersion)" />
</ItemGroup>

<ItemGroup Condition="'$(ManagePackageVersionsCentrally)' != 'true'">
  <PackageReference Include="Particular.Analyzers" Version="$(ParticularAnalyzersVersion)" PrivateAssets="All" />
</ItemGroup>

If central package management is enabled, then our company-managed Roslyn analyzers package is included as a GlobalPackageReference. If not, it's added to the project as a regular PackageReference.

But the duplicate name caused a problem with the discover code tried to do .ToDictionary(…) on the name alone, so this change uses a lookup to allow multiple values.

A test has been added that demonstrates the desired result.

@DavidBoike DavidBoike requested a review from a team as a code owner April 30, 2024 15:26
@github-actions github-actions bot added the L: dotnet:nuget NuGet packages via nuget or dotnet label Apr 30, 2024
@DavidBoike
Copy link
Contributor Author

I see that one of the tests in Specs / ci (nuget, nuget, nuget) is failing. I don't grok Ruby or RSpec very well, unfortunately, but I do see that the most recent run on main also had a failure in that test suite, although it's a different test. I don't know if the RSpec tests even run NuGetUpdater.Cli (I can't find any evidence that it does?) but I did take the failure I saw (getting 5 dependencies out of a 9-dependency packages.config file) and set up a different test in the .NET project tests, and the updater was still coming up with 9 dependencies there, so I don't know what's up with that.

@DavidBoike
Copy link
Contributor Author

Must have been a heisenbug, there was a new commit to main, and after I rebased all the tests came up green.

@thavaahariharangit thavaahariharangit merged commit 41313ee into dependabot:main May 2, 2024
50 checks passed
@brettfo
Copy link
Collaborator

brettfo commented May 2, 2024

@DavidBoike If you curious for the future, this repo only invokes the Ruby unit tests so to include the C# tests we got sneaky and run dotnet test ... here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: dotnet:nuget NuGet packages via nuget or dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Closing .NET PRs as "no longer needed" when dependency is still present and out-of-date
3 participants