Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a "treat warnings as errors" option #68

Closed
Porges opened this issue Apr 10, 2015 · 31 comments
Closed

Add a "treat warnings as errors" option #68

Porges opened this issue Apr 10, 2015 · 31 comments
Assignees
Labels
help wanted Issues that the core team doesn't plan to work on, but would accept a PR for. Comment to claim. triaged User Experience

Comments

@Porges
Copy link

Porges commented Apr 10, 2015

I would like an option to be able to treat all warnings as errors, when running from the command line.

@AndyGerlicher
Copy link
Contributor

Thanks for the suggestion, sounds like a potentially useful feature. This doesn't meet the bar for our roadmap at this time however.

@jbjoshi
Copy link

jbjoshi commented Apr 15, 2015

Is there a Roadmap defined somewhere?

@eatdrinksleepcode
Copy link

If it is a potentially useful feature, why is the issue closed? Someone from the community may want to pick it up.

@chandramouleswaran
Copy link

As a work around, you should be (I think) able to add TreatWarningsAsErrors property in a targets file - set it to true and invoke that target as a part of the msbuild command line argument along with your default targets. Wouldn't that work?

@Porges
Copy link
Author

Porges commented Apr 15, 2015

@chandramouleswaran: that's for the compiler warnings, not for warnings from MSBuild itself.

I want to be able to make things like missing files be errors, not warnings like they are currently.

@chandramouleswaran
Copy link

@Porges - Good point 👍

@rainersigwald
Copy link
Member

Reopening and marking as up-for-grabs. This isn't high on our priority list but it's a good feature to have.

@rainersigwald rainersigwald reopened this Sep 1, 2015
@rainersigwald rainersigwald added the help wanted Issues that the core team doesn't plan to work on, but would accept a PR for. Comment to claim. label Sep 1, 2015
@iouri-s
Copy link

iouri-s commented Nov 19, 2015

We are getting a bunch of these warnings as well. The code accumulated dozens of warnings such as importing a target file several times, even though we have TreatWarningsAsErrors=true.

@D3-LucaPiombino
Copy link

We are in the same situation.

Use case

Misconfigured Platform across a Solution

Having some dependency that require a specific platform (e.g. x64) but that when build from msbuild using the solution configuration matrix produce:

C:\Program Files (x86)\MSBuild\14.0\bin\Microsoft.Common.CurrentVersion.targets (1820, 5)
There was a mismatch between the processor architecture of the project being built "MSIL" and the processor architecture of the reference "Microsoft.ServiceFabric.Internal", "AMD64". This mismatch may cause runtime failures. Please consider changing the targeted processor architecture of your project through the Configuration Manager so as to align the processor architectures between your project and references, or take a dependency on references with a processor architecture that matches the targeted processor architecture of your project.

Since this is plugged in a CI/CD workflow, it would be nice to be able to block the release process in this case.

@dotMorten
Copy link

dotMorten commented Aug 19, 2016

We want the appveyor builds to fail if a PR introduces new warnings. However we don't want the dev to deal with this permanently as they are working towards a PR. Essentially we want gated checkins that also prevents users from introducing new warnings. Simply being able to do this through a commandline would be the easiest way.

@jeffkl jeffkl self-assigned this Nov 16, 2016
@jeffkl
Copy link
Contributor

jeffkl commented Nov 16, 2016

I'm working on this for MSBuild 15.

Design

A new command-line argument /WarnAsError to enable the functionality. When a warning is treated as an error, the execution of the target will continue but the overall build will be marked as failed.

All Warnings as Errors

Specifying just /WarnAsError will log all warnings as errors and the build will fail.

Example

msbuild.exe myproject.proj /warnaserror

Specifying Which Warnings Should Be Errors

Specifying a comma-delimited list of warning codes will treat just that set of warnings as errors. The value can include duplicates and only that set will be logged as errors and the build will fail.

Example

msbuild.exe myproject.proj /warnaserror:MSB3000,MSB4000,CA1000,FOO,123

This will also work within response files.

Suppressing Warnings

Although we don't recommend it, there are cases when it might be necessary to suppress warnings. To do this, you'll be able to specify a comma-delimited list of warning codes to suppress. You must specify a list of warnings to suppress as we will not be just suppressing everything.

Example

msbuild.exe myproject.proj /nowarn:MSB1000,MSB2000,FOO,123

Open Questions

  1. The values cannot be MSBuild properties so they are passed in as command-line arguments or contained in response files. Should there be environment variables as well?
  2. The build will fail if a warning is treated as an error but tasks won't stop executing. The target execution model will still treat it as a warning and tasks will continue executing. This is because warnings are typically non-critical. Is this okay?
  3. Is it acceptable that attached loggers will only receive the mutated warning as an error and will not be aware that it was supposed to be a warning?

Please send me any feedback on this design.

@iouri-s
Copy link

iouri-s commented Nov 16, 2016

This design would satisfy our needs. Replacing "can not" with "cannot" in the first open question would make it easier to understand.

@dotMorten
Copy link

  1. The values can not be MSBuild properties so they are passed in as command-line arguments or contained in response files

Does this mean we can't enable this in .csproj? That would suck ☹

@jeffkl
Copy link
Contributor

jeffkl commented Nov 17, 2016

@dotMorten The problem is that project properties are declared too late. MSBuild logs warnings during project evaluation which would mean they could not be turned into errors. For example:

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup Condition="true or true and true">
    <MSBuildWarningsAsErrors>MSB4130</MSBuildWarningsAsErrors>
  </PropertyGroup>
  <Target Name="Build" />
</Project>

Results in the MSB4130 warning:

warning MSB4130: The condition "true or true and true" may have been evaluated incorrectly in an
earlier version of MSBuild. Please verify that the order of the AND and OR clauses is written
 as intended.  To avoid this warning, add parentheses to make the evaluation order explicit.

The warning is logged before the MSBuildWarningsAsErrors property is declared. The TreatWarningsAsErrors works as a property because the compiler is called after the property is declared. But I don't know of any property you can declare in your project that will affect MSBuild itself, just the targets that are executed. There's also the problem of project-to-project references where MSBuild is building a massive tree of hundreds of projects in a solution, if the setting was a property in one of the projects, should it apply to everything in the build graph?

It might be possible to add a new property to the <MSBuild /> task which would let you use a property to control it. But I felt that in order to really nail this scenario, it needed to be a global control knob. Since it can only be enabled as a command-line parameter it will only really be helpful in the hosted build scenario for things like CI environments. So local builds in Visual Studio and command-line buiilds wthout the switch would still emit warnings but branch owners would be able to keep the warnings from making their way into the code base.

You had original stated:

Simply being able to do this through a commandline would be the easiest way.

And this design should provide this capability. Does this seem reasonable?

jeffkl added a commit to jeffkl/msbuild that referenced this issue Nov 17, 2016
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 dotnet#68 and will close in my next change to add /NoWarn.
jeffkl added a commit to jeffkl/msbuild that referenced this issue Nov 17, 2016
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 dotnet#68 and will close in my next change to add /NoWarn.
jeffkl added a commit to jeffkl/msbuild that referenced this issue Nov 17, 2016
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 dotnet#68 and will close in my next change to add /NoWarn.
jeffkl added a commit to jeffkl/msbuild that referenced this issue Nov 17, 2016
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 dotnet#68 and will close in my next change to add /NoWarn.
jeffkl added a commit to jeffkl/msbuild that referenced this issue Nov 18, 2016
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 dotnet#68 and will close in my next change to add /NoWarn.
jeffkl added a commit to jeffkl/msbuild that referenced this issue Nov 19, 2016
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
jeffkl added a commit to jeffkl/msbuild that referenced this issue Nov 19, 2016
Warnings are logged as low importance messages instead.  This way they can still be seen with more verbosity but are essentially suppressed.

Closes dotnet#68
jeffkl added a commit to jeffkl/msbuild that referenced this issue Nov 21, 2016
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
@danmoseley
Copy link
Member

danmoseley commented Nov 22, 2016

We used to recommend that all tasks return return !Log.HasLoggedErrors; from Execute() -- the idea being that all well behaved tasks log one or more errors if and only if they are fail -- really Execute() should have been void and the engine could have enforced this. Anyway -- what happens to tasks that log a warning which is upgraded to an error. I didn't look carefully at the change but I'm guessing TaskLoggingHelper (aka Log) is unaware so the task will still succeed in this case and build will continue. Presumably that's what desired.

[edited to make sense]

@jeffkl
Copy link
Contributor

jeffkl commented Nov 22, 2016

The logic will be if you specify /warnaserror than the overall build result will fail if any errors are logged. Individual targets can still show as "Success". So if I build pragmatically via Build(), you would get back a BuildResult with an OverallResult of BuildResultCode.Failure. But in the ResultsByTarget collection, all of the target results could show as success.

If all of the tasks return !Log.HasLoggedErrors than the target would show as fail like you said so the logic is a catch-all. It would be a little harder to fail the task/target so I figured it was good enough that at least the overall build fails. Do you agree?

@iouri-s
Copy link

iouri-s commented Nov 22, 2016

@jeffkl - I am not sure I am convinced on the command line switch vs. MSBuild property argument.
For most cases, specifying the property early enough would solve the corner case you show.
It is expected in a project file that an msbuild property does not take effect before it is declared.
The behavior of a switch can be achieved with the existing /p command-line switch even if it is a property.
Project-to-project references are not a problem either. Typically, a (compiler) warnings as errors policy is team-wide, and teams already have a process in place for ensuring that the property gets into all the necessary projects, by creating a common targets file, for example.
Having inconsistent behavior between hosts will create further confusion and frustration.

@jeffkl
Copy link
Contributor

jeffkl commented Nov 22, 2016

@iouri-s The main concern is that if you set a property to suppress a warning and there is ever a case where that warning wouldn't be treated as an error, it defeats the purpose of this functionality. My example above is not really a corner case in my opinion.

Do you think an environment variable would good enough?

I believe that the local build experience will still show warnings unless people manually specify to treat them as errors. But the /warnaserror switch would at least let hosted build environments like CI to fail if a warning is introduced. This means that pull request validation builds would fail and users would know they need to fix the warning.

I would love to come up with a way to have the warning list be an MSBuild property but I just don't see a way to ensure that what is specified will actually be treated as an error.

jeffkl added a commit that referenced this issue Nov 29, 2016
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 #68 and will close in my next change to add /NoWarn.

* Feature switch FEATURE_RESOURCEMANAGER_GETRESOURCESET
* Support for command-line arguments having empty values
jeffkl added a commit to jeffkl/msbuild that referenced this issue Nov 29, 2016
Warnings are logged as low importance messages instead.  This way they can still be seen with more verbosity but are essentially suppressed.

Closes dotnet#68
jeffkl added a commit to jeffkl/msbuild that referenced this issue Nov 29, 2016
Warnings are logged as low importance messages instead.  This way they can still be seen with more verbosity but are essentially suppressed.

Closes dotnet#68
jeffkl added a commit that referenced this issue Nov 29, 2016
You cannot suppress all warnings, if you specify empty `/warnasmessage` or `/nowarn`, you get an error message.
Warnings are mutated into just a message.
Ignore the first commit which is the implementation of /WarnAsError

Closes #68
Closes #47
Closes #910 

So this:
```
MyProject.proj(3,18): warning MSB4130: The condition "true or true and true" may have been evaluated incorrectly 
in an earlier version of MSBuild. Please verify that the order of the AND and OR clauses is written as intended. To 
avoid this warning, add parentheses to make the evaluation order explicit.
```
Becomes this in with Verbosity=Detailed 
```
MyProject.proj(3,18): message MSB4130: The condition "true or true and true" may have been evaluated incorrectly
in an earlier version of MSBuild. Please verify that the order of the AND and OR clauses is written as intended. To
avoid this warning, add parentheses to make the evaluation order explicit.
```

Here is the help message for review:
```
  /warnasmessage[:code[;code2]]
                     List of warning codes to treats as low importance
                     messages.  Use a semicolon or a comma to separate
                     multiple warning codes.
                     (Short form: /nowarn[:c;[c2]])

                     Example:
                       /warnasmessage:MSB3026
```
@davkean
Copy link
Member

davkean commented Jan 17, 2017

@jeffkl Hey Jeff, have you talked to anyone inside Visual Studio to get an option inside VS to turn this on?

@Porges
Copy link
Author

Porges commented Jan 17, 2017

I never noticed this was closed. Thanks @jeffkl! 👍

@jeffkl
Copy link
Contributor

jeffkl commented Jan 17, 2017

@davkean Not yet, any recommendations on who to talk to?

@Porges You're very welcome, I've been wanting the feature myself for a long time. Spread the word!

@memark
Copy link

memark commented Nov 9, 2017

Any update on this?

@jeffkl
Copy link
Contributor

jeffkl commented Nov 9, 2017

@memark the command-line arguments shipped in MSBuild 15.1 and the properties shipped in MSBuild 15.3 via #1928

radical added a commit to radical/msbuild that referenced this issue Jul 27, 2018
.. which was getting overwritten by an older one, due to a bad merge!

The older one is missing the commit
a5a9fd5, required for linux builds.

Thanks to @directhex for catching this.
@solvingj
Copy link

solvingj commented Aug 5, 2019

@jeffkl this is a great improvement, thank you!

This was closed quite a while ago. Since then, have you worked with the VS team to get this added as a VS option as @davkean mentioned? Until that exists, it's my understanding that there is simply no way to control this behavior for any Visual-Studio-Driven workflows.

In our specific case, we would want to add a property to Directory.Build.Props. As @iouri-s mentioned, the property would work for a majority of cases. At least it would be a start, and could come with the disclaimer that users would have to ensure the order it was processed.

Of note, for native C/C++ projects, we have ForceImportBeforeCppTargets flag for MSBuild. However that feature is implemented in MSBuild seems like it could be generalized to ForceImportBeforeAnythingElse for this very specific case of "bootstrapping MSBuild settings".

@rainersigwald
Copy link
Member

@solvingj <MSBuildTreatWarningsAsErrors>true</MSBuildTreatWarningsAsErrors> has been supported since MSBuild 15.3:

the command-line arguments shipped in MSBuild 15.1 and the properties shipped in MSBuild 15.3 via #1928

@solvingj
Copy link

solvingj commented Aug 5, 2019

Thank you for that @rainersigwald . I was actually more specifically needing to solve this related issue of suppressing specific warnings, in my case MSB4011:
#910

I'm about to look at that link you provided to see if there's a mechanism for this as well.

@solvingj
Copy link

solvingj commented Aug 5, 2019

It looks like this has no effect, likely due to the limitation explained in the start of the PR.
<MSBuildWarningsAsMessages>MSB4011</MSBuildWarningsAsMessages>

I guess the remaining questions is, how is it possible that there is no way for the user to append/alter or otherwise affect the MSBuild flags when using Visual Studio? If that existed, we could have easily passed /WarnAsMessage (and any other future flag) from visual studio without even needing this property, and without any caveats.

@MattBussing
Copy link

@rainersigwald , if I understand this comment correctly, Visual Studio should support MSBuildWarningsAsErrors. It isn't working for me. Here is my Directory.Build.props.

<Project>
    <PropertyGroup>
        <MSBuildWarningsAsErrors>MSB3276;MSB3247;MSB3277;NU1605;MSB3245;MSB3243</MSBuildWarningsAsErrors>
        <WarningsAsErrors>MSB3276;MSB3247;MSB3277;NU1605;MSB3245;MSB3243</WarningsAsErrors>
    </PropertyGroup>
</Project>

I am using Visual Studio 2019. When I build my solution it doesn't fail the build or output the warnings as errors to the error pane. It just outputs them in the warning pane like before I set the MSBuildWarningsAsErrors setting. Also, I did this after deleting the .vs folder and clearing the bin and obj folders. On the other hand, Rider and MSBuild will fail and show the errors.

What do you or anyone else recommend we do to get this fixed?

p.s. I know I put the same warnings in both MSBuildWarningsAsErrors and WarningsAsErrors. I wasn't sure what the difference was, so I used both.

@rainersigwald
Copy link
Member

@MattBussing I don't reproduce your problem. Can you file a new issue with more details (ideally a repro project) please?

image

p.s. I know I put the same warnings in both MSBuildWarningsAsErrors and WarningsAsErrors. I wasn't sure what the difference was, so I used both.

Great news! As of #5774 you can just use WarningsAsErrors.

In the past, WarningsAsErrors was passed explicitly to some places (notably the C# compiler) but never applied to MSBxxxx messages, and many tasks didn't use it. MSBuildWarningsAsErrors is respected by MSBuild itself, so it can apply to any* warning that has a code.

* This may actually be the problem you're seeing; some of MSBuild's warnings are emitted before we can read this property and thus can't be changed by the property. But please let us know the specific problem you're seeing.

@MattBussing
Copy link

@rainersigwald , thank you so much for the quick response! Thanks for your patience while I filed another issue. Here is the issue with a project that reproduces the error. #6473

So what you are saying for the second part is that WarningsAsErrors is the same as MSBuildWarningsAsErrors now?

@rainersigwald
Copy link
Member

So what you are saying for the second part is that WarningsAsErrors is the same as MSBuildWarningsAsErrors now?

Correct, in MSBuild 16.9 and higher.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues that the core team doesn't plan to work on, but would accept a PR for. Comment to claim. triaged User Experience
Projects
None yet
Development

No branches or pull requests