Common.targets doesn't promote TreatWarningsAsErrors to MSBuildTreatWarningsAsErrors

[rainersigwald]

Common.targets does respect the "plain property" version of some warning-related settings:

msbuild/src/Tasks/Microsoft.Common.CurrentVersion.targets

Lines 668 to 672 in 69b3e7a

	<PropertyGroup>
	<MSBuildWarningsAsMessages Condition="'$(MSBuildWarningsAsMessages)'==''">$(NoWarn)</MSBuildWarningsAsMessages>
	<MSBuildWarningsAsErrors Condition="'$(MSBuildWarningsAsErrors)'==''">$(WarningsAsErrors)</MSBuildWarningsAsErrors>
	<MSBuildWarningsNotAsErrors Condition="'$(MSBuildWarningsNotAsErrors)'==''">$(WarningsNotAsErrors)</MSBuildWarningsNotAsErrors>
	</PropertyGroup>

but it doesn't do the broader setting MSBuildTreatWarningsAsErrors in the same way.

This adds to the confusion about which settings to use.

[SimaTian]

We've implemented the intended solution here. Then we had to roll it back due to breaking sdk tests while updating msbuild dependencies.
The crux of the issue is the fact that some projects started to use non-prefixed version to selectively turn on TreatWarningsAsErrors - e.g. they have use case like TreatWarningsAsErrors only for this subproject. This is a legitimate use case.
Unfortunately this leaves us in a weird state where our attempt to unify the behavior breaks more people than expected.

Current status:

• we have a code change ready
• we have validated that it works
• unfortunately it breaks lot of builds.

There are several possible ways forward:

• We could unify everything except the TreatWarningsAsErrors which is the most commonly used "subproject specific" one. It would bring us somewhat closer to an unified world. I don't like this solution as it creates a weird "exception inside an exception" kind of state, possibly adding another layer of complexity to an already weird behavior.
• We could break people - obviously, we don't want to do that.
• We could create a build check, let it simmer, then break people with a changewave, then break people. This would require lot of babysitting, but maybe it could be a way forward, although not a straightforward one. Also, it still breaks people even if we soften the blow somewhat.
• We can keep the status quo, since it is a minor annoyance for the most part. This might be bad for the nuget issue that initially gave us the motivation to attempt this unification.

@baronfel, what do you think please?

[baronfel]

Adding @aortiz-msft for visibility and insight into the impact to NuGet

[aortiz-msft]

@nkolev92

[nkolev92]

@SimaTian reached out to me offline about this.

I am comfortable with the 1st proposed path forward in #10871 (comment).

MSBuildTreatWarningsAsErrors came after TreatWarningsAsErrors. It will always be a breaking change. I don't have data on how often MSB warnings exist in builds but feels like something that happens quite frequently in large repos.

I think #10801 is way more important.
Currently it forces people to duplicate 3 properties. It seems like MSBuild is elevating errors that another component chose not to elevate.
To me naively seems like MSBuildTreatWarningsAsErrors and the other 3 MSBuild prefixed properties should've applied to MSB prefixed warnings only.