dependabot / dependabot-core

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

PR not created for Framework-limited NuGet dependencies #10422

Closed DavidBoike closed 1 week ago

DavidBoike commented 1 month ago

Is there an existing issue for this?

Package ecosystem

NuGet

Package manager version

No response

Language version

.NET SDK 8.0.303

Manifest location and content before the Dependabot update

https://github.com/ParticularLabs/FrameworkLimitedDependencySample/blob/main/FrameworkLimitedDependencySample.csproj

dependabot.yml content

https://github.com/ParticularLabs/FrameworkLimitedDependencySample/blob/main/.github/dependabot.yml

Updated dependency

System.Management.Automation 7.2.21 => 7.4.4

What you expected to see, versus what you actually saw

The project is a minimal reproduction of a real-life error we are experiencing in a library as a dependency of PowerShell cmdlets.

The project in question targets net6.0-windows in order to remain compatible with PowerShell 7.2, which is the oldest current LTS release of PowerShell, which is based on .NET 6.

The newest version of System.Management.Automation (7.4.4) is only available for net8.0 because that's what PowerShell 7.4 is based on.

So when Dependabot runs:

This can be seen in a recent Dependabot log on GitHub Actions which I'll provide relevant bits of here:

Running for SDK-style project
dotnet build in GetAllPackageDependenciesAsync failed. STDOUT:   Determining projects to restore...
/tmp/package-dependency-resolution_kdfEWN/Project.csproj : error NU1202: Package System.Management.Automation 7.4.4 is not compatible with net6.0-windows7.0 (.NETCoreApp,Version=v6.0). Package System.Management.Automation 7.4.4 supports: net8.0 (.NETCoreApp,Version=v8.0)
  Failed to restore /tmp/package-dependency-resolution_kdfEWN/Project.csproj (in 721 ms).

Build FAILED.

/tmp/package-dependency-resolution_kdfEWN/Project.csproj : error NU1202: Package System.Management.Automation 7.4.4 is not compatible with net6.0-windows7.0 (.NETCoreApp,Version=v6.0). Package System.Management.Automation 7.4.4 supports: net8.0 (.NETCoreApp,Version=v8.0)
    0 Warning(s)
    1 Error(s)

…

updater | 2024/08/12 21:46:33 ERROR <job_868520955> Error processing System.Management.Automation (Dependabot::DependabotError)
2024/08/12 21:46:33 ERROR <job_8[685](https://github.com/ParticularLabs/FrameworkLimitedDependencySample/actions/runs/10359849593/job/28677091198#step:3:687)20955> FileUpdater failed
2024/08/12 21:46:33 ERROR <job_868520955> /home/dependabot/dependabot-updater/lib/dependabot/dependency_change_builder.rb:69:in `run'
…
updater | 2024/08/12 21:46:33 INFO <job_868520955> Finished job processing
updater | 2024/08/12 21:46:33 INFO Results:
Dependabot encountered '1' error(s) during execution, please check the logs for more details.
+----------------------------------------------+
|        Dependencies failed to update         |
+------------------------------+---------------+
| System.Management.Automation | unknown_error |
+------------------------------+---------------+

Native package manager behavior

If you try to update this dependency in Visual Studio, you will get the exact same error.

However…

If this were expected, Dependabot should identify the scenario and not go to unknown_error and fail the Dependabot run. But more importantly, without a Dependabot PR created (even if it is destined to fail in CI) developers such as myself will have no feedback to know that there even is a new version of the dependency. The PR should be raised anyway, which would at least give me the choice of whether it was time to update to net8.0, or whether to instruct Dependabot to ignore this minor version. Right now I'm not given that opportunity.

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

Dependabot is unable to raise any PRs due to the bug.

Smallest manifest that reproduces the issue

This repo is a minimal reproduction: https://github.com/ParticularLabs/FrameworkLimitedDependencySample

brettfo commented 4 weeks ago

@DavidBoike The issue here is that the current update checker for NuGet does some pretty simple version number checks and doesn't take into consideration the $(TargetFramework) of the relevant project. The good news is that we have code that does the proper update analysis. We're still working through some issues on it so we're slowly rolling it out over the coming weeks, but soon enough this issue should go away entirely.

I'll leave this issue open as a tracking item until then.

DavidBoike commented 4 weeks ago

@brettfo would it be possible to share any more details? We're trying to decide if our expectation is in line with what should happen. Our process reflects our current understanding that Dependabot should let us know that there's something potentially updateable. So when you say "but soon enough this issue should go away entirely" does that mean that this example repo in this state would raise a PR, or that it would just "swallow the error" due to the framework limitation?

From our spelunking, this change https://github.com/dependabot/dependabot-core/pull/8624 makes us think that our understanding may be incorrect. Because we would prefer to err on the side of having the PR (and the associated knowledge that an update is out there) than to be in the dark.

brettfo commented 4 weeks ago

The short version is that the NuGet updater should only try to update packages with compatible Target Framework values, but the current implementation isn't correctly handling this and is trying to update to a package with an incompatible Target Framework.

The more detailed version is this:

The overall behavior of dependabot is essentially this:

  1. updater reports all detected dependencies to dependabot
  2. dependabot asks what newer packages exist for each dependency
  3. dependabot asks the updater to update some of the dependencies to the newer versions detected in step 2

Right now when step 2 reports all newer packages, it doesn't do a complete "upgradability" check and it misses some cases where a Target Framework might be incompatible and that's why you're seeing a PR get generated that results in a build failure; because the new package doesn't support net6.0-windows that's listed in your project.

We have an alternate step 2 that we're experimenting with right now that re-wrote the package upgrade compatibility checker. Once this code is fully rolled out as the default, then we'll no longer report that an updated package exists because the Target Framework isn't compatible and the result of this particular dependabot run will be a no-op, because no package update can be performed that's compatible with the given Target Framework.

we would prefer to err on the side of having the PR (and the associated knowledge that an update is out there) than to be in the dark

Our goal is to only produce a PR that will pass your build and there's not really another mechanism to communicate any information out of dependabot, other than a PR, so if you're needing a tool to list all possible package updates, even if it violates <TargetFramework>, that won't be able to be dependabot.

DavidBoike commented 4 weeks ago

Interesting.

it misses some cases where a Target Framework might be incompatible and that's why you're seeing a PR get generated that results in a build failure

That's not actually what we're seeing. We're seeing no PR, but the "dependabot run" results in "failure". The reason we were running this down was to make sure this failure wasn't blocking other updates from being made.

Our goal is to only produce a PR that will pass your build

I guess I understand that design decision, but from our perspective that's the exact opposite of what we would want, and indeed perfecting the logic to hide information from us would make Dependabot less useful to us. We want the visibility that dependency updates exist.

We don't only want PRs that are "going to pass". Consider dependencies with breaking changes. Those aren't going to build, but we still want those. We certainly wouldn't want to be in the dark about new major versions with breaking changes. It appears where we don't agree is that we would consider framework-based breaks to be just another example of that.

Is it possible this behavior could become configurable at some point?

brettfo commented 4 weeks ago

We're seeing no PR, but the "dependabot run" results in "failure".

Thanks for the clarification, I missed that in your original post. Ideally this situation wouldn't result in any failure, I'll have to dig in a bit.

Is it possible this behavior could become configurable at some point?

I'm assuming the behavior you want is to always produce a PR, even if it violates a <TargetFramework> restriction? That's not a scenario that we'll support. One of the goals of dependabot as a whole (meaning, other ecosystems like NPM, Python, etc.) pulled from the README is "Check for the latest version of a dependency that's resolvable given a project's other dependencies" and we consider <TargetFramework> to be a dependency that we absolutely cannot change so if a package isn't compatible, we won't report it. As for it being a configurable option, that's also unlikely because that would be a NuGet-specific option and we try to only introduce a point of configuration if all (or at least most) package ecosystems could benefit from it and the concept of <TargetFramework> is very NuGet specific. The closest analog in other package ecosystems I can think of would be processor architecture or OS, or the compiler edition for Rust/Cargo and those are also treated as invariants.