Support warnaserror

[sbomer]

We should have some way to treat all warnings or specific warnings as errors. What exists today:

• SDK properties without special behavior: (TreatWarningsAsErrors bool, WarningsAsErrors list)
  These are explicitly passed as task arguments to tasks that support them.
  They are the properties that are set by the VS UI.
• MSBuild properties that automatically promote warnings to errors (MSBuildTreatWarningsAsErrors bool, MSBuildWarningsAsErrors list)
  Support NoWarn as MSBuildWarningsAsMessages msbuild#4421 is about inferring the MSBuild properties from the above.
  The MSBuild flag /warnaserror behaves similarly to setting MSBuildTreatWarningsAsErrors.

It would be nice to rely on the MSBuild properties instead of re-implementing support for --warnaserror - however, we will probably need a workaround for dotnet/msbuild#5511 if we do that. If there's no easy workaround, we probably need to support --warnaserror.

If we also want to support WarningsNotAsErrors, we will need to either support this via --warnaserror-, or wait for MSBuildWarningsNotAsErrors (dotnet/msbuild#3062).

[marek-safar]

Isn't the work mostly on SDK side for this task?

[vitek-karas]

Based on some offline discussions we're leaning towards not relying on MSBuild for this functionality. That means we need the linker to promote warnings to errors (the task or the SDK can't really do that AFAIK). Which leads to adding the --warnaserror command line option.

@sbomer - could you please add a short "spec" of how the --warnaserror should work? (I assume we will copy the behavior from Roslyn for the most part).

[sbomer]

Here's how it would work if we do what csc does (https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-options/warnaserror-compiler-option), with minor syntax changes:

syntax:                          --warnaserror[+|-] [warning-list]
all warnings as errors:          --warnaserror
                                 --warnaserror+
all warnings not as errors::     --warnaserror-
specific warnings as errors:     --warnaserror IL2006,IL2007
                                 --warnaserror+ IL2006,IL2007
specific warnings not as errors  --warnaserror- IL2006,IL2007

Options are processed in order, last wins - so you can for example treat all warnings except IL2006 as errors with:

--warnaserror --warnaserror- IL2006

(this is what TreatWarningsAsErrors: true, WarningsNotAsErrors: IL2006 will do.)

Like --nowarn, non-linker codes should be ignored, and we should support the same set of separators for consistency. Csc also supports -warnaserror nullable - so in the future, we could extend this (and maybe --nowarn) to support categories.

[agocke]

Yup, this looks like how I remember it. This seems like a good strategy to me.

The one thing to keep in mind: many tools expect that tools that report errors do not output files. In fact, that's often the definition of what an error means. However, exiting at the first sign of an error is usually a problem as it results in a very frustrating cycle of build, see an error, fix an error, rebuild, see a new error, etc.

[agocke]

(Also, nullable is a bit of a special case, I would ignore it for the purposes of diagnostics for the moment. It's probably more complicated than necessary).

[sbomer]

tools that report errors do not output files [....] exiting at the first sign of an error is usually a problem

Great points. Currently, we have both fatal errors that stop the linker, and recoverable ones that don't prevent the linker from producing outputs. The SDK handles this by touching an extra file (which we need anyway for incremental builds) to indicate that the linker completed successfully and without errors - which unfortunately doesn't work with MSBuildWarningsAsErrors (dotnet/msbuild#5511).

If we support --warnaserror, I believe this will just work with the SDK logic, even if we continue to output files when we produce errors.

[agocke]

If we support --warnaserror, I believe this will just work with the SDK logic, even if we continue to output files when we produce errors.

If the user is ignoring the error code from MSBuild, and just relying on the file being present to indicate a passing build, would this system continue to work? Effectively, would the absence of the extra file prevent the copying of the linker files to the output directory? (and keep them in the obj directory)

[sbomer]

If the user is ignoring the error code from MSBuild, and just relying on the file being present to indicate a passing build, would this system continue to work?

If I understand the question, yes - as long as they're looking at the publish directory and not obj. I don't know if that's reliable for incremental builds though. IIRC, publish used to never clean up old files in the publish directory, but I think there was some work done to make it more incremental.

Effectively, would the absence of the extra file prevent the copying of the linker files to the output directory? (and keep them in the obj directory)

Yup, effectively - it's more that the exit value of the task determines whether the extra SDK logic (touching that file + copying linker outputs from obj to the publish directory) runs - so we need to ensure that the task says it failed, if we logged any errors.

[agocke]

Cool. Then it sounds like we're covered, since the only supported way to use the linker is through the SDK, and the SDK prevents accidentally taking a dependency on an output from a failed task.

[vitek-karas]

I assume what's left is:

• Wire this through ILLink task (mono/linker repo) - let's use this issue to finish that work
• Wire this to the right MSBuild properties - @sbomer do we have a dotnet/sdk issue already, if not, can you please create one?

[sbomer]

Yup, the SDK issue is dotnet/sdk#12601.

[sbomer]

Closing this since it is fixed by #1388. The only remaining work is in the SDK.