Platform Specifications Should Match Casing Among All Build Scripts

[ivdiazsa]

Description

Recently, a conversation about making all platform ID's in build scripts lowercase was revived in PR #80164. However, the lack of uniformity can lead to unexpected and wrong behavior to unsuspecting developers passing flags directly to MSBuild when using other scripts, like for example eng/common/build.sh.

Reproduction Steps

See this example:

1. Build CLR and Libs:

./build.sh -s clr+libs -c Release

2. Generate the test layout and build some tests:

./src/tests/build.sh -x64 -release -generatelayoutonly
./src/tests/build.sh -x64 -release -test:JIT/Methodical/Methodical_d2.csproj

3. Generate the Merged Payloads for Helix locally:

./eng/common/msbuild.sh src/tests/Common/helixpublishwitharcade.proj -maxcpucount -bl -p:TargetArchitecture=x64 -p:TargetOS=Linux -p:TargetOSSubgroup= -p:Configuration=Release

Expected behavior

The Merged Payloads are generated in the same folder as the test builds.

Actual behavior

After running the last command, checking in under artifacts/tests/coreclr, there will be two variants of the same results folder:

• Linux.x64.Release
• linux.x64.Release

The test builds will be under linux and the Merged Payloads will be under Linux. Consequently, the Merged Payloads will be empty since no tests were found under that path.

Regression?

Yes, but according to the discussions in #80164 and other issues linked there, the lowercase behavior added in that PR is the one that the scripts ought to adopt.

Known Workarounds

Pass -p:TargetOS=linux instead of -p:TargetOS=Linux.

Configuration

No response

Other information

While this is not breaking by any means, at least for the time being, it can be very deceiving how many configuration options are case-insensitive (or are converted to a certain case behind curtains), and then suddenly one becomes case-sensitive. This confusion becomes more apparent when we consider the platform string is expected to be lower case, and all MSBuild properties are Pascal Case.

[ghost]

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

Recently, a conversation about making all platform ID's in build scripts lowercase was revived in PR #80164. However, the lack of uniformity can lead to unexpected and wrong behavior to unsuspecting developers passing flags directly to MSBuild when using other scripts, like for example eng/common/build.sh.

Reproduction Steps

See this example:

1. Build CLR and Libs:

./build.sh -s clr+libs -c Release

2. Generate the test layout and build some tests:

./src/tests/build.sh -x64 -release -generatelayoutonly
./src/tests/build.sh -x64 -release -test:JIT/Methodical/Methodical_d2.csproj

3. Generate the Merged Payloads for Helix locally:

./eng/common/msbuild.sh src/tests/Common/helixpublishwitharcade.proj -maxcpucount -bl -p:TargetArchitecture=x64 -p:TargetOS=Linux -p:TargetOSSubgroup= -p:Configuration=Release

Expected behavior

The Merged Payloads are generated in the same folder as the test builds.

Actual behavior

After running the last command, checking in under artifacts/tests/coreclr, there will be two variants of the same results folder:

• Linux.x64.Release
• linux.x64.Release

The test builds will be under linux and the Merged Payloads will be under Linux. Consequently, the Merged Payloads will be empty since no tests were found under that path.

Regression?

Yes, but according to the discussions in #80164 and other issues linked there, the lowercase behavior added in that PR is the one that the scripts ought to adopt.

Known Workarounds

Pass -p:TargetOS=linux instead of -p:TargetOS=Linux.

Configuration

No response

Other information

While this is not breaking by any means, at least for the time being, it can be very deceiving how many configuration options are case-insensitive (or are converted to a certain case behind curtains), and then suddenly one becomes case-sensitive. This confusion becomes more apparent when we consider the platform string is expected to be lower case, and all MSBuild properties are Pascal Case.

Author:	ivdiazsa
Assignees:	ivdiazsa
Labels:	area-Infrastructure
Milestone:	8.0.0

[ViktorHofer]

Yes, but according to the discussions in #80164 and other issues linked there, the lowercase behavior added in that PR is the one that the scripts ought to adopt.

I tried to cover all possible cases and scripts in that PR but based on the huge number of files using the TargetOS property, I might have missed a place. @ivdiazsa as you assigned yourself, I assume you will send a fix?

Note that we have validation in MSBuild that makes sure that the TargetOS value is lowercased:

runtime/Directory.Build.targets

Lines 81 to 84 in 9b76c28

	<Target Name="ValidateTargetOSLowercase"
	Condition="!$(TargetOS.Equals($(TargetOS.ToLower()), StringComparison.InvariantCulture))">
	<Error Text="The passed-in TargetOS property value '$(TargetOS)' must be lowercase." />
	</Target>

Presumably the runtime tests don't import the repo infrastructure?

[ivdiazsa]

I don't currently have a fix ready but I assigned myself because I'm willing to work on it. However, if you or anyone else has a fix ready, feel free to submit it.