search

aspnet / KoreBuild

OBSOLETE REPO - see readme
Other
37 stars 23 forks source link

Enable CSC and MSBuild warning as errors #170

Closed mikeharder closed 6 years ago

mikeharder commented 7 years ago

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:

https://github.com/Microsoft/msbuild/pull/1366

It appears that CSC warnings are currently not be treated as errors, at least for the Entropy\samples projects (https://github.com/aspnet/Entropy/issues/238). Also ensure this MSBuild/NuGet warning from Entropy would be an error: https://github.com/aspnet/Entropy/issues/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 commented 7 years ago

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

mikeharder commented 7 years ago

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 commented 7 years ago

We already default to CSC warnings as errors.

natemcmaster commented 7 years ago

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 https://github.com/NuGet/Home/issues/4587, but otherwise I think this is something we can do soon.

Eilon commented 7 years ago

FYI: This is not for preview1.

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

dougbu commented 7 years ago

@mikeharder why did you change the assignment here?

mikeharder commented 7 years ago

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

dougbu commented 7 years ago

@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 commented 7 years ago

It's yours.

dougbu commented 7 years ago

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:

natemcmaster commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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. https://github.com/aspnet/BuildTools/pull/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 commented 7 years ago

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

Eilon commented 7 years ago

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