Implement /WarnAsError to treat specified warnings as errors by jeffkl · Pull Request #1355 · dotnet/msbuild

jeffkl · 2016-11-17T18:28:34Z

Specify just /WarnAsError to have all warnings logged as errors as well as have the build fail.
Specify a comma delimited list of warning codes to have just that set of warnings treated as errors as well as have the build fail.

Targets will still show as succeeded and the tasks will continue to execute but the overall build result will be a failure.

Related to #68 and will close in my next change to add /NoWarn.

Help text should be reviewed as well:

  /warnaserror[:code[;code2]]
                     List of warning codes to treats as errors.  Use a semicolon
                     or a comma to separate multiple warning codes. To treat all
                     warnings as errors use the switch with no values.
                     (Short form: /err[:c;[c2]])
                     Example:
                       /warnaserror:MSB4130

                     When a warning is treated as an error the target will
                     continue to execute as if it was a warning but the overall
                     build result will fail.

rainersigwald

Biggest concern is the additive-vs-override nature of multiple arguments.

rainersigwald · 2016-11-18T00:57:19Z

src/XMakeBuildEngine/BackEnd/BuildManager/BuildManager.cs

typo: no space in intact

rainersigwald · 2016-11-18T00:59:00Z

src/XMakeBuildEngine/BackEnd/BuildManager/BuildManager.cs

Is there a named constant somewhere for the magic -1?

I did a quick search and didn't find anything. I saw this line and figured it was okay to use -1. Should I declare a constant like NullSubmissionId ?

rainersigwald · 2016-11-18T01:02:12Z

src/XMakeBuildEngine/BackEnd/BuildManager/BuildManager.cs

Is there a good way to abstract this logic? It's pretty repetitive in here. Maybe EnsureFailureIfWarningsAsErrors(BuildResult, ILoggingService)?

Good idea, I can make a method that does this for two of the places.

rainersigwald · 2016-11-18T01:03:21Z

src/XMakeBuildEngine/BackEnd/BuildManager/BuildParameters.cs

It's probably not worth changing this, but I would hazard a guess that for this use case (small sets of inputs), a SortedSet might actually be faster.

I did some limited research on what was faster and from what I could tell there was no difference. Do you know of any guidance on which is faster? I wanted the fastest possible lookup for sure.

AFAIK it's highly dependent on the individual use case, so we probably can't tell for this case without extensive benchmarking.

My understanding is based on Pike's third rule: Fancy algorithms are slow when n is small, and n is usually small. In this case, a hashtable is awesomely super fast, linear lookup . . . but requires hashing and indirection/pointer-following. A sorted set that is small, as I expect this thing to be, could easily use fewer instructions per lookup.

But we'd have to measure to know (Rule 2!) and I don't think this will be worth worrying about, so I'd say to leave it as-is.

I just did some benchmarks. If you have 10 warnings to set as errors and your build logs 10,000 warnings, the fastest is to use HashSet<T>(StringComparer.OrdinalIgnoreCase). I tried doing a case sensitive search but things placed in the set are upper case and the item being checked for is uppercased before the check but that was slower. The SortedSet<T> was just a little bit slower.

In my scenario, it took 1.04 milliseconds to check 10,000 times for the 10 items. I think the perf here is good enough but at least know we know. :emoji-for-the-'the-more-you-know'-public-service-annoncement:

Alternatively, declare this as an ImmutableHashset and just copy the reference rather than cloning a new one. Theoretically they are slower, with logn lookup. Can you benchmark this one as well? :)

In reply to: 88770185 [](ancestors = 88770185)

rainersigwald · 2016-11-18T01:04:18Z

src/XMakeBuildEngine/BackEnd/BuildManager/BuildParameters.cs

Hmm. The double-use here seems a bit mysterious, in a way it doesn't on the command line. But it's not wrong . . .

I was trying to avoid having another property like bool HasUserSpecifiedWarningsAsErrors so I am using the three states of a reference type; null, not null; or has a value. The usages of these are all internal so we can change it later. I made this decision for simplicity but am open to suggestions.

rainersigwald · 2016-11-18T01:18:02Z

src/XMakeBuildEngine/UnitTests/BackEnd/EventSourceSink_Tests.cs

Yikes! But I guess it's not worth implementing IEquatable just for this test.

Yeah I thought about making a Clone() method as well but I didn't want to go there... This unit test made sure I was properly copying every single property when I mutate the warning into an error (and I did miss one 😄)

rainersigwald · 2016-11-18T01:22:55Z

src/XMakeCommandLine/Resources/Strings.resx

"but the overall build will fail." seems a bit clearer.

Haha that's what I had originally and then I second guessed myself :sad:

rainersigwald · 2016-11-18T01:27:43Z

src/XMakeCommandLine/UnitTests/CommandLineSwitches_Tests.cs

Is this test implying that the switches are additive? I find that a bit surprising. And definitely I find it surprising that if the last one is a no-code-specified-turn-everything-on setting, that doesn't win.

I was imaging a scenario where the list of warning codes was specified in one or more response files, an environment variable and the command-line. I figured I would just conglomerate them into one set. If this is the case, then specifying the switch by itself doesn't add any more codes and some codes already exist so the functionality is to only treat the specified codes as errors.

This is sort of how /p:foo=bar works minus the ability to clear them all by specifying just /p. Let's discuss this tomorrow.

For posterity: we discussed, and decided that the additive model makes sense, but a new no-code-specified setting should override previous specific ones. If you need to clear the setting you can do /warnaserror /warnaserror:X to get back to a "just X" state.

rainersigwald · 2016-11-18T01:28:15Z

src/XMakeCommandLine/UnitTests/CommandLineSwitches_Tests.cs

ResourceManager.GetResourceSet() doesn't exist on .NET core. This test is just making sure I typed up the new command-line argument usage correctly and so it technically doesn't matter if it isn't running on .NET Core since it's testing all of the switches. The RESX resources are not #ifdef'd out, only the display of them.

Should I put a comment?

rainersigwald · 2016-11-18T01:29:46Z

src/XMakeCommandLine/XMake.cs

👻 trailing whitespace

rainersigwald · 2016-11-19T01:49:46Z

src/XMakeCommandLine/UnitTests/CommandLineSwitches_Tests.cs

typo: Parmaters

rainersigwald · 2016-11-19T01:50:00Z

src/XMakeCommandLine/CommandLineSwitches.cs

typo: an -> a

This originally said "an empty string", thanks for catching it

rainersigwald · 2016-11-19T01:52:52Z

src/XMakeCommandLine/UnitTests/CommandLineSwitches_Tests.cs

For posterity: we discussed, and decided that the additive model makes sense, but a new no-code-specified setting should override previous specific ones. If you need to clear the setting you can do /warnaserror /warnaserror:X to get back to a "just X" state.

rainersigwald · 2016-11-19T01:53:52Z

src/XMakeCommandLine/XMake.cs

I don't feel strongly about this. Doesn't seem worth it.

jeffkl · 2016-11-19T02:42:49Z

I'm going to squash this and re-push. When we're ready I'd like to do a merge commit so I can start on the /NoWarn switch and not have any merge conflicts.

Specify just /WarnAsError to have all warnings logged as errors as well as have the build fail. Specify a list of warning codes to have just that set of warnings treated as errors as well as have the build fail. Targets will still show as succeeded and the tasks will continue to execute but the overall build result will be a failure. Related to dotnet#68 and will close in my next change to add /NoWarn. * Feature switch FEATURE_RESOURCEMANAGER_GETRESOURCESET * Support for command-line arguments having empty values

dnfclas added the cla-already-signed label Nov 17, 2016

Sarabeth-Jaffe-Microsoft added the Needs Review label Nov 17, 2016

rainersigwald requested changes Nov 18, 2016

View reviewed changes

Sarabeth-Jaffe-Microsoft added the Feature Request label Nov 18, 2016

jeffkl force-pushed the 68 branch 2 times, most recently from 5be587d to 25c00b7 Compare November 18, 2016 23:23

rainersigwald approved these changes Nov 19, 2016

View reviewed changes

jeffkl force-pushed the 68 branch from 25c00b7 to e00eab4 Compare November 19, 2016 02:51

jeffkl force-pushed the 68 branch from e00eab4 to 2ee88dc Compare November 21, 2016 17:58

jeffkl modified the milestone: Visual Studio 15 RTM Nov 28, 2016

jeffkl merged commit 7633e84 into dotnet:xplat Nov 29, 2016

Sarabeth-Jaffe-Microsoft removed the Needs Review label Nov 29, 2016

jeffkl deleted the 68 branch December 7, 2016 20:12

rainersigwald mentioned this pull request Jan 12, 2021

HasLoggedError Respects MSBuildWarningsAsErrors #5957

Closed

2 tasks

benvillalobos mentioned this pull request Jan 15, 2021

Tasks Log.HasLoggedError now respects MSBuildWarningsAsErrors #6040

Closed

2 tasks

benvillalobos mentioned this pull request Feb 10, 2021

NuGet pack raises a warning, which is turned into an error, but the build still passes #4842

Closed

benvillalobos mentioned this pull request Feb 18, 2021

Tasks Log.HasLoggedError now respects MSBuildWarningsAsErrors #6174

Merged

2 tasks

Conversation

jeffkl commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffkl commented Nov 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jeffkl commented Nov 17, 2016 •

edited

Loading