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

Solution level NuGetAuditLevel settings #13022

Closed
gmartinka opened this issue Nov 17, 2023 · 15 comments
Closed

Solution level NuGetAuditLevel settings #13022

gmartinka opened this issue Nov 17, 2023 · 15 comments

Comments

@gmartinka
Copy link

gmartinka commented Nov 17, 2023

NuGet Product(s) Involved

Visual Studio Package Management UI

The Elevator Pitch

When dealing with a solution with a lot of projects it is difficult to delay dealing with the NuGetAudit errors.

A solution level NuGetAuditLevel setting to adjust the entire solution would be appreciated.

Additional Context and Details

My solution uses TreatWarningsAsErrors and after updating VS to 17.8.0 I now have 26 failed projects in my solution due to nuget audit errors. My team has opted to work on these package updates on a separate timeline which means I need a way to proceed with my work without updating. Changing the audit level in 26 projects is annoying and I will want to change it back when I push my work so that the vulnerabilities stay visible.

@jeffkl
Copy link
Contributor

jeffkl commented Nov 17, 2023

@gmartinka can you try setting these properties in Directory.Build.props at the root of your repository to control them for all projects?

https://learn.microsoft.com/visualstudio/msbuild/customize-by-directory?view=vs-2022#directorybuildprops-and-directorybuildtargets

@jeffkl jeffkl added the WaitingForCustomer Applied when a NuGet triage person needs more info from the OP label Nov 17, 2023
@gmartinka
Copy link
Author

@gmartinka can you try setting these properties in Directory.Build.props at the root of your repository to control them for all projects?

https://learn.microsoft.com/visualstudio/msbuild/customize-by-directory?view=vs-2022#directorybuildprops-and-directorybuildtargets

That works! I did not know about this option. It is basically a default so anything, like TreatWarningsAsErrors, specified in the csproj is overwritten. I put <WarningsNotAsErrors>NU1901;NU1902;NU1903;NU1904</WarningsNotAsErrors> in my Directory.Build.props and it solved my problem, after restarting.

@ghost ghost added WaitingForClientTeam Customer replied, needs attention from client team. Do not apply this label manually. and removed WaitingForCustomer Applied when a NuGet triage person needs more info from the OP labels Nov 17, 2023
@gmartinka
Copy link
Author

I am going to mark this as closed since it solves my problem. A UI setting would be nice but probably not necessary given this option.

@ghost ghost removed the WaitingForClientTeam Customer replied, needs attention from client team. Do not apply this label manually. label Nov 17, 2023
@jeffkl
Copy link
Contributor

jeffkl commented Nov 17, 2023

That's great to hear, glad that helped. Be aware that some properties that are semicolon delimited like WarningsNotAsErrors can be additive so they don't get replaced:

<PropertyGroup>
  <NoWarn>$(NoWarn);123</NoWarn>
  <WarningsNotAsErrors>$(WarningsNotAsErrors);456</WarningsNotAsErrors>
</PropertyGroup>

Then in individual projects you can do the same thing and all values are used instead of being entirely overridden.

@zivkan
Copy link
Member

zivkan commented Nov 18, 2023

Also, there will be VS project properties coming for NuGetAudit settings, but a partner teams owns the project settings window

@Piedone
Copy link

Piedone commented Jun 13, 2024

@gmartinka did you do something like this in a Directory.Build.props file?

<Project>

  <PropertyGroup>
    <WarningsNotAsErrors>$(WarningsNotAsErrors);NU1901;NU1902;NU1903;NU1904</WarningsNotAsErrors>
  </PropertyGroup>
  
</Project>

Because this doesn't work for me in two separate solutions. It does work though if I add the same PropertyGroup to the csproj of the project depending on the vulnerable package.

@gmartinka
Copy link
Author

@gmartinka did you do something like this in a Directory.Build.props file?

<Project>

  <PropertyGroup>
    <WarningsNotAsErrors>$(WarningsNotAsErrors);NU1901;NU1902;NU1903;NU1904</WarningsNotAsErrors>
  </PropertyGroup>
  
</Project>

Because this doesn't work for me in two separate solutions. It does work though if I add the same PropertyGroup to the csproj of the project depending on the vulnerable package.

Thit is my Directory.Build.props, and that file lives in the root directory where all my source is stored so it works on all the slns in that directory.

<Project> <PropertyGroup> <WarningsNotAsErrors>NU1901;NU1902;NU1903;NU1904</WarningsNotAsErrors> </PropertyGroup> </Project>

@Piedone
Copy link

Piedone commented Jun 13, 2024

Thanks, yeah, that's virtually the same. I also tried exactly that, i.e. without $(WarningsNotAsErrors);. Interestingly, it doesn't work for me, running this still shows the warning:

dotnet build -c Release -warnaserror /p:TreatWarningsAsErrors=true /p:RunAnalyzers=true --no-incremental

@zivkan
Copy link
Member

zivkan commented Jun 20, 2024

I created a sample project with a Directory.Build.props that sets WarningsNotAsErrors: gh13022.zip

I can't reproduce the behaviour you're reporting. dotnet restore reports the NU1903 as a warning, not an error.

@Piedone
Copy link

Piedone commented Jun 20, 2024

Thank you!

Note that I've run dotnet build, not dotnet restore. I've now checked this (with a non-built, cleaned solution):

dotnet restore
dotnet build -c Release -warnaserror /p:TreatWarningsAsErrors=true /p:RunAnalyzers=true --no-restore

With your sample, this indeed only produces a warning (one with each command).

When just running the build alone (or running it with --no-incremental), it still produces an error:

dotnet build -c Release -warnaserror /p:TreatWarningsAsErrors=true /p:RunAnalyzers=true --no-incremental

This is something I can live with, though I'd expect the config to be consistent.

And I still get audit advisory as error on build for Orchard Core's solution somehow, despite following the same pattern. So, this is some issue with that solution that I can't find (I don't see any config reverting WarningsNotAsErrors, I also removed all other instances of it).

Oh well. I addressed our CI builds failing on new warnings with /p:NuGetAudit=false under OrchardCMS/OrchardCore#16317 (we still see the warnings locally, the audit still runs for a NuGet release, and packages are updated weekly by Dependabot, so this is not a big issue).

@zivkan
Copy link
Member

zivkan commented Jun 21, 2024

When just running the build alone (or running it with --no-incremental), it still produces an error

Ok, running locally, it appears that -wanaserror is telling MSBuild to treat warnings as errors, and MSBuild isn't respecting WarningsNotAsErrors.

According to msbuild's CLI docs, there's a -warnNotAsError[:code] argument, so if you really want to use -warnaserror, then you should also use -warnnotaserror:NU1901,NU1902,NU1903,NU1904. However, using -p:TreatWarningsAsErrors=true has the same functional effect, at least for restore, except NuGet will implement the logic, rather than MSBuild.

The reason I'm so confident that it's MSBuild interpreting the warnings as errors is because years ago I made NuGet output "Warning As Error:" for all warnings that are elevated to errors by NuGet's own logic. When I run dotnet restore -warnaserror, NU1903 is shown as an error, but without that "Warning As Error" message. Hence, I believe MSBuild is promoting the error to a warning, despite NuGet calling MSBuild's logging API telling it to output a warning.

@Piedone
Copy link

Piedone commented Jun 21, 2024

I also tried -warnnotaserror before (specifically -warnNotAsError:NU1902) and it didn't have an effect, NU1902 still showed up as an error.

Do I understand correctly that you think this is an MSBuild bug, then?

@zivkan
Copy link
Member

zivkan commented Jun 21, 2024

It Works For Me:
image

But it does appear to be an MSBuild specific thing. I wouldn't be surprised if it's "not a bug" though. There's a reasonable chance that these -WarnAsError and -WarnNotAsError flags were added before anyone started using <TreatWarningsAsErrors>, <WarningsAsErrors> and <WarningsNotAsErrors> properties. In other words, I think those CLI flags probably never read project properties, it's probably a different implemenetation of the same concept, just with slightly different semantics. But that's something you can discuss with the MSBuild team over at https://github.com/dotnet/msbuild.

@Piedone
Copy link

Piedone commented Jun 23, 2024

OK, thank you!

@milesbuckton
Copy link

milesbuckton commented Oct 10, 2024

This worked for me:

<Project>
  <PropertyGroup>
    <NuGetAudit>false</NuGetAudit>
  </PropertyGroup>
</Project>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants