Cleaner way of specifying environment variables for tests

[markples]

Existing ways of specifying environment variables:

• [3 times] CLRTestBashEnvironmentVariable is a list of export var=value strings
• [2 times] CLRTestBatchEnvironmentVariable is a list of set var=value strings
• [~100 times] BashCLRTestPreCommands and CLRTestBatchPreCommands are set to include export/set strings for the same variables

This changes CLRTestBashEnvironmentVariable and CLRTestBatchEnvironmentVariable to be a list of Identity/Value pairs and adds a new list CLRTestEnvironmentVariable in the same format. These are automatically expanded to the necessary export/set strings as appropriate.

A few details of note:

• "Real" changes are in CLRTest.Execute.Bash/Batch.targets. This moves the environment variables section of the generated cmd/sh files down to the precommands area. This is only a change for the 5 existing uses, which look ok.
• If a test has a more complicated precommands list, it may not be appropriate to extract the variables. For example, src\tests\readytorun\tests\mainv1.csproj includes them in setlocal/endlocal because they are for crossgen2 in the precommands, not the test itself. This change only updates otherwise blank precommands lists.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Existing ways of specifying environment variables:

• [3 times] CLRTestBashEnvironmentVariable is a list of export var=value strings
• [2 times] CLRTestBatchEnvironmentVariable is a list of set var=value strings
• [~100 times] BashCLRTestPreCommands and CLRTestBatchPreCommands are set to include export/set strings for the same variables

This changes CLRTestBashEnvironmentVariable and CLRTestBatchEnvironmentVariable to be a list of Identity/Value pairs and adds a new list CLRTestEnvironmentVariable in the same format. These are automatically expanded to the necessary export/set strings as appropriate.

A few details of note:

• "Real" changes are in CLRTest.Execute.Bash/Batch.targets. This moves the environment variables section of the generated cmd/sh files down to the precommands area. This is only a change for the 5 existing uses, which look ok.
• If a test has a more complicated precommands list, it may not be appropriate to extract the variables. For example, src\tests\readytorun\tests\mainv1.csproj includes them in setlocal/endlocal because they are for crossgen2 in the precommands, not the test itself. This change only updates otherwise blank precommands lists.

Author:	markples
Assignees:	markples
Labels:	area-CodeGen-coreclr
Milestone:	-

[markples]

This diff condenses 6 variables to 5 because COMPlus_TC_OnStackReplacement is defined twice.

[markples]

@trylek @jkoritzinsky Could one of you please take a look at this? This is separate from the naming cleanup that was discussed in the disasm checks PR, though it does remove a lot of uses of the precommands variables. I doubt this will conflict with anything that you're doing, and if anything, putting these in symbolic form might help.

cc @dotnet/jit-contrib This makes the specification of environment variables much simpler/shorter for tests.

[kunalspathak]

Nice cleanup. Did you use some script to convert the old to new format?

[markples]

Did you use some script to convert the old to new format?

Yes, I used a very brittle C# program, but it looks like all of these were copy/pasted since it worked on all of them.

[kunalspathak]

Yes, I used a very brittle C# program, but it looks like all of these were copy/pasted since it worked on all of them.

Got it. I was asking, so we don't have to review changes in each and every *.csproj :)

[kunalspathak]

Not sure if failures are related though.

[markples]

Got it. I was asking, so we don't have to review changes in each and every *.csproj :)

I did the 5 (not PreCommands) cases by hand and did look through the others myself. That was how I noticed that one duplicate definition that I added a comment about :)

[trylek]

Nit - feel free to ignore this or leave for a subsequent mop-up PR especially if you'd need to regenerate all the project changes but it would be nice to get rid of the emptied property groups.

[markples]

This is a really good catch. Most of the empty property groups are gone. My script did that, but I shouldn't have left it quiet when it found additional lines since my audit obviously missed that. In a few cases there were comments that should be moved as well. I'll do that by hand on top of these changes.

[trylek]

I noticed that these are not really frequent and are probably just caused by the comments which are presumably valuable and can be moved to the new style environment variable items exactly as you say. Thanks for doing that!

[markples]

Just for reference - 24e297a does this. I removed a few comments that said capitalization matters for Linux because it applies to every one of these.

[trylek]

Now this is a really beautiful cleanup, thanks so much Mark!

[markples]

Not sure if failures are related though.

https://dev.azure.com/dnceng-public/public/_build/results?buildId=36583&view=logs&jobId=2de479a8-eae7-5324-c49d-bebaef329288&j=2de479a8-eae7-5324-c49d-bebaef329288&t=bb577e75-8247-55f1-4164-04f3b0f2d1eb

looks like it might be #73299

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-76458-merge-33b41689da3c4da681/JIT.1/1/console.d8e8c8d8.log?helixlogtype=result

is a bit suspicious since pinnedlocal is actually a test where this PR moves the GCStress=3 to the new format. #70641 contains a reference (from June) about this test failing intermittently on Mono for a long time. It looks like most PRs skip this test leg.

My changes have triggered another run, so we'll get another data point.

[BruceForstall]

Looks good.

Did you consider eliminating the COMPlus_ or DOTNET_ prefix in the names? E.g., instead of:

<CLRTestEnvironmentVariable Include="COMPlus_DoubleArrayToLargeObjectHeap" Value="0x64" />

use:

<CLRTestEnvironmentVariable Include="DoubleArrayToLargeObjectHeap" Value="0x64" />

and have the CLRTestEnvironmentVariable processing auto-prepend DOTNET_? It would help in getting us away from using COMPlus_. I suppose you might need a CLRTestEnvironmentVariableRaw that doesn't do that, for some cases.

[markples]

Did you consider eliminating the COMPlus_ or DOTNET_ prefix in the names?

@BruceForstall I didn't think of that. Maybe we could even use metadata Raw="true" in the same list.

There are various places in the tree that check for one of COMPlus_ or DOTNET_ so I get a bit worried about messing with them. The compatibility checks in the test .cmd/.sh are one place, though these variables are set after those checks so that should be ok. I'd prefer to keep a rename separate from this mechanism change.

[markples]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[markples]

Failure was JIT/opt/OSR/pinnedlocal/pinnedlocal.sh on Mono Browser wasm Release @ Ubuntu.1804.Amd64.Open.

This test was known to occasionally fail on Mono and is now disabled by #76980