Set WarnAsError to false by default for build.cmd script

[AArnott]

As requested in #31993 (comment)

[AlekseyTs]

This doesn't feel like the right thing to do. Regular compiler warnings should be treated as errors, only the formatting warnings shouldn't.

[AArnott]

@AlekseyTs dude, what did you mean by this comment then?

Therefore, I think this PR doesn't fix the problem with Build.cmd.

Which problem with build.cmd were you referring to?

When the CI accepts build warnings, and the IDE accepts build warnings, why should build.cmd default to disallowing them? Evidently (since the warnings were checked in in the first place) no quality gate blocks introducing these warnings. The getting started docs for this repo indicate a new user should run build.cmd to build the repo, and it has been broken for the last week because this is the only script that breaks on warnings.

If we want to prevent warnings from creeping in, the right place to block them is at least the CI/PR system so they are never allowed to be shared. If after that, build.cmd wants to help make that a measurable bar in a local build, that's fine.

[AlekseyTs]

By the comment I mean that warnings about formatting (they are not reported by the compiler and are added for Roslyn recently with intent that they will not be considered as errors) should not be treated as errors. That doesn't mean that all other warnings shouldn't be treated as errors.
CC @sharwell

[AArnott]

warnings about formatting (they are not reported by the compiler

Actually, I believe they are reported by the compiler, since the compiler is what runs the IDE* analyzers. And the compiler can be made to treat all warnings as errors, or treat specific warnings as errors. But AFAIK there is no way to categorically say "treat all formatting warnings" as warnings, and other warnings as errors. The only way to get close to that is with ruleset files and identifying each rule individually, which will always be a bit "behind" since new diagnostics codes are added all the time.

In my projects, I solve this by making all local builds emit warnings, and then my PR/CI treat warnings as errors. This allows an efficient dev inner-loop where formatting and other warnings are permissible while developing, but the PR build ensures that they've been cleaned up before being merged. I also (sometimes) set the "Release" configuration to treat warnings as errors so that developers can easily run a local build that will fail if the PR build would fail, allowing them to fix the issues more consistently before submitting the PR in the first place.

[AlekseyTs]

There is a way to specify that some specific warnings shouldn't be treated as errors. I could be wrong, but the set of formatting warnings is probably small and they all can be handled individually, at least for now.

[sharwell]

There is a way to specify that some specific warnings shouldn't be treated as errors

No, if you pass /warnaserror to MSBuild, it will take final precedence and cannot be bypassed for specific warnings. I removed that flag as part of the work to treat IDE0055 as a warning, but @tmat added it back during the switch to arcade as all arcade builds use that flag by default. The only way I know to turn it back off was ruled out as not allowed by arcade policy, so we have to wait for @tmat to restore the desired behavior in a manner supported by the shared infrastructure.

[tmat]

You can set $warnAsError to $false in build.ps1 to switch it off like this PR does. The same in build.sh.

See https://github.com/dotnet/roslyn/blob/master/eng/common/tools.ps1#L27

[tmat]

It would be better though to switch it off only for local builds, i.e. when $ci is not $true.

[tmat]

We should set this to false only when $ci is $false.
The same should be done in build.sh.

[sharwell]

You can set $warnAsError to $false in build.ps1 to switch it off like this PR does.

This would result in excluding both /warnaserror and /p:TreatWarningsAsErrors=true, which is incorrect. The former produces an incorrect result, but the latter is required for correctness.

[tmat]

You can pass /p:TreatWarningsAsErrors=true explicitly to the MSBuild function call if needed.

The former produces an incorrect result, but the latter is required for correctness.

Can you elaborate a bit?

[AArnott]

It would be better though to switch it off only for local builds, i.e. when $ci is not $true.

This script isn't used for CI builds anyway. If it were, the build warnings that crept in recently wouldn't have been allowed.

[sharwell]

Can you elaborate a bit?

• /warnaserror produces an incorrect result: We use <WarningsNotAsErrors>IDE0055</WarningsNotAsErrors> to prevent IDE0055 from being escalated from warning to error. /warnaserror overrides this setting, forcing errors to be reported in cases where the original design mandated that nothing more severe than warnings be reported.
• /p:TreatWarningsAsErrors=true is required for correctness: This is the mode used by our CI. Developers expect that running Build.cmd locally report errors for the same cases where the compiler in CI will report errors. Omitting it from Build.cmd will deviate from a long-standing operating consistency and increase the number of cases where PR build failures occur that should have been caught earlier.

[tmat]

@AArnott It is used for CI:
https://github.com/dotnet/roslyn/blob/master/eng/cibuild.cmd

[AArnott]

@tmat Then I'm missing something, because I don't know why #31993 was necessary. Why was my local build broken, and why wasn't your CI broken for a week?

[tmat]

@sharwell What's the intent? Is it to have IDE055 reported on command line (both locally and in the CI) as an error, but in IDE as a warning?

[sharwell]

@tmat IDE0055 should be reported in CI, command line, and IDE as a warning. All other warnings should be reported as warnings in the IDE, but errors in both CI and command line.

[tmat]

@sharwell Removing /warnaserror won't achieve that though - it will result in warnings reported by other tasks than C# (e.g. nuget) being reported as warnings, not as errors.

I think this should do what we want:

• keep $warnAsError=$true (that is /warnaserror /p:TreatWarningsAsErrors=true)
• keep including IDE0055 in WarningsNotAsErrors
• add /warnasmessage:IDE0055 to msbuild invocation

[sharwell]

@tmat IDE0055 is expected to be reported as a warning on the command line. This design is incompatible with /warnaserror. However, MSBuild provides other properties that allow specific warnings (by ID) from other sources to be reported as errors. These properties should be used to avoid the incorrect application of /warnaserror to the CSC task.

[tmat]

I see. I don't think such design is viable then with the current msbuild capabilities. Maintaining list of all possible warning IDs that non-C# tasks may report is not reasonable.

I think we need a new switch /warnnotaserror in msbuild that excludes warnings that /warnaserror applies to.
@rainersigwald

For now, as a workaround, we can set $warnAsError=$false and pass /p:TreatWarningsAsErrors=true explicitly here: https://github.com/dotnet/roslyn/blob/master/eng/build.ps1#L161

[rainersigwald]

The MSBuild /warnnotaserror request is tracked by dotnet/msbuild#3062.

[tmat]

Closing as the issue has been addressed by #32152.