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

explicitly override TreatWarningsAsErrors to false during discovery #11786

Merged
merged 2 commits into from
Mar 12, 2025

Conversation

brettfo
Copy link
Contributor

@brettfo brettfo commented Mar 11, 2025

If all of the following are true:

  • The repo doesn't have a NuGet.Config file (or it does but doesn't specify the <clear /> directive)
  • The repo is using Central Package Management (i.e., Directory.Packages.props)
  • The project sets $(TreatWarningsAsErrors) to true
  • The dependabot.yml file lists custom NuGet feeds

...then dependency discovery can fail because the updater generates a NuGet.Config file before the updater is run that contains all of the feeds listed in dependabot.yml as well as api.nuget.org and if there are multiple remote feeds used with CPM, a warning NU1507 is generated indicating that no packageSourceMapping elements were specified in NuGet.Config. $(TreatWarningsAsErrors) then kicks in and prevents dependency discovery from happening.

Dependency discovery needs to succeed so we inject the command line argument /p:TreatWarningsAsErrors=false to override any property that might be set in the project file or imported .props or .targets so the NU1507 warning doesn't cause a failure. Note, properties specified on the command line always override even explicitly set properties in the project or imported files.

Side note on why api.nuget.org is added to the generated NuGet.Config that leads to this issue:

The default feed of api.nuget.org is explicitly added because if the repo doesn't contain a NuGet.Config file (or if it does and it doesn't specify the <clear /> directive) then they're already opting in to the default feed behavior which means api.nuget.org. It's possible that a developer's local machine manually removed api.nuget.org from the user-level file, but there is no way dependabot can know about that scenario and having the fallback to api.nuget.org is the default.

@github-actions github-actions bot added the L: dotnet:nuget NuGet packages via nuget or dotnet label Mar 11, 2025
@brettfo brettfo force-pushed the dev/brettfo/nuget-multi-sources-with-cpm branch from 03e6712 to 2c81a6a Compare March 11, 2025 16:35
@@ -69,6 +75,105 @@ public static TestHttpServer CreateTestStringServer(Func<string, (int, string)>
return CreateTestServer(bytesRequestHandler);
}

public static TestHttpServer CreateTestNuGetFeed(params MockNuGetPackage[] packages)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We commonly need to create test-only HTTP NuGet feeds. In the case where the feed is expected to be well-behaved, this helper makes it a one-line addition to a test.

@brettfo brettfo force-pushed the dev/brettfo/nuget-multi-sources-with-cpm branch from 2c81a6a to 69c33de Compare March 11, 2025 16:40
@brettfo brettfo marked this pull request as ready for review March 11, 2025 21:10
@brettfo brettfo requested a review from a team as a code owner March 11, 2025 21:10
@honeyankit honeyankit force-pushed the dev/brettfo/nuget-multi-sources-with-cpm branch from 6f5218a to a5dbec2 Compare March 11, 2025 22:18
@sachin-sandhu sachin-sandhu force-pushed the dev/brettfo/nuget-multi-sources-with-cpm branch from a5dbec2 to 321f6bc Compare March 12, 2025 12:45
@randhircs randhircs force-pushed the dev/brettfo/nuget-multi-sources-with-cpm branch from 321f6bc to cf208e3 Compare March 12, 2025 16:34
@randhircs randhircs merged commit 1912c93 into main Mar 12, 2025
77 checks passed
@randhircs randhircs deleted the dev/brettfo/nuget-multi-sources-with-cpm branch March 12, 2025 17:32
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.

5 participants