Grouped dependabot updates for NuGet updates more packages than it should

martincostello commented 8 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Package ecosystem

NuGet

Package manager version

.NET SDK 8.0.100

Language version

C# 12

Manifest location and content before the Dependabot update

Directory.Packages.props

dependabot.yml content

dependabot.yml

Updated dependency

xunit 2.6.2 => 2.6.3

What you expected to see, versus what you actually saw

Dependabot should have updated xunit, and only xunit, to 2.6.3.

Instead it also updates xunit.abstractions and xunit.extensibility.execution which are explicitly marked as ignored in dependabot.yml.

Expected

  <PackageVersion Include="ReportGenerator" Version="5.2.0" />
  <PackageVersion Include="Shouldly" Version="4.2.1" />
  <PackageVersion Include="StyleCop.Analyzers" Version="1.2.0-beta.507" />
- <PackageVersion Include="xunit" Version="2.6.2" />
+ <PackageVersion Include="xunit" Version="2.6.3" />
  <PackageVersion Include="xunit.abstractions" Version="2.0.2" />
  <PackageVersion Include="xunit.extensibility.execution" Version="2.4.0" />
  <PackageVersion Include="xunit.runner.visualstudio" Version="2.5.5" />

Actual

  <PackageVersion Include="ReportGenerator" Version="5.2.0" />
  <PackageVersion Include="Shouldly" Version="4.2.1" />
  <PackageVersion Include="StyleCop.Analyzers" Version="1.2.0-beta.507" />
- <PackageVersion Include="xunit" Version="2.6.2" />
- <PackageVersion Include="xunit.abstractions" Version="2.0.2" />
- <PackageVersion Include="xunit.extensibility.execution" Version="2.4.0" />
+ <PackageVersion Include="xunit" Version="2.6.3" />
+ <PackageVersion Include="xunit.abstractions" Version="2.0.3" />
+ <PackageVersion Include="xunit.extensibility.execution" Version="2.6.3" />
  <PackageVersion Include="xunit.runner.visualstudio" Version="2.5.5" />

Native package manager behavior

No response

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

https://github.com/martincostello/xunit-logging/pull/533

Smallest manifest that reproduces the issue

No response

martincostello commented 8 months ago

Relevant logs:

// snipped
updater | 2023/12/09 16:29:57 INFO <job_760260334> All versions of Microsoft.Extensions.Logging ignored, no update allowed
updater | 2023/12/09 16:29:57 INFO <job_760260334> All versions of xunit.abstractions ignored, no update allowed
updater | 2023/12/09 16:29:57 INFO <job_760260334> All versions of xunit.extensibility.execution ignored, no update allowed
updater | 2023/12/09 16:29:57 INFO <job_760260334> Starting grouped update job for martincostello/xunit-logging
updater | 2023/12/09 16:29:57 INFO <job_760260334> Found 1 group(s).
updater | 2023/12/09 16:29:57 INFO <job_760260334> Starting update group for 'xunit'
updater | 2023/12/09 16:29:57 INFO <job_760260334> Checking if xunit 2.6.2 needs updating
  proxy | 2023/12/09 16:29:57 [422] GET https://azuresearch-usnc.nuget.org:443/query?q=xunit&prerelease=true&semVerLevel=2.0.0
  proxy | 2023/12/09 16:29:57 [422] 200 https://azuresearch-usnc.nuget.org:443/query?q=xunit&prerelease=true&semVerLevel=2.0.0
  proxy | 2023/12/09 16:29:57 [426] GET https://api.nuget.org:443/v3-flatcontainer/xunit/2.6.2/xunit.nuspec
  proxy | 2023/12/09 16:29:57 [426] 200 https://api.nuget.org:443/v3-flatcontainer/xunit/2.6.2/xunit.nuspec
  proxy | 2023/12/09 16:29:57 [428] GET https://api.nuget.org:443/v3-flatcontainer/xunit/2.6.2/xunit.2.6.2.nupkg
  proxy | 2023/12/09 16:29:57 [428] 200 https://api.nuget.org:443/v3-flatcontainer/xunit/2.6.2/xunit.2.6.2.nupkg
  proxy | 2023/12/09 16:29:58 [430] GET https://api.nuget.org:443/v3-flatcontainer/xunit/2.6.3/xunit.nuspec
  proxy | 2023/12/09 16:29:58 [430] 200 https://api.nuget.org:443/v3-flatcontainer/xunit/2.6.3/xunit.nuspec
  proxy | 2023/12/09 16:29:58 [432] GET https://api.nuget.org:443/v3-flatcontainer/xunit/2.6.3/xunit.2.6.3.nupkg
  proxy | 2023/12/09 16:29:58 [432] 200 https://api.nuget.org:443/v3-flatcontainer/xunit/2.6.3/xunit.2.6.3.nupkg
updater | 2023/12/09 16:29:58 INFO <job_760260334> Latest version is 2.6.3
updater | 2023/12/09 16:29:58 INFO <job_760260334> Requirements to unlock all
updater | 2023/12/09 16:29:58 INFO <job_760260334> Requirements update strategy 
updater | Finding updated dependencies for xunit.
  proxy | 2023/12/09 16:29:58 [434] GET https://api.nuget.org:443/v3-flatcontainer/xunit/2.6.3/xunit.nuspec
  proxy | 2023/12/09 16:29:58 [434] 200 https://api.nuget.org:443/v3-flatcontainer/xunit/2.6.3/xunit.nuspec
updater | 2023/12/09 16:29:58 INFO <job_760260334> Updating xunit from 2.6.2 to 2.6.3
updater | running NuGet updater:
updater | /opt/nuget/NuGetUpdater/NuGetUpdater.Cli update --repo-root /home/dependabot/dependabot-updater/repo --solution-or-project /home/dependabot/dependabot-updater/repo/tests/Logging.XUnit.Tests/MartinCostello.Logging.XUnit.Tests.csproj --dependency xunit --new-version 2.6.3 --previous-version 2.6.2  --verbose
// snipped
updater |   Updating global.json files.
updater |     Dependency [xunit] not found in any global.json files.
updater |   No dotnet-tools.json files found.
updater | Running for project [/home/dependabot/dependabot-updater/repo/tests/Logging.XUnit.Tests/MartinCostello.Logging.XUnit.Tests.csproj]
updater |   Running for SDK-style project
updater |     Found incorrect [PackageVersion] version attribute in [Directory.Packages.props].
updater |     Found incorrect peer [PackageVersion] version attribute in [Directory.Packages.props].
updater |     Found incorrect peer [PackageVersion] version attribute in [Directory.Packages.props].
updater |     Saved [Directory.Packages.props].
updater | Update complete.
updater | The contents of file [Directory.Packages.props] were updated.
// snipped
updater | 2023/12/09 16:30:38 INFO Results:
updater | +-----------------------------------------+
updater | |   Changes to Dependabot Pull Requests   |
updater | +---------+-------------------------------+
updater | | created | xunit ( from 2.6.2 to 2.6.3 ) |
updater | +---------+-------------------------------+

martincostello commented 6 months ago

Another case of a package being updated that is explicitly marked as ignored in the Dependabot configuration: https://github.com/martincostello/sqllocaldb/pull/786

abdulapopoola commented 5 months ago

@martincostello ; the crew has been shipping a lot of fixes in this area; can you confirm if this still repros?

martincostello commented 5 months ago

When did the possible fix for this get shipped? I was observing issues like this yesterday, e.g. https://github.com/App-vNext/Polly-Samples/pull/90

abdulapopoola commented 5 months ago

We've been shipping multiple daily fixes all week. IIUC, the issue is that grouping does not respect ignore conditions?

Tagging @jurre , @jakecoffman , @Nishnha as fyi

martincostello commented 5 months ago

It's possible I confused two different issues I logged you've commented on today. The above example was of a package that should have been updated as part of a group not being updated.

abdulapopoola commented 5 months ago

Yeah; I've been a bit active and you are just as active 😄

It does seem that Polly got updated in a group; is the issue now resolved then?

jakecoffman commented 5 months ago

It looks like xunit was updated, but the native NuGet Updater updated too many xunit-related dependencies?

I'd have to defer to @dependabot/azure-dev-ops's expertise on this one.

martincostello commented 4 months ago

In the example I gave the problem was that it should have updated three Polly packages, but it only updated two. I had to manually patch the third to get the intended result.

abdulapopoola commented 4 months ago

Got it; tagging @sebasgomez238

JoeRobich commented 4 months ago

These are peer dependencies that are required to be updated to avoid a package downgrade error when updating xunit. If ignoring these packages should trump updating xunit, then we should consider this a bug in the updater.

xunit 2.6.3 depends on xunit.core 2.6.3

xunit.core 2.6.3 depends on xunit.extensibility.execution 2.6.3

xunit.core 2.6.3 also depends on xunit.extensibility.core 2.6.3

xunit.extensibility.core 2.6.3 depends on xunit.abstractions 2.0.3

martincostello commented 4 months ago

That's true, but the reference I'm ignoring is for a library that only depends on the one I'm ignoring explicitly. That project's tests don't reference that package explicitly, and use the other two and transiently reference the one I'm ignoring.

martincostello commented 4 months ago

Essentially, I have one project that references the extensibility library which I never want dependabot to update because I want to target the lowest common denominator.

Then I have the test project for that library that references the other two I always want to be kept up to date as a pair.

The issue here is that dependabot is ignoring the ignore, but also not taking into account which projects are using the versions that are in the Directory.Packages.props file.

JoeRobich commented 4 months ago

@martincostello Thanks for pointing that out. I agree there is a bug here. We need to do a better job updating dependencies based on how the project sees them instead of how they exist in the workspace as a whole.

martincostello commented 4 months ago

I have a similar (or same) issue with this repository. Dependabot seems to get confused in a multi-targeting scenario and errors:

updater | Running for project file [src/MartinCostello.BrowserStack.Automate/MartinCostello.BrowserStack.Automate.csproj]
updater | Updating project [/home/dependabot/dependabot-updater/repo/src/MartinCostello.BrowserStack.Automate/MartinCostello.BrowserStack.Automate.csproj]
updater |   Updating [global.json] file.
updater |     Dependency [Microsoft.AspNetCore.WebUtilities] not found.
updater |   Running for SDK-style project
updater | dotnet build in GetAllPackageDependenciesAsync failed. STDOUT: MSBuild version 17.9.6+a4ecab324 for .NET
updater |   Determining projects to restore...
updater | /tmp/package-dependency-resolution_F5Zkcw/Project.csproj : error NU1202: Package Microsoft.AspNetCore.WebUtilities 8.0.0 is not compatible with netstandard2.0 (.NETStandard,Version=v2.0). Package Microsoft.AspNetCore.WebUtilities 8.0.0 supports: net8.0 (.NETCoreApp,Version=v8.0)
updater |   Failed to restore /tmp/package-dependency-resolution_F5Zkcw/Project.csproj (in 1.4 sec).
updater | 
updater | Build FAILED.
updater | 
updater | /tmp/package-dependency-resolution_F5Zkcw/Project.csproj : error NU1202: Package Microsoft.AspNetCore.WebUtilities 8.0.0 is not compatible with netstandard2.0 (.NETStandard,Version=v2.0). Package Microsoft.AspNetCore.WebUtilities 8.0.0 supports: net8.0 (.NETCoreApp,Version=v8.0)
updater |     0 Warning(s)
updater |     1 Error(s)
updater | 
updater | Time Elapsed 00:00:02.50
updater | 
updater |  STDERR: 
updater | 
updater |     Package [Microsoft.AspNetCore.WebUtilities] already meets the requested dependency version in [/home/dependabot/dependabot-updater/repo/src/MartinCostello.BrowserStack.Automate/MartinCostello.BrowserStack.Automate.csproj].
updater | Update complete.

Then in another repo it appears to be getting the dependency chain wrong(?) and failing:

updater | Running for project file [src/SqlLocalDb/MartinCostello.SqlLocalDb.csproj]
updater | Updating project [/home/dependabot/dependabot-updater/repo/src/SqlLocalDb/MartinCostello.SqlLocalDb.csproj]
updater |   Updating [global.json] file.
updater |     Dependency [Microsoft.Data.SqlClient] not found.
updater |   Running for SDK-style project
updater | dotnet build in GetAllPackageDependenciesAsync failed. STDOUT: MSBuild version 17.9.6+a4ecab324 for .NET
updater |   Determining projects to restore...
updater | /tmp/package-dependency-resolution_eA3afe/Project.csproj : error NU1605: Warning As Error: Detected package downgrade: Microsoft.Win32.Registry from 5.0.0 to 4.7.0. Reference the package directly from the project to select a different version. 
updater | /tmp/package-dependency-resolution_eA3afe/Project.csproj : error NU1605:  Project -> Microsoft.Data.SqlClient 5.1.5 -> Microsoft.Win32.Registry (>= 5.0.0) 
updater | /tmp/package-dependency-resolution_eA3afe/Project.csproj : error NU1605:  Project -> Microsoft.Win32.Registry (= 4.7.0)
updater |   Failed to restore /tmp/package-dependency-resolution_eA3afe/Project.csproj (in 3.75 sec).
updater | 
updater | Build FAILED.
updater | 
updater | /tmp/package-dependency-resolution_eA3afe/Project.csproj : error NU1605: Warning As Error: Detected package downgrade: Microsoft.Win32.Registry from 5.0.0 to 4.7.0. Reference the package directly from the project to select a different version. 
updater | /tmp/package-dependency-resolution_eA3afe/Project.csproj : error NU1605:  Project -> Microsoft.Data.SqlClient 5.1.5 -> Microsoft.Win32.Registry (>= 5.0.0) 
updater | /tmp/package-dependency-resolution_eA3afe/Project.csproj : error NU1605:  Project -> Microsoft.Win32.Registry (= 4.7.0)
updater |     0 Warning(s)
updater |     1 Error(s)
updater | 
updater | Time Elapsed 00:00:04.95

martincostello commented 4 months ago

This morning dependabot has started opening PRs which either mention dependencies it states it has updated which are not present in the diff, or it generates package updates that should not have occurred.

https://github.com/justeattakeaway/JustEat.StatsD/pull/649 - The title states that System.Memory has been updated, but the diff instead contains a reference to Microsoft.Extensions.DependencyInjection.Abstractions. This dependency update is not required as the project which references BenchmarkDotNet does not explicitly reference that package, and it is also explicitly ignored.
https://github.com/justeattakeaway/JustSaying/pull/1383 - Similarly updates ignored packages and packages that are not referenced by the projects containing the top-level dependency being updated.
https://github.com/justeattakeaway/JustSaying/pull/1382 - States that Microsoft.Bcl.AsyncInterfaces has been updated, but isn't in the diff (and if it was, shouldn't be for the same reasons as above regarding the project-level references).
https://github.com/justeattakeaway/JustSaying/pull/1381 - States that System.Threading.Channels has been updated, but again not in the diff or a package that needs updating.

Presumably a recent change has made this issue more prevalent than it was previously.

martincostello commented 4 months ago

Another interesting weird behaviour that just appeared - dependabot opened two PRs similar to the above updating things it shouldn't, but one of the two PRs then was closed as superseded by the second.

carlincherry commented 3 months ago

@brettfo I'm triaging bugs with @abdulapopoola and came across this; how's it going? 🙏

brettfo commented 3 months ago

@martincostello I'm trying to understand the original issue, but I'm not quite there: xUnit can't update from 2.6.2 to 2.6.3 because doing so would force xunit.abstractions to go from 2.0.2 to 2.0.3, so what would the desired output be? It can't be that xunit goes 2.6.2 => 2.6.3, but xunit.abstractions remains the same, because that's not a valid package graph. Is the desired behavior that the update from 2.6.2 doesn't happen because that would violate the other constraint?

Edit: I looked through the repo a bit and while the <PackageReference> elements are in separate projects, e.g., src/ has xunit.abstractions and test/ has xunit, there's still a <ProjectReference> from the test/ project to the src/ project which still means it wouldn't work.

martincostello commented 3 months ago

It definitely works, otherwise I wouldn't be able to manually update the reference because the code wouldn't compile.

My understanding is that as xunit itself depends on a higher version of the package, it can be updated without also updating the reference in the referenced project as the test project will get the version xunit wants.

If the graph traversal to derive that is too complicated, I would say consider the configuration of ignores to be a "I know better leave it alone" directive and for dependabot to honour it.

martincostello commented 3 months ago

On reflection, in fact yes, it's more that it's not honouring my ignores. "Update everything that matches xunit* except the ones I explicitly told you not to".

I could change the group pattern to explicitly bump the two I want updated together, but I think it's counter-intuitive that ignores aren't being ignored.

martincostello commented 3 months ago

I could change the group pattern to explicitly bump the two I want updated together

I've done this anyway, but updates are broken due to #9495 so I can't tell if it's made any difference.

brettfo commented 2 months ago

@martincostello the linked issue has now been fixed, are you still seeing this issue?

martincostello commented 2 months ago

Now I have a new problem and xunit isn't being updated at all.

Unhandled exception: System.ArgumentException: '1.2.0.556' is not a valid version string. (Parameter 'value')
   at NuGet.Versioning.SemanticVersion.Parse(String value) in /opt/nuget/lib/NuGet.Client/src/NuGet.Core/NuGet.Versioning/SemanticVersionFactory.cs:line 22
   at NuGetUpdater.Core.Discover.SdkProjectDiscovery.GetTransitiveDependencies(String repoRootPath, String projectPath, ImmutableArray`1 tfms, ImmutableArray`1 directDependencies, Logger logger) in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Core/Discover/SdkProjectDiscovery.cs:line 114
   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 70
   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
   at NuGetUpdater.Core.Discover.DiscoveryWorker.RunForDirectoryAsnyc(String repoRootPath, String workspacePath) in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Core/Discover/DiscoveryWorker.cs:line 125
   at NuGetUpdater.Core.Discover.DiscoveryWorker.RunAsync(String repoRootPath, String workspacePath, String outputPath) in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Core/Discover/DiscoveryWorker.cs:line 59
   at NuGetUpdater.Cli.Commands.DiscoverCommand.<>c.<<GetCommand>b__4_0>d.MoveNext() in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Cli/Commands/DiscoverCommand.cs:line 30

abdulapopoola commented 2 months ago

@martincostello , this PR should fix the issue. Can you recheck again?

martincostello commented 2 months ago

Still updates ignored packages:

martincostello commented 2 months ago

In a different repo I tried to update xunit and spotted this error in the logs after nothing happened:

updater | Unhandled exception: System.ArgumentException: ".." can be only added at the beginning of the pattern.
updater |    at Microsoft.Extensions.FileSystemGlobbing.Internal.Patterns.PatternBuilder.Build(String pattern)
updater |    at Microsoft.Extensions.FileSystemGlobbing.Matcher.AddInclude(String pattern)
updater |    at NuGetUpdater.Core.MSBuildHelper.GetProjectPathsFromProject(String projFilePath)+MoveNext() in /opt/nuget/lib/NuGetUpdater/NuGetUpdater.Core/Utilities/MSBuildHelper.cs:line 92
updater |    at System.Linq.Enumerable.SelectEnumerableIterator`2.ToArray()
updater |    at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
updater |    at System.Linq.OrderedEnumerable`1.ToArray()
updater |    at System.Collections.Immutable.ImmutableArray.CreateRange[T](IEnumerable`1 items)
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 60
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():

dependabot / dependabot-core