dependabot / dependabot-core

🤖 Dependabot's core logic for creating update PRs.
https://docs.github.com/en/code-security/dependabot
MIT License
4.67k stars 1.01k forks source link

Leaf level dependencies should be updated first #6710

Open batkaevruslan opened 1 year ago

batkaevruslan commented 1 year ago

Is there an existing issue for this?

Package ecosystem

NuGet

Package manager version

No response

Language version

No response

Manifest location and content before the Dependabot update

https://github.com/batkaevruslan/dependabot_issues/blob/main/DependabotRepro/A/A.csproj https://github.com/batkaevruslan/dependabot_issues/blob/main/DependabotRepro/B/B.csproj

dependabot.yml content

https://github.com/batkaevruslan/dependabot_issues/blob/main/.github/dependabot.yml

Updated dependency

AspNetCore.HealthChecks.Rabbitmq from 5.0.2 to 6.0.2

What you expected to see, versus what you actually saw

I expect Dependabot to update dependency from the leaf level of dependency graph first: Microsoft.Extensions.Diagnostics.HealthChecks image

Unfortunately, the root dependency AspNetCore.HealthChecks.Rabbitmq is updated first. That gives me a non-buildable solution: image

In other words, I expect the Dependabot log for my repo to be:

updater | | Changes to Dependabot Pull Requests | updater | +---------+------------------------------------------------------------------------+ updater | | created | Microsoft.Extensions.Diagnostics.HealthChecks ( from 5.0.17 to 7.0.3 ) | updater | | created | AspNetCore.HealthChecks.Rabbitmq ( from 5.0.2 to 6.0.2 ) | updater | +---------+------------------------------------------------------------------------+

instead of the current:

updater | | Changes to Dependabot Pull Requests | updater | +---------+------------------------------------------------------------------------+ updater | | created | AspNetCore.HealthChecks.Rabbitmq ( from 5.0.2 to 6.0.2 ) | updater | | created | Microsoft.Extensions.Diagnostics.HealthChecks ( from 5.0.17 to 7.0.3 ) | updater | +---------+------------------------------------------------------------------------+

Native package manager behavior

No response

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

https://github.com/batkaevruslan/dependabot_issues/pull/1

Smallest manifest that reproduces the issue

Main branch can be built: https://github.com/batkaevruslan/dependabot_issues

Dependabot's branch that fails to build: https://github.com/batkaevruslan/dependabot_issues/tree/dependabot/nuget/DependabotRepro/main/AspNetCore.HealthChecks.Rabbitmq-6.0.2

My branch with suggested update order (when Microsoft.Extensions.Diagnostics.HealthChecks is updated first): https://github.com/batkaevruslan/dependabot_issues/tree/chore/update-Microsoft.Extensions.Diagnostics.HealthChecks

deivid-rodriguez commented 1 year ago

Hello!

Please forgive my ignorance of the nuget ecosystem. Are these dependencies force-linked together? In general, Dependabot should not create any PRs that when merged, break a project, so I think if AspNetCore.HealthChecks.Rabbitmq needs a newer version of Microsoft.Extensions.Diagnostics.HealthChecks, then the AspNetCore.HealthChecks.Rabbitmq PR should include updates to both versions at the same time.

batkaevruslan commented 1 year ago

@deivid-rodriguez , hi!

Are these dependencies force-linked together?

I'm not sure what "force-linking" means :)

I think this can help: https://learn.microsoft.com/en-us/nuget/concepts/dependency-resolution#direct-dependency-wins

Warning

The Direct dependency wins rule can result in a downgrade of the package version, thus potentially breaking other dependencies in the graph.

So I expect Dependabot to create PR with Microsoft.Extensions.Diagnostics.HealthChecks first or, as you suggested, "PR should include updates to both versions at the same time".

deivid-rodriguez commented 1 year ago

I'm not sure what "force-linking" means :)

Haha, of course, I think I made that up. What I meant is weather AspNetCore.HealthChecks.Rabbitmq has a dependency on Microsoft.Extensions.Diagnostics.HealthChecks. If it does, then Dependabot should never create a PR to upgrade AspNetCore.HealthChecks.Rabbitmq to a version incompatible with the existing version of Microsoft.Extensions.Diagnostics.HealthChecks.

batkaevruslan commented 1 year ago

What I meant is weather AspNetCore.HealthChecks.Rabbitmq has a dependency on Microsoft.Extensions.Diagnostics.HealthChecks

Yes, it has. https://www.nuget.org/packages/AspNetCore.HealthChecks.Rabbitmq/ image

batkaevruslan commented 1 year ago

@deivid-rodriguez , if it's a bug, will it be fixed? If yes, could you give us ETA (in weeks\months)?

deivid-rodriguez commented 1 year ago

Hi @batkaevruslan, no ETA, sorry.

The good news is that this repo is open source, so you can try to fix the issue yourself and help us out! We really appreciate people familiarized with the package managers we support getting involved, because you usually know your package manager better than us.

If you clone this repository, then assuming you have docker installed, you can bin/docker-dev-shell nuget from the project root. Then inside the container, running bin/dry-run.rb nuget batkaevruslan/dependabot_issues --dir /DependabotRepro --dep AspNetCore.HealthChecks.Rabbitmq should reproduce this (incorrect diff that only bumps AspNetCore.HealthChecks.Rabbitmq). That's a good starting point to start digging.

Something else that usually helps is to answer the question "How would you go about updating this dependency yourself?" Does the native package manager provide some command that you can run, or do you have to check dependencies manually? Answering that question goes a long way since Dependabot "just" needs to imitate what users would do themselves.

deivid-rodriguez commented 11 months ago

This will be fixed by #8179 🎉

deivid-rodriguez commented 10 months ago

Unfortunately we regressed here, reopening.