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

use MSBuild binlog to report dependencies #10597

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

brettfo
Copy link
Contributor

@brettfo brettfo commented Sep 12, 2024

This PR contains a temporary commit to redirect the smoke tests. That commit will have to be removed prior to merging.

This PR has a corresponding smoke test PR that will hav to be merged at the same time: dependabot/smoke-tests#227

Previously, a temporary project was created to try and determine a project's full set of dependencies. This wasn't always 100% accurate and could be slow due to all of the file copying.

This PR instead invokes MSBuild directly against the relevant project and produces a binary log (.binlog) that is then analyzed to get the full set of dependencies.

One of the benefits to this approach is that any give package can be directly associated with its parent project and MSBuild handles all of the complex property evaluation that might occur.

There are two main differences in behavior:

  1. Properties are now fully evaluated; we no longer return $(SomePackageVersion); we know exactly what was resolved.
  2. We no longer report Directory.Packages.props as the source of a dependency. While the dependency version might be found there, the dependency really lies with the project file. This has no effect on actually performing the update; that occurs just like before.

Another relatively minor side effect is that we no longer report NETStandard.Library as a dependency; it's explicitly excluded. While it is a NuGet package that gets resolved, it's not one that can be directly updated so it is simply no longer reported.

The vast majority of the changes in this PR are to tests.

The primary file to review is SdkProjectDiscovery.cs; that's where the dependency analysis was completely migrated to the binary log.

@brettfo brettfo added the L: dotnet:nuget NuGet packages via nuget or dotnet label Sep 12, 2024
@brettfo brettfo force-pushed the dev/brettfo/nuget-report-dependencies branch 2 times, most recently from 1edef09 to eaa3f30 Compare September 16, 2024 18:22
@brettfo brettfo force-pushed the dev/brettfo/nuget-report-dependencies branch 4 times, most recently from cccbcf2 to 60c6971 Compare September 17, 2024 23:56
Copy link
Contributor

@martincostello martincostello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With MSBuild evaluation, will this improve (or fix) the way dependabot handles NuGet updates for projects that multi-target and/or prevent transient packages being updated unnecessarily?

@brettfo
Copy link
Contributor Author

brettfo commented Oct 17, 2024

With MSBuild evaluation, will this improve (or fix) the way dependabot handles NuGet updates for projects that multi-target and/or prevent transient packages being updated unnecessarily?

The handling is certainly better. When detecting dependencies we can now handle all Condition attributes no matter where they occur, but this PR is purely focused on the detection; there's still the issue of us being asked to update some package and making the change in the non-ideal or even wrong spot. I'm discussing some features with the MSBuild team that could potentially allow more accurate location information for when we try to do the actual update, but that's future work.

@brettfo brettfo force-pushed the dev/brettfo/nuget-report-dependencies branch from 60c6971 to 2ed2f0f Compare November 8, 2024 23:50
@brettfo brettfo marked this pull request as ready for review November 9, 2024 00:12
@brettfo brettfo requested review from a team as code owners November 9, 2024 00:12
@brettfo
Copy link
Contributor Author

brettfo commented Nov 9, 2024

I've added tests explicitly checking:

  1. Dependencies from repos using Central Package Versioning can be found. I don't think we do the proper update yet (that'll be a follow-up PR), but we detect the proper package and the relevant Packages.props.
  2. Central Package Management when the $(DirectoryPackagesPropsPath) variable is overridden. Same as above, we detect the package versions and the imported files properly, but we may not yet be able to do the update, but that's a future PR.

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.

3 participants