Enable NoWarn Fixes #4421 #5671
Conversation
Fixes dotnet#4421 Currently not behind a warning wave, so will have to update that when it's available.
|
Wait, why would |
|
You're right; will fix. 😄 |
| @@ -1278,6 +1278,15 @@ private void ConfigureWarningsAsErrorsAndMessages() | |||
| } | |||
|
|
|||
| ISet<string> warningsAsMessages = ParseWarningCodes(project.GetPropertyValue(MSBuildConstants.WarningsAsMessages)); | |||
| ISet<string> noWarn = ParseWarningCodes(project.GetPropertyValue(MSBuildConstants.NoWarn)); | |||
There was a problem hiding this comment.
Why do this in code instead of in common.targets?
There was a problem hiding this comment.
I can move it there instead. When I was look for where I want to make a change, I sometimes forget to include .targets files in the search.
There was a problem hiding this comment.
Actually, after change waves come in, will there be an easy way to read in an environment variable and use it to determine whether a property should be reset?
There was a problem hiding this comment.
Especially keeping in mind that it would have to consider if the user disabled an earlier change wave.
There was a problem hiding this comment.
Yes, we will need change wave observation in properties. Not via environment variable but explicit change-wave checks.
| @@ -611,6 +611,11 @@ Copyright (C) Microsoft Corporation. All rights reserved. | |||
| <ContinueOnError Condition="'$(ContinueOnError)' == ''">false</ContinueOnError> | |||
| </PropertyGroup> | |||
|
|
|||
| <!-- Users familiar with how some other repos work try to use NoWarn with MSBuild in place of MSBuildWarningsAsMessages. --> | |||
| <PropertyGroup Condition="'$(MSBuildWarningsAsMessages)'=='' And $(MSBuildChangeWaveVersion) != '' And $([MSBuild]::VersionGreaterThanOrEquals('$(MSBuildChangeWaveVersion)', '16.8'))"> | |||
There was a problem hiding this comment.
| <PropertyGroup Condition="'$(MSBuildWarningsAsMessages)'=='' And $(MSBuildChangeWaveVersion) != '' And $([MSBuild]::VersionGreaterThanOrEquals('$(MSBuildChangeWaveVersion)', '16.8'))"> | |
| <PropertyGroup Condition="'$(MSBuildWarningsAsMessages)'=='' And $([MSBuild]::VersionGreaterThanOrEquals('$(MSBuildChangeWaveVersion)', '16.8'))"> |
MSBuildChangeWaveVersion is guaranteed to have a proper value, so this check isn't necessary.
There was a problem hiding this comment.
That's true whether or not the user specifies a value? And does that also get pulled in for random tests?
There was a problem hiding this comment.
Yes, each project will have MSBuildChangeWaveVersion set as a reserved property. If unset, it currently defaults to 999.999.
And does that also get pulled in for random tests?
I'm not sure what you mean?
There was a problem hiding this comment.
I added that because unit tests failed without it. Unit tests often don't have a proper project/run a proper build, and I haven't checked whether these particular tests do or don't. Is that a problem?
There was a problem hiding this comment.
This is actually a great point and something that'll have to be added to the docs. I had to make sure to build using a ProjectCollection so that the reserved properties would be set at all. Tests that need to build should be done similar to this:
string projectFile = @"
<Project>
<Target Name='HelloWorld' Condition=""'$(MSBUILDCHANGEWAVEVERSION)' == '999.999' and $([MSBuild]::VersionLessThan('" + featureWave + @"', '$(MSBUILDCHANGEWAVEVERSION)'))"">
<Message Text='Hello World!'/>
</Target>
</Project>";
TransientTestFile file = env.CreateFile("proj.csproj", projectFile);
ProjectCollection collection = new ProjectCollection();
MockLogger log = new MockLogger();
collection.RegisterLogger(log);
collection.LoadProject(file.Path).Build().ShouldBeTrue();Are there a significant amount of tests that fail here?
Co-authored-by: Ben Villalobos <bevillal@microsoft.com>
Enable NoWarn Fixes dotnet#4421
Fixes #4421
Currently not in a change wave, so will have to update that when it's available.