Skip to content

Contribute changes to NuGet updater from Azure team#8179

Merged
deivid-rodriguez merged 86 commits intodependabot:mainfrom
brettfo:dev/brettfo/nuget-clean
Nov 27, 2023
Merged

Contribute changes to NuGet updater from Azure team#8179
deivid-rodriguez merged 86 commits intodependabot:mainfrom
brettfo:dev/brettfo/nuget-clean

Conversation

@brettfo
Copy link
Copy Markdown
Contributor

@brettfo brettfo commented Oct 11, 2023

Contribute NuGet changes back to the community

Internally at Microsoft we've been making some updates to the NuGet dependabot updater and are now in a position to contribute those changes back to the community. We're excited to be able to do this and hope that it will help improve the experience for everyone.

This work was announced by Bryan Sullivan at Microsoft Build 2023 in May of this year.

Summary of improvements

  • Rewrite core updater logic in C#. External contributors that want to make further improvements will likely be more comfortable working in a .NET environment with Visual Studio. Some Ruby code still exists to take advantage of the functionality offered in the base updaters.
  • Use code from the NuGet project (via a submodule) to handle updates for packages.config scenarios. This ensures that we're using as close to the same logic as possible as the NuGet CLI to update the references.
  • Use MSBuild to compute the complete list of .props and .targets files that are imported, regardless of any complicated path computations. This ensures that for each project file being analyzed, we can better determine the exact file that declares the package version. This can be particularly helpful in large monorepos.
  • Propertly handle updates to implicit dependencies. This is particularly useful for SDK-style projects that may not explicitly list all of their dependencies.
  • Propertly handle updates to peer dependencies. This will help reduce build failues in generated pull requests by ensuring that all applicable packages are updated together.
  • Update binding redirect assembly versions in app.config/web.config files. Without these changes the user's app will likely fail to load the assemblies at runtime, making these errors hard to detect.

Notes

Part of the NuGet updater work (i.e., supporting packages.config) required code from the NuGet/NuGet.Client repo. The specific code required only exists as a Windows console app targeting net472 (see nuget.org/downloads) and the NuGet team does not want contributions back to allow this to build on Linux/net7.0 (I asked) so to avoid copying the entire repo, a submodule was added tied to the latest public release. To help make local development easier, the NuGet build script checks for a sentinel file in that submodule and if it's not present, will report the command the developer can use to fix the issue.

Related PRs

There were a number of other internal improvements that we made that have been split into separate PRs:

Caveats

There will be a few more changes from our side that I'll add to this PR as appropriate, but wanted to get the ball rolling on review and feedback.

I'll separately be working on cleaning up the git history to remove duplicate commits. When that work is done I'll force-push to this branch to help ensure a clean merge when it's all complete.

As of internal commit 15e3b8d09374468e40d8d77bafc1c01178db444c

Closed issues

@github-actions github-actions bot added L: docker Docker containers L: dotnet:nuget NuGet packages via nuget or dotnet L: javascript L: python labels Oct 11, 2023

Check failure

Code scanning / CodeQL

Server-side request forgery

The URL of this request depends on a [user-provided value](1).
@brettfo brettfo force-pushed the dev/brettfo/nuget-clean branch from 4d543e5 to 3487492 Compare October 11, 2023 23:44
@brettfo brettfo force-pushed the dev/brettfo/nuget-clean branch from 3487492 to 0a2ca5e Compare October 12, 2023 17:34
@brettfo brettfo force-pushed the dev/brettfo/nuget-clean branch 5 times, most recently from 0b320c8 to 3a74f2b Compare October 12, 2023 23:23
Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

I made a quick pass commenting on some stuff that seems general and not necessarily related to NuGet.

It'd be nice to discuss & merge those changes independently through separate PRs.

deivid-rodriguez and others added 9 commits November 27, 2023 12:20
Native helpers path is for native helpers.
This is more consistent with other ecosystems.
And not only build them.

This is consistent with other ecosystems and simplifies dev loop when
working with native helpers inside the dev image, since you only have to
run `nuget/helpers/build` after changing them.
It's generally a bad practice and Ruby warns it:

```
 => bump aspnetcore.healthchecks.rabbitmq and microsoft.extensions.diagnostics.healthchecks in /dependabotrepro
system temporary path is world-writable: /tmp
/tmp is world-writable: /tmp
system temporary path is world-writable: /tmp
/tmp is world-writable: /tmp

    ± DependabotRepro/Directory.Packages.props
    ~~~
    --- /home/dependabot/original20231117-11-ghq2ml	2023-11-17 13:19:01.669901000 +0000
    +++ /home/dependabot/updated20231117-11-bwqzij	2023-11-17 13:19:01.669901000 +0000
    @@ -4,7 +4,7 @@
         <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
       </PropertyGroup>
       <ItemGroup>
    -    <PackageVersion Include="AspNetCore.HealthChecks.Rabbitmq" Version="5.0.2" />
    -    <PackageVersion Include="Microsoft.Extensions.Diagnostics.HealthChecks" Version="5.0.17" />
    +    <PackageVersion Include="AspNetCore.HealthChecks.Rabbitmq" Version="7.0.0" />
    +    <PackageVersion Include="Microsoft.Extensions.Diagnostics.HealthChecks" Version="7.0.9" />
       </ItemGroup>
     </Project>
    \ No newline at end of file
    ~~~
    3 insertions (+), 3 deletions (-)
```

I fixed it by telling nuget to use a different path where we have
permissions.
Also fix a typo in the MSBuild targets.
Only allow empty `Condition` attributes _or_ `Condition` attributes that are comparing to an emtpy string.
@deivid-rodriguez deivid-rodriguez merged commit 9ed0101 into dependabot:main Nov 27, 2023
@brettfo brettfo deleted the dev/brettfo/nuget-clean branch January 3, 2024 23:45
@2PintChristianN
Copy link
Copy Markdown

So this needs to be reverted or fixed, preferrably also with a unit test that makes sure wildcard version strings isn't broken again
#9442 (comment)

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.

Leaf level dependencies should be updated first NuGet: package update is offered that is incompatible with the target framework