search

dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.23k stars 1.35k forks source link

MSBuildTreatWarningsAsErrors does not integrate with WarningsNotAsErrors well - project vs solution #10801

Open nkolev92 opened 1 month ago

nkolev92 commented 1 month ago

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 commented 1 month ago

In Visual Studio, it's remains a warning.

JanKrivanek commented 1 month ago

Internal communication related to this

rainersigwald commented 3 weeks ago

What I know so far:

I can repro on Windows with https://github.com/dotnet/templating/commit/33a8ecba4a717c81b261ab0aeb1a34164c730d4a. 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 commented 3 weeks ago

In the success case in,

https://github.com/dotnet/msbuild/blob/69b3e7a43fb2fb26812b20807333cf6ca62167f2/src/Build/BackEnd/Components/Logging/LoggingService.cs#L1994-L2003

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

rainersigwald commented 3 weeks ago

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 commented 3 weeks ago

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

rainersigwald commented 3 weeks ago

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 (https://github.com/dotnet/msbuild/issues/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 (https://github.com/dotnet/msbuild/issues/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 https://github.com/dotnet/msbuild/issues/10874 that I identified above.