Inconsistent BuildCheck warnings promotability to errors

JanKrivanek commented 1 month ago

Context

MSBuildTreatWarningsAsErrors and MSBuildWarningsAsErrors seem to apply only to BuildCheck diagnostics produced by the 'in-node' check (BC0201-BC0203). The behavior should be consistent and ideally apply to all Checks - built-in and custom.

Sample case

Case showing some diagnostics are promoted while some are not:

Repro

csproj:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
      <OutputType>Library</OutputType>
      <TargetFramework>net8.0</TargetFramework>
      <IsPackable>true</IsPackable>
      <NoWarn>NU1701</NoWarn>
          <DummyProp>xyz</DummyProp>
      <PackageVersion>$(CalculatorVersion)</PackageVersion>
      <PackageId>DotUtils.Calculator</PackageId>
      <MSBuildTreatWarningsAsErrors>true</MSBuildTreatWarningsAsErrors>
      <MSBuildWarningsAsErrors>BC0103</MSBuildWarningsAsErrors>
  </PropertyGroup>
</Project>

.editorconfig:

root = true

# Buildcheck rules
[*.csproj]
build_check.BC0103.severity=warning
build_check.BC0203.severity=warning

Run:

$env:CalculatorVersion=1.2.3
dotnet build /check

Expected: All Diagnostics are promoted to errors (as per MSBuildTreatWarningsAsErrors) Observed: BC0103 remains as warning

Notes

The investigation and fix should apply to custom checks as well
We should add testcases into our e2e tests - where we use MSBuildTreatWarningsAsErrors and verify it works properly. The e2e tests might be easier way of investigating the case for custom checks.

JanKrivanek commented 1 month ago

team triage: let's investigate the reason then decide

JanKrivanek commented 1 month ago

One cause is in skipping the check for events that don't have ProjectInstanceId set to invalid values:

https://github.com/dotnet/msbuild/blob/3b9f2e9569db38b34a36154f73e2aaef2f89c796/src/Build/BackEnd/Components/Logging/LoggingService.cs#L1967

But that is what happens for Evaluation time events (more details: https://github.com/dotnet/msbuild/blob/main/documentation/wiki/Binary-Log.md#meaning-of-various-ids-in-the-buildeventargs)

So if the warning is emited during eval - it will skip here

JanKrivanek commented 1 month ago

And more fundametely - it's one of the problems from the huge bucket of 'we need information from evaluation in order to process something earlier in the evaluation for BuildChecks'.

JanKrivanek commented 1 month ago

And it's even more intricate than that - the noWarn/WarnAsError/TreatWarnings as errors are being done during logging in the node that build the project, it's already not set in the main node. But majority of our Checks run in the main node.

We'll need to set it in the main node as well

JanKrivanek commented 1 month ago

Initial version of deferring: https://github.com/dotnet/msbuild/compare/main...proto/buildcheck-warns-promotability

The setting of warn to error etc. in the main node will need to be added as well

dotnet / msbuild