Enable CSC and MSBuild warning as errors

[mikeharder]

We should ensure that warnings-as-errors is enabled by default for both MSBuild and CSC for src, test, and samples in all repos. If needed, this can be disabled for individual repos/projects/warnings.

This change should allow us to enable warnings-as-errors by default for MSBuild:

dotnet/msbuild#1366

It appears that CSC warnings are currently not be treated as errors, at least for the Entropy\samples projects (aspnet/Entropy#238). Also ensure this MSBuild/NuGet warning from Entropy would be an error: aspnet/Entropy#239.

The Internal.AspNetCore.Sdk sets WarningsAsErrors:

https://github.com/aspnet/BuildTools/blob/dev/src/Internal.AspNetCore.Sdk/targets/Common.props#L27

However, samples don't reference this SDK, so we'll need to find some other way to enable warnings-as-errors for samples.

[natemcmaster]

To be clear, you're talking about MSBuild warnings, not CSC warnings, right?

[mikeharder]

I think both should be treated as errors by default, with exceptions as needed. My understanding is that our build is already configured to treat CSC warnings as errors, but we should verify. If true, then the only remaining work is to treat MSBuild warnings as errors.

[natemcmaster]

We already default to CSC warnings as errors.

[natemcmaster]

We're pretty close to having eliminated all build warnings. We still have a few assembly conflicts, and we still have an issue with NuGet NuGet/Home#4587, but otherwise I think this is something we can do soon.

[Eilon]

FYI: This is not for preview1.

We want to make sure that warnings-as-errors is enabled for both MSBuild and CSC.

[dougbu]

@mikeharder why did you change the assignment here?

[mikeharder]

I thought @ryanbrandenburg could take this since he's working on other build-related stuff. Do you want to keep it?

[dougbu]

@mikeharder yes, this work naturally follows on after my work on API Check (aspnet/BuildTools#146). Both issues involve lots of local Universe builds 😺 Plus, I'm not fighting fires as @ryanbrandenburg is right now.

Any objection to me taking this back @ryanbrandenburg or @mikeharder?

[mikeharder]

It's yours.

[dougbu]

CSC warnings are currently limited to the [Obsolete] warnings discussed in aspnet/Entropy#238 and the new xUnit1004 noise. We should be able to centralize disabling xUnit1004.

MSBuild warnings fall into two categories:

• 60 (❗️❗️) missing Api Check baseline files. It's relatively simple to disable the warnings or generate the baselines. But, must research whether the packages have been released on NuGet to know which is the correct choice for a project. The repo owners should own this process.
• Various NuGet.targets or Microsoft.Common.CurrentVersion.targets warnings related to mixed dependency versions. I believe @ryanbrandenburg is already working on some of these. I'll take another look next time our build passes.

[natemcmaster]

FYI for context on xUnit1004 - this warning happens when tests are permanently skipped, i.e. [Fact(Skip = "Flaky")]. xUnit1004 is a symptom of flaky tests or dead code. Both conditions warrant action: fix it or remove it.

[dougbu]

@natemcmaster sure, action is necessary but these particular warnings should not fail our CI builds. I'm saying these particular warnings should not included in a broad "all warnings should be treated as errors" mandate, not that they should be hidden or ignored.

In any case, we should have issues for all disabled tests already -- ready for triage decisions et cetera. Our teams have a clear preference for disabling tests temporarily over removing code (that'll come back soon-ish).

[natemcmaster]

these particular warnings should not fail our CI builds

One more piece of context...before upgraded to xunit.analyzers, we added this particular warning to CSC's WarningsNotAsErrors list to avoid just this problem. aspnet/BuildTools#254.

On a separate note, MSBuild's flag /warnaserrors is good, but not perfect at preventing builds from passing that shouldn't. /warnaserrors allows the build to complete as if the flag is not set, but just turns warnings to errors at the end. Because CSC is incremental and analyzers only run when CSC is actually executed, this means you can easily hit this:

dotnet clean
dotnet build /warnaserrors     # fails because xUnit1004 was upgraded to an error, yet compilation was allowed to finish
dotnet build /warnaserrors     # succeeds. xUnit1004 was not produced because CSC did not recompile

[mikeharder]

@natemcmaster: Good to know for dev scenarios, but I don't think this will impact the CI builds since they only build once.

[Eilon]

@mikeharder please check if there's any work left.