August infra rollout: continued cleanup of runtime tests

[trylek]

Follow-up of:

#37440

After the runtime test source code and project scripts have been moved under src/tests, I have the following proposals for further test cleanup:

1. Mop-up changes - moving the rest of src/coreclr/tests/src and ideally the entire src/coreclr/tests folder under src/tests;
2. Move build-test.cmd / build-test.sh from src/coreclr to src/tests; it might make sense to rename them to just build.cmd / build.sh in the process as the "test" part is implied by the folder and becomes superfluous in the script name;
3. Modify the logic for Crossgen1/Crossgen2 compilation of framework assemblies to r2rtest or, if that doesn't work, to MSBuild;
4. Start moving most of functional logic in the build-test scripts to the build.proj script with the ultimate goal of either keeping the two scripts as just thin command-line front-ends or deleting them altogether and triggering test build directly from the root build script. I don't think this is achievable for the August infra rollout in its entirety but it would be good to agree on the general process and start executing on it.

/cc: @dotnet/runtime-infrastructure

[trylek]

/cc: @jkotas @marek-safar @SamMonoRT @naricc @ViktorHofer

[trylek]

@ViktorHofer - As I want to move the build-test scripts under src/tests, I'm wondering where to put the script src/coreclr/setup_vs_tools.cmd that is called both from the CoreCLR build scripts and from the test build script; do you think it would be reasonable to move it under eng with the rationale that it's more or less generally usable?

[jkotas]

Yes, it would be nice to switch other places where we have this logic duplicated while you are on it:
https://github.com/dotnet/runtime/blob/master/src/libraries/Native/build-native.cmd#L43
https://github.com/dotnet/runtime/blob/master/src/installer/corehost/build.cmd#L48

[ViktorHofer]

Ideally we would use what comes with Arcade. I believe they are doing something similar.

[jkotas]

I believe they are doing something similar.

I do not see anything close enough in Arcade.

[trylek]

Thanks @jkotas and @ViktorHofer for your quick feedback. It seems to me that somewhat similar logic resides in the script

runtime/eng/common/tools.ps1

Line 287 in 6998f4c

function InitializeVisualStudioMSBuild([bool]$install, [object]$vsRequirements = $null) {

I can look into switching over the existing scripts to use this logic instead of using several open-coded versions; I have not yet verified if it's a full replacement of the existing VS detection logic bits though.

[jkotas]

I have not yet verified if it's a full replacement of the existing VS detection

It is not. eng/common/tools.ps1 locates msbuild. We need to setup environment for C++ compilers that is quite different.

Also, eng/common/tools.ps1 has non-desirable reliability characteristics. It downloads vswhere from the network, instead of just using the one one the machine. I do not think we want yet another place where the build can fail due to network glitch.

[trylek]

Thanks @jkotas for clarifying. Adding @dotnet/runtime-infrastructure to chime in as I believe JanK's responses imply that:

1. We cannot use the existing Arcade scripting logic to locate and set up VS build environment.

2. Should we perhaps create a new future-facing issue to work with Arcade on adding such logic?

3. As a corollary of (1), for now we need to keep some homebrew scripting logic for VS environment setup.

4. Is src/coreclr/setup_vs_tools.cmd the best implementation? Should we somehow consolidate it with the other versions of this detection logic that JanK mentioned above?

5. We want to unify several places in the runtime repo scripts doing VS detection and setup. Is it reasonable to move the consolidated VS setup script under the eng folder?

[naricc]

So, in this new directory structure, where should any runtime test related utilities go? Undr /src/tests? Specifically I am thinking of the wasm/android test runner I am creating.

[trylek]

@naricc - Right now the various scripts and utilities reside under src/tests/Common inherited from src/coreclr/tests/src/Common; I don't think it's ideal but it was the option causing the least amount of churn. I'm thinking about adding a new folder src/tests/eng where I'd move the various remaining scripts from src/coreclr/tests (perhaps under a coreclr folder) and use for all test infrastructure stuff going forward; ultimately we should probably move the Common folder there too but at this point it feels like expendable busywork to me. This is also what I'd suggest as the root for mono test harness scripts - in your case something like src/tests/eng/wasm/android. I'm open to any suggestions regarding organization of the newly emerging src/tests subtree.

[trylek]

Status update - I got traditionally delayed on this; after rebasing my initial changes against the recent addition of F# support I started hitting weird behavior that took me quite some time to track down. I'm trying to put together at least a scoped down version of the cleanup; if I don't manage to make that work by tomorrow, I'll need to postpone this to September as I don't think it would be a good idea to risk destabilizing testing in master at this point.

[ViktorHofer]

Thanks for the update Tomas. We can definitely move this to September, just let us know :)

[trylek]

I think I have finally realized how to approach this; I don't want to just punt it. I was trying to get too much stuff done in a single PR and that's not good in this case. I'm trying to remake the change by making it minimal in the sense that it only does the "disruptive things" - it basically renames src/coreclr/build-test.cmd/sh to src/tests/build.cmd/sh and src/coreclr/tests/runtest.cmd/sh to src/tests/run.cmd/sh. Even though it's somewhat noisy in terms of requiring renames in several YAML scripts, it's a trivial rename that shouldn't be too disruptive and can be easily communicated. The various additional cleanups I originally proposed as part of this rollout are small script cleanups and shuffles most of which don't affect developers' daily life and can be merged in out-of-band according to my opinion, these shouldn't need to wait for the monthly rollouts.

[trylek]

@jashook - one of the interesting issues I hit in my original attempt at the broader cleanup change is that the project you recently added, test_dependencies_fs/test_dependencies.fsproj, actually doesn't build, it fails with the FSC compiler complaining about "No input files". The reason it works is that, when we build both projects in

runtime/src/coreclr/tests/src/runtest.proj

Line 408 in 7d3b804

<MSBuild Projects="$(MSBuildThisFileDirectory)Common\test_dependencies_fs\test_dependencies.fsproj"

MSBuild apparently gets confused and builds the top test_dependencies.csproj project twice. I think you recently mentioned hitting something similar, does this ring a bell?

[jashook]

Ah I believe that adding in runtest was incorrect. We just have no code path in the CI that goes down the code path.

We only need the Fsharp dependencies in Core_Root which is handled in coreclr/tests/build.proj

Let me open a PR, sorry you had to hit that.

[jashook]

Will open the pr early tomorrow to make sure I can still do a innerloop build and test run with the change

[trylek]

@jashook - no worries, please don't haste with the PR otherwise you'll create a conflict for me :-), just wanted to mention some of the issues I hit in the process. Another one was move of the stress_dependencies, external and scripts projects, that took me more than a day to figure out how to fix properly in the props files, but all of this is a terrible mess that doesn't need to be made part of the primary "developer-facing roll-out PR". I have also figured out how to stop restoring these into the source code folders but that's even more complex. Long story short, please focus on reviewing my primary rename PR if possible and let's fix these nits later this week or the next.

[trylek]

Actually, one more related nitpick is that we should probably also add the test_dependencies.fsproj project to the test project exclusions under

runtime/src/coreclr/tests/src/dirs.proj

Line 13 in 7d3b804

<DisabledProjects Include="$(TestRoot)Common\test_dependencies\test_dependencies.csproj" />

I think I'm approaching the phase when we'll be able to proclaim all tests under src/tests/Common as disabled but that can also wait one or two more days.

[jashook]

Sounds good, thank you for this work!