Move setting variables to HelixPreCommands

[BruceForstall]

Don't inject them in the generated RunTests.cmd script.

Namely, __IsXUnitLogCheckerSupported=1 and __TestArchitecture.

[ghost]

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

Issue Details

---

Don't inject them in the generated RunTests.cmd script.

Namely, __IsXUnitLogCheckerSupported=1 and __TestArchitecture.

Author:	BruceForstall
Assignees:	BruceForstall
Labels:	area-Infrastructure-libraries, needs-area-label
Milestone:	-

[ViktorHofer]

This is interesting as @agocke suggested in @carlossanlop's original PR to use the script infrastructure.

[ghost]

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

Issue Details

---

Don't inject them in the generated RunTests.cmd script.

Namely, __IsXUnitLogCheckerSupported=1 and __TestArchitecture.

Author:	BruceForstall
Assignees:	BruceForstall
Labels:	area-Infrastructure
Milestone:	-

[carlossanlop]

This is interesting as @agocke suggested in @carlossanlop's original PR to use the script infrastructure.

+1, agreed.

@BruceForstall - I actually got my first PR blocked from merging until I moved the setting of the variables inside the runner scripts. Since your change is essentially going in the opposite direction, it would conflict with Andy's original blocking request.

@agocke, @ivdiazsa @hoyosjs can you please weigh in on this change? It's affecting both coreclr and libraries so this sounds like a decision that needs to be made by the XUnitLogChecker owners.

[ivdiazsa]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ivdiazsa]

Looking at these changes as they are, I don't think they would cause any problems. That said, let's wait until the pipelines come back with results. I also started an outerloop run to get better coverage. If everything goes fine, then I think we should be able to merge this.

[ivdiazsa]

This is interesting as @agocke suggested in @carlossanlop's original PR to use the script infrastructure.

My comments said, I still would like to hear the motivation behind this, as Andy Gocke preferred the original way and explained why in Carlos' original PR.

[BruceForstall]

I mentioned in #95762 (comment) that __TestArchitecture should not be set in the RunTests.cmd script because that makes the script architecture-specific, which it should not be (e.g., building everything for x86 then for x64 overwrites the RunTests.cmd script which then is not appropriate for x86. Anything built in the build that is architecture-specific must be built into architecture-specific directories).

I would prefer that __TestArchitecture not be set at all, in any script, and the code that needs it just figures out the architecture when it needs to (when it is looking for cbg.exe).

I don't care so much about __IsXUnitLogCheckerSupported but it seems it should follow the same path as __TestArchitecture.

[BruceForstall]

Related links:

#93906
#94868
#95762

[agocke]

The main thing I wanted to prevent was us messing with the YAML to run tests, as I have a high degree of concern with trying to flow down too much information. This still avoids messing with YAML, so I would be OK with moving this to the helix invocation scripts.

[agocke]

Also, no opinion on the TestArchitecture stuff -- I was focusing on the LogCheckerSupported pieces.

[ivdiazsa]

Thanks a lot for your input @agocke. In this case, then I have no further comments on this, besides that it lgtm. I've rerun the failing pipelines. I think those failures were unrelated, but I'm not 100% sure. If this is the case, and no one else has further comments to address, then I would say this is good and ready to merge.

[ViktorHofer]

Is XUnitLogChecker just a CI tool or does it work locally as well when running tests? If the former then it might make sense to condition this item group on '$(ArchiveTests)' == 'true'.

[BruceForstall]

Maybe @ivdiazsa or @carlossanlop can answer that.

I realized I didn't want to mess with the XUnitLogChecker since a recent fix made it not break on local dev machines. So I've scoped back this PR to only fix the architecture-specific setting of __TestArchitecture.

[ivdiazsa]

XUnitLogChecker can be run locally, but it has to be called manually by the user. Automatically, it's only run through the CI scripts. That said, it is always built alongside all common test tools, like for example with:

# XUnitLogChecker is included in all that would be built with this command.
src/tests/build.sh -x64 -release -generatelayoutonly