MSBuildTreatWarningsAsErrors does not integrate with WarningsNotAsErrors well - project vs solution

[nkolev92]

Issue Description

Take the following project:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFrameworks>net472</TargetFrameworks>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
    <WarningsNotAsErrors>$(WarningsNotAsErrors);NU1603</WarningsNotAsErrors>
    <MSBuildTreatWarningsAsErrors>true</MSBuildTreatWarningsAsErrors>
    <!-- <MSBuildWarningsNotAsErrors>NU1603</MSBuildWarningsNotAsErrors> -->
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="NuGet.Common" Version="6.10.10" /> 
  </ItemGroup>

</Project>

The expectation would be that when restore is run on this project, NU1603 is raised, and because of TreatWarningsAsErrors and WarningsNotAsErrors, it'd remain a warning.

Now if you specify the project file, it errors, if you don't specify the project file, it warns.
After some investigation, it seems like it's a solution vs project thing.

I have validated that NuGet does the same thing in both cases. After all, NuGet does not depend on MSBuildTreatWarningsAsErrors.

Steps to Reproduce

I have a repro attached that demonstrates the behavior.
Extract the project and run the run.ps1 script.

Warnings-Update.zip

Expected Behavior

Running outside of the project folder
Running -t:restore src/Warnings.csproj
MSBuild version 17.13.0-preview-24504-04+c4d51a11b for .NET Framework
C:\Code\Warnings\src\Warnings.csproj : warning NU1603: Warnings depends on NuGet.Common (>= 6.10.10) but NuGet.Common 6.10.10 was not found. NuGet.Common 6.11.0 was resolved instead.
Running -t:restore src/
MSBuild version 17.13.0-preview-24504-04+c4d51a11b for .NET Framework
C:\Code\Warnings\src\Warnings.csproj : warning NU1603: Warnings depends on NuGet.Common (>= 6.10.10) but NuGet.Common 6.10.10 was not found. NuGet.Common 6.11.0 was resolved instead. [C:\Code\Warnings\src\War
nings.sln]
Running in the project folder
Running -t:restore Warnings.csproj
MSBuild version 17.13.0-preview-24504-04+c4d51a11b for .NET Framework
C:\Code\Warnings\src\Warnings.csproj : warning NU1603: Warnings depends on NuGet.Common (>= 6.10.10) but NuGet.Common 6.10.10 was not found. NuGet.Common 6.11.0 was resolved instead.
Running -t:restore
MSBuild version 17.13.0-preview-24504-04+c4d51a11b for .NET Framework
C:\Code\Warnings\src\Warnings.csproj : warning NU1603: Warnings depends on NuGet.Common (>= 6.10.10) but NuGet.Common 6.10.10 was not found. NuGet.Common 6.11.0 was resolved instead. [C:\Code\Warnings\src\War
nings.sln]

Actual Behavior

Running outside of the project folder
Running -t:restore src/Warnings.csproj
MSBuild version 17.13.0-preview-24504-04+c4d51a11b for .NET Framework
C:\Code\Warnings\src\Warnings.csproj : error NU1603: Warnings depends on NuGet.Common (>= 6.10.10) but NuGet.Common 6.10.10 was not found. NuGet.Common 6.11.0 was resolved instead.
Running -t:restore src/
MSBuild version 17.13.0-preview-24504-04+c4d51a11b for .NET Framework
C:\Code\Warnings\src\Warnings.csproj : warning NU1603: Warnings depends on NuGet.Common (>= 6.10.10) but NuGet.Common 6.10.10 was not found. NuGet.Common 6.11.0 was resolved instead. [C:\Code\Warnings\src\War
nings.sln]
Running in the project folder
Running -t:restore Warnings.csproj
MSBuild version 17.13.0-preview-24504-04+c4d51a11b for .NET Framework
C:\Code\Warnings\src\Warnings.csproj : error NU1603: Warnings depends on NuGet.Common (>= 6.10.10) but NuGet.Common 6.10.10 was not found. NuGet.Common 6.11.0 was resolved instead.
Running -t:restore
MSBuild version 17.13.0-preview-24504-04+c4d51a11b for .NET Framework
C:\Code\Warnings\src\Warnings.csproj : warning NU1603: Warnings depends on NuGet.Common (>= 6.10.10) but NuGet.Common 6.10.10 was not found. NuGet.Common 6.11.0 was resolved instead. [C:\Code\Warnings\src\War
nings.sln]

Analysis

🤷

Versions & Configurations

17.12, 17.8 seem to have the same way as 17.13.

We do expect an influx of WarningsNotAsErrors though, since that's part of the recommendation for audit warnings (the original way we noticed this issue).

[nkolev92]

In Visual Studio, it's remains a warning.

[JanKrivanek]

Internal communication related to this

[rainersigwald]

What I know so far:

I can repro on Windows with dotnet/templating@33a8ecb. The errors come when building with build.cmd and require multiproc MSBuild. In the worker node, a warning is logged and correctly not elevated to a warning. It is then sent to the scheduler node, which elevates it when trying to log it. The LoggingService in the main node knows that "all warnings should be errors" and doesn't have the but-not-these list.

I canNOT repro with

<Project>
  <PropertyGroup>
    <MSBuildTreatWarningsAsErrors>true</MSBuildTreatWarningsAsErrors>
    <MSBuildWarningsNotAsErrors>ABC123</MSBuildWarningsNotAsErrors>
  </PropertyGroup>

  <Target Name="Dispatch">
    <ItemGroup>
        <P Include="$(MSBuildThisFileFullPath)" AdditionalProperties="Num=1" />
        <P Include="$(MSBuildThisFileFullPath)" AdditionalProperties="Num=2" />
        <P Include="$(MSBuildThisFileFullPath)" AdditionalProperties="Num=3" />
        <P Include="$(MSBuildThisFileFullPath)" AdditionalProperties="Num=4" />
    </ItemGroup>
    <MSBuild Projects="@(P)" BuildInParallel="true" Targets="Warn" />

  </Target>

  <Target Name="Warn">
    <Warning Code="ABC123" Text="Hello from instance $(Num) in pid $([System.Diagnostics.Process]::GetCurrentProcess().Id)" />
    <Exec Command="sleep 1" /><!-- To give worker nodes some time to spin up -->
  </Target>
</Project>

Which seems like exactly the same thing. I want to debug into that now to see how it's different.

[rainersigwald]

In the success case in,

msbuild/src/Build/BackEnd/Components/Logging/LoggingService.cs

Lines 1994 to 2003 in 69b3e7a

	if (_warningsAsErrorsByProject != null && warningEvent.BuildEventContext != null && warningEvent.BuildEventContext.ProjectInstanceId != BuildEventContext.InvalidProjectInstanceId)
	{
	// Attempt to get the list of warnings to treat as errors for the current project
	WarningsConfigKey key = GetWarningsConfigKey(warningEvent);
	if (_warningsAsErrorsByProject.TryGetValue(key, out ISet<string> codesByProject))
	{
	// We create an empty set if all warnings should be treated as errors so that should be checked first.
	// If the set is not empty, check the specific code.
	ISet<string> codesToIgnoreByProject = null;
	_warningsNotAsErrorsByProject?.TryGetValue(key, out codesToIgnoreByProject);

_warningsAsErrorsByProject is populated and applies. In the failure case it exists but is empty.

[rainersigwald]

Aha! The templating build.cmd adds /warnaserror to the MSBuild command line. With that the simple repro project above does repro:

❯ dotnet msbuild -m .\foo.csproj /warnaserror

  foo succeeded with 1 warning(s) (0.0s)

    C:\Users\raines\Downloads\foo.csproj(19,5): warning ABC123: Hello from instance 1 in pid 39896

  foo succeeded (0.0s)

    C:\Users\raines\Downloads\foo.csproj(19,5): error ABC123: Hello from instance 4 in pid 3804

  foo succeeded (0.0s)

    C:\Users\raines\Downloads\foo.csproj(19,5): error ABC123: Hello from instance 3 in pid 5872

  foo succeeded (0.0s)

    C:\Users\raines\Downloads\foo.csproj(19,5): error ABC123: Hello from instance 2 in pid 8016


Build failed with 3 error(s) and 1 warning(s) in 0.2s

[rainersigwald]

Looks like c1d088e alllllmost fixed this but scoped it to when buildchecks are enabled. Removing that part of the condition, it works.

[rainersigwald]

For the single-proc case from the OP where projects (or Directory.Build.props) are multitargeted and say

    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
    <WarningsNotAsErrors>$(WarningsNotAsErrors);NU1603</WarningsNotAsErrors>

CLI args	Solution restore	Project restore
no -warnaserror	warning ✅	warning ✅
-warnaserror	error ❌ [a]	error ❌ [a]

In this case, MSBuild elevates the errors because it only pays attention to $(MSBuildWarningsNotAsErrors) when $(MSBuildTreatWarningsAsErrors) is set, and Common.targets doesn't promote TreatWarningsAsErrors to MSBuildTreatWarningsAsErrors (#10871). (It also hits cause [b] below but that's not the big problem in this scenario.)

If we add that:

    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
    <WarningsNotAsErrors>$(WarningsNotAsErrors);NU1603</WarningsNotAsErrors>
    <MSBuildTreatWarningsAsErrors>true</MSBuildTreatWarningsAsErrors>

CLI args	Solution restore	Project restore
no -warnaserror	warning ✅	error ❌ [b]
-warnaserror	error ❌ [c]	error ❌ [b]

[b] is caused by the multitargeting project not importing common.targets, and thus not importing the adopt-unprefixed-warning-tweaks-to-MSBuild-prefixed ones (#10873).

Working around that with

    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
    <WarningsNotAsErrors>$(WarningsNotAsErrors);NU1603</WarningsNotAsErrors>
    <MSBuildTreatWarningsAsErrors>true</MSBuildTreatWarningsAsErrors>
    <MSBuildWarningsNotAsErrors>$(WarningsNotAsErrors)</MSBuildWarningsNotAsErrors>

CLI args	Solution restore	Project restore
no -warnaserror	warning ✅	warning ✅
-warnaserror	error ❌ [c]	warning ✅

[c] is because MSBuild is configured overall to promote warnings, and does so for the warning raised by NuGet in the solution metaproject. That project doesn't import Directory.Build.props nor know the properties set in individual projects, so the MSBuild engine doesn't know to not promote the warning to error, and does so.

Dropping a Directory.Solution.props with

<Project>
  <PropertyGroup>
    <MSBuildTreatWarningsAsErrors>true</MSBuildTreatWarningsAsErrors>
    <MSBuildWarningsNotAsErrors>NU1603</MSBuildWarningsNotAsErrors>
  </PropertyGroup>
</Project>

CLI args	Solution restore	Project restore
no -warnaserror	warning ✅	warning ✅
-warnaserror	warning ✅	warning ✅

Hooray! But wait, what if things happen out of proc? Let's force that with MSBUILDNOINPROCNODE=1

CLI args	Solution restore	Project restore
no -warnaserror	warning ✅	warning ✅
-warnaserror	error ❌ [d]	error ❌ [d]

That fails [d] because of #10874 that I identified above.