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

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

Closed
1 task done
DavidBoike opened this issue Apr 25, 2024 · 5 comments · Fixed by #9642
Closed
1 task done
Labels
L: dart:pub Dart packages via pub L: dotnet:nuget NuGet packages via nuget or dotnet L: git:submodules Git submodules T: bug 🐞 Something isn't working

Comments

@DavidBoike
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Package ecosystem

.NET

Package manager version

No response

Language version

No response

Manifest location and content before the Dependabot update

https://github.com/Particular/ServiceControl/blob/master/src/Directory.Packages.props#L17

dependabot.yml content

https://github.com/Particular/ServiceControl/blob/master/.github/dependabot.yml

Updated dependency

Microsoft.AspNetCore.Mvc.Testing from 8.0.3 to 8.0.4

What you expected to see, versus what you actually saw

Dependabot PR was approved, and auto-merge was enabled. Instead of merging, Dependabot left the comment:

Looks like Microsoft.AspNetCore.Mvc.Testing is no longer a dependency, so this is no longer needed.

Except it's definitely still used. A code search of the repo shows the version defined in https://github.com/Particular/ServiceControl/blob/da452ea82a2cf6aca420d73029c7861cc009c764/src/Directory.Packages.props#L17 as 8.0.3 and the package being referenced in several projects in the solution, while the actual current version of the package on NuGet is 8.0.4.

Native package manager behavior

N/A

Images of the diff or a link to the PR, issue, or logs

Particular/ServiceControl#4066

Smallest manifest that reproduces the issue

Not sure, but we have only seen this in:

  1. Repos with NuGet central package management via a Directory.Packages.props file.
  2. Private repos. I assume this is less helpful, but I can find examples of this too if necessary.
  3. Public repo example: Particular/NServiceBus.Transport.AzureServiceBus: Bump Azure.Messaging.ServiceBus from 7.17.2 to 7.17.5 but in this case, the version was bumped in a different PR, so the version is up-to-date, but Dependabot still used the incorrect "Looks like Azure.Messaging.ServiceBus is no longer a dependency, so this is no longer needed." response.
    • Unfortunately not able to find any public-repo-non-central-package-management cases where the dependency has been left un-updated.
@DavidBoike DavidBoike added the T: bug 🐞 Something isn't working label Apr 25, 2024
@github-actions github-actions bot added L: dart:pub Dart packages via pub L: dotnet:nuget NuGet packages via nuget or dotnet L: git:submodules Git submodules labels Apr 25, 2024
@DavidBoike
Copy link
Contributor Author

Here's another, on a public repo, after rebasing a PR, which left the dependency out of date:

Particular/TimeoutMigrationTool: Bump System.IdentityModel.Tokens.Jwt from 7.3.0 to 7.5.1 after a @dependabot rebase was closed with the comment "Looks like System.IdentityModel.Tokens.Jwt is no longer a dependency, so this is no longer needed." but the dependency still exists at version 7.3.0.

In the Dependabot log I do see an error:

updater | Unhandled exception: System.ArgumentException: An item with the same key has already been added. Key: Particular.Analyzers
updater |    at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
updater |    at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
updater |    at System.Linq.Enumerable.ToDictionary[TSource,TKey](IEnumerable`1 source, Func`2 keySelector, IEqualityComparer`1 comparer)
updater |    at NuGetUpdater.Core.Discover.SdkProjectDiscovery.DiscoverAsync(String repoRootPath, String workspacePath, String projectPath, Logger logger) in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Core/Discover/SdkProjectDiscovery.cs:line 32
updater |    at NuGetUpdater.Core.Discover.DiscoveryWorker.RunForProjectPathsAsync(String repoRootPath, String workspacePath, IEnumerable`1 projectPaths) in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Core/Discover/DiscoveryWorker.cs:line 160
updater |    at NuGetUpdater.Core.Discover.DiscoveryWorker.RunForDirectoryAsnyc(String repoRootPath, String workspacePath) in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Core/Discover/DiscoveryWorker.cs:line 125
updater |    at NuGetUpdater.Core.Discover.DiscoveryWorker.RunAsync(String repoRootPath, String workspacePath, String outputPath) in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Core/Discover/DiscoveryWorker.cs:line 59
updater |    at NuGetUpdater.Cli.Commands.DiscoverCommand.<>c.<<GetCommand>b__4_0>d.MoveNext() in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Cli/Commands/DiscoverCommand.cs:line 30
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Invocation.AnonymousCommandHandler.InvokeAsync(InvocationContext context)
updater |    at System.CommandLine.Invocation.InvocationPipeline.<>c__DisplayClass4_0.<<BuildInvocationChain>b__0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass17_0.<<UseParseErrorReporting>b__0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass12_0.<<UseHelp>b__0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass22_0.<<UseVersionOption>b__0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass19_0.<<UseTypoCorrections>b__0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<UseSuggestDirective>b__18_0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass16_0.<<UseParseDirective>b__0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<RegisterWithDotnetSuggest>b__5_0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass8_0.<<UseExceptionHandler>b__0>d.MoveNext():
updater | Discovering build files in workspace [/home/dependabot/dependabot-updater/repo].
updater |   No dotnet-tools.json file found.
updater |   No global.json file found.
updater |   Discovering projects beneath [.].
updater |   No packages.config file found.
updater | Unhandled exception: System.ArgumentException: An item with the same key has already been added. Key: Particular.Analyzers
updater |    at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
updater |    at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
updater |    at System.Linq.Enumerable.ToDictionary[TSource,TKey](IEnumerable`1 source, Func`2 keySelector, IEqualityComparer`1 comparer)
updater |    at NuGetUpdater.Core.Discover.SdkProjectDiscovery.DiscoverAsync(String repoRootPath, String workspacePath, String projectPath, Logger logger) in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Core/Discover/SdkProjectDiscovery.cs:line 32
updater |    at NuGetUpdater.Core.Discover.DiscoveryWorker.RunForProjectPathsAsync(String repoRootPath, String workspacePath, IEnumerable`1 projectPaths) in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Core/Discover/DiscoveryWorker.cs:line 160
updater |    at NuGetUpdater.Core.Discover.DiscoveryWorker.RunForDirectoryAsnyc(String repoRootPath, String workspacePath) in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Core/Discover/DiscoveryWorker.cs:line 125
updater |    at NuGetUpdater.Core.Discover.DiscoveryWorker.RunAsync(String repoRootPath, String workspacePath, String outputPath) in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Core/Discover/DiscoveryWorker.cs:line 59
updater |    at NuGetUpdater.Cli.Commands.DiscoverCommand.<>c.<<GetCommand>b__4_0>d.MoveNext() in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Cli/Commands/DiscoverCommand.cs:line 30
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Invocation.AnonymousCommandHandler.InvokeAsync(InvocationContext context)
updater |    at System.CommandLine.Invocation.InvocationPipeline.<>c__DisplayClass4_0.<<BuildInvocationChain>b__0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass17_0.<<UseParseErrorReporting>b__0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass12_0.<<UseHelp>b__0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass22_0.<<UseVersionOption>b__0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass19_0.<<UseTypoCorrections>b__0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<UseSuggestDirective>b__18_0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass16_0.<<UseParseDirective>b__0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<RegisterWithDotnetSuggest>b__5_0>d.MoveNext()
updater | --- End of stack trace from previous location ---
updater |    at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass8_0.<<UseExceptionHandler>b__0>d.MoveNext()
updater | 
updater | 2024/04/26 15:21:17 WARN <job_820286141> Please check your configuration as there are groups where no dependencies match:
updater | - awssdk
updater | - nservicebuscore
updater | 
updater | This can happen if:
updater | - the group's 'pattern' rules are misspelled
updater | - your configuration's 'allow' rules do not permit any of the dependencies that match the group
updater | - the dependencies that match the group rules have been removed from your project
updater | 
updater | 2024/04/26 15:21:17 INFO <job_820286141> Starting PR update job for Particular/TimeoutMigrationTool
updater | 2024/04/26 15:21:17 INFO <job_820286141> Telling backend to close pull request for System.IdentityModel.Tokens.Jwt - dependency removed
updater | 2024/04/26 15:21:18 INFO <job_820286141> Finished job processing
updater | 2024/04/26 15:21:18 INFO Results:
updater | +--------------------------------------------------------------+
updater | |             Changes to Dependabot Pull Requests              |
updater | +----------------------------+---------------------------------+
updater | | closed: dependency_removed | System.IdentityModel.Tokens.Jwt |
updater | +----------------------------+---------------------------------+
updater | time="2024-04-26T15:21:18Z" level=info msg="task complete" container_id=job-820286141-updater exit_code=0 job_id=820286141 step=updater

Is it possible that the updater continues after this error thinking there are no dependencies?

@DavidBoike
Copy link
Contributor Author

Note that we do have this in our Directory.Build.props, which is distributed organization-wide:

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

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

Could this be the source of the item already added to dictionary exception? Because this is completely valid in MSBuild and required to support projects that are configured with central package management as well as those that are not.

@brettfo
Copy link
Collaborator

brettfo commented May 1, 2024

Looks like the same exception and stack trace in #9495.

@DavidBoike
Copy link
Contributor Author

@brettfo
Copy link
Collaborator

brettfo commented May 1, 2024

@DavidBoike Thank you for calling my attention to this. I was doing a lightning triage this morning and skipped right past your PR. I've approved it and I'm working on finding somebody with merge permissions to finish it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: dart:pub Dart packages via pub L: dotnet:nuget NuGet packages via nuget or dotnet L: git:submodules Git submodules T: bug 🐞 Something isn't working
Projects
None yet
2 participants