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.

Sign up for GitHub

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

Jump to bottom

Enable NoWarn Fixes #4421 #5671

Merged
merged 8 commits into from
Oct 2, 2020
Merged

Enable NoWarn Fixes #4421 #5671

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
252ae6e
Enable NoWarn
Forgind Aug 21, 2020
9202b10
WarningsAsMessages, not Errors
Forgind Aug 24, 2020
b8272b6
Switch to updating default property
Forgind Aug 27, 2020
a0fab55
Correct change wave check
Forgind Sep 14, 2020
68a3e7f
Correct change wave check
Forgind Sep 15, 2020
88c9099
Update src/Tasks/Microsoft.Common.CurrentVersion.targets
Forgind Sep 21, 2020
0300e76
Merge branch 'master' of https://github.com/dotnet/msbuild into nowarn
Forgind Sep 25, 2020
e0c0f6c
Fix check
Forgind Sep 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1278,6 +1278,15 @@ private void ConfigureWarningsAsErrorsAndMessages()
}

ISet<string> warningsAsMessages = ParseWarningCodes(project.GetPropertyValue(MSBuildConstants.WarningsAsMessages));
ISet<string> noWarn = ParseWarningCodes(project.GetPropertyValue(MSBuildConstants.NoWarn));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do this in code instead of in common.targets?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially keeping in mind that it would have to consider if the user disabled an earlier change wave.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we will need change wave observation in properties. Not via environment variable but explicit change-wave checks.

if (warningsAsMessages is null)
{
warningsAsMessages = noWarn;
}
else if (noWarn is object)
{
warningsAsMessages.UnionWith(noWarn);
}

if (warningsAsMessages?.Count > 0)
{
Expand Down
5 changes: 5 additions & 0 deletions src/Shared/Constants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ internal static class MSBuildConstants
/// </summary>
internal const string WarningsAsMessages = "MSBuildWarningsAsMessages";

/// <summary>
/// Alternate name for the MSBuildTreatWarningsAsMessages property to bring MSBuild into line with other repositories.
/// </summary>
internal const string NoWarn = "NoWarn";

/// <summary>
/// The name of the environment variable that users can specify to override where NuGet assemblies are loaded from in the NuGetSdkResolver.
/// </summary>
Expand Down