Build coreclr targetGeneric tests separately for CI

[sdmaclea]

Fixes #34508

This PR change splits the building of targetGeneric and targetSpecific tests in ci.yml

• Add testBuildPhase parameter to build-test-job
  • Modify names of artifacts and logs for targetGeneric build
• Add testBuildPhased parameter to run-test job
  • Add DependsOn as needed
  • Add additional test download as needed
• Modifies CI.yml to call build-test-job twice. Once withtestBuildPhase:targetGeneric and once with testBuildPhase:targetSpecific
• Modifies CI.yml to call run-test-job with testBuildPhased:true

[ghost]

Tagging @ViktorHofer as an area owner

[jashook]

Will be able to take a look tomorrow morning.

[sdmaclea]

I am waiting on a review on this before investing more in this.

• The current implementation requires manually enabling this at every call/instantiation of build-test-job and run-test-job
• I have not touched all the call/instantiation points.
• It appears I have not touched the most important one as I don't see the separation on the runs in this PR.

Is this the correct approach?
Is there a better approach?
How should I test?

[jkoritzinsky]

To test this in CI, you need to change the runtime.yml pipeline to use it. The ci.yml pipeline you changed is the CoreCLR outerloop job, so it doesn't run in PR. (It should definitely also have this change though, so don't undo it.)

[sdmaclea]

The Windows tests runs are failing because they are trying to download CoreCLRManagedTestArtifacts_AnyOS_AnyCPU_checked_LibDebug.zip, but the artifact is actually saved from OSX as CoreCLRManagedTestArtifacts_AnyOS_AnyCPU_checked_LibDebug.tar.gz.

[trylek]

Hmm, that is annoying. I recall that @jashook mentioned some limitations regarding what compression tools are available on the various build machines; I guess we need to figure out whether it's easier to use zip on OSX or tar.gz on Windows.

[sdmaclea]

Looks like I have some control, if I chose to not use the default.

          tarCompression: $(tarCompression)
          archiveExtension: $(archiveExtension)

[sdmaclea]

Tests are now running.
There are 45 test failures on this run.

A lot of them are COM Interop tests. I would guess they should be marked TargetSpecific, because they are only supported on Windows. I'll take a look today. /cc @jkoritzinsky

There were also Libraries test run failures (some on one Mono leg, and some on one CoreCLR leg). It is not clear to me whether these are related failures or not. I will assume not until I clean up the CoreCLR test legs.

[sdmaclea]

The PR was clean last night. So this change is now ready for review.
At the moment there are merge conflicts, so I will rebase to fix the merge conflicts.

edit...
Looks like I introduced issues with the rebase. Working on fixing them...

[trylek]

Thanks Steve, this looks like a solid step forward but I believe a few things are still missing:

1. At the bottom of your "PR conversation" I see that your runtime pipeline didn't even start. I believe this is due to incorrect handling of testPriority in the two templates (build-test-job, run-test-job) as I described in another review comment.

2. I suspect we're missing an execution of build-test in run-test-job to build the target-specific tests in phased mode.

3. I cannot find the counterpart changes to the src/coreclr/build-test.cmd / sh scripts implementing the new targetSpecific and targetGeneric switches. I'm not seeing any such logic in the scripts as they stand today.

As a side note, Vlad mentioned in your yesterday meeting that he was hitting some issues with the item-based test traits and mentioned that we might need to rethink this bit and instead use project properties to declare them. Once you manage to fix the issues per my bullets 1-3, be prepared that you may hit difficulties in the test split because of that.

Thanks

Tomas

[sdmaclea]

@trylek Thanks for the review.

As a side note, Vlad mentioned in your yesterday meeting that he was hitting some issues with the item-based test traits and mentioned that we might need to rethink this bit and instead use project properties to declare them. Once you manage to fix the issues per my bullets 1-3, be prepared that you may hit difficulties in the test split because of that

#34658 Added properties because that was how existing p0/p1 filtering was being done.

[sdmaclea]

I guess. I need to change code base on the Mono file relocation too...

[sdmaclea]

The YAML is parsing again. I would expect the test run to pass.

Do we need to trigger a build somewhere else to the the changes in ci.yml?

[trylek]

https://dev.azure.com/dnceng/public/_build?definitionId=655&_a=summary

runtime-coreclr outerloop

[sdmaclea]

Innerloop looks good to me minus spurious failures. I don't have permissions at the moment to re-run failed jobs due to dotnet/core-eng#9655
Outerloop passed.
https://dev.azure.com/dnceng/public/_build/results?buildId=612835

Edit -- retriggered failed innerloop jobs....

[trylek]

Ship it!

[trylek]

And if for whatever reason we haven't managed to scare you off further infra work yet, I think the following items are up for grabs:

1. Do we really need the pair "CLRTestNeedsTarget" / "OsSpecific" everywhere? Perhaps one of them (probably the former) might suffice.

2. Get rid of managedBuildOsGroup/Subgroup as it should be superfluous with this change.

3. Monitor perf implications of this change and decide whether we want to move OsSpecific test build to run-test-job and revert the pipeline changes you've made or whether we want to keep the separate set of build jobs for the OS specific tests in which case we should ultimately update all our existing pipelines.

Thanks for your great work, tenacity, and patience!

Tomas

[sdmaclea]

Do we really need the pair "CLRTestNeedsTarget" / "OsSpecific" everywhere? Perhaps one of them (probably the former) might suffice.

I added CLRTestNeedsTarget everywhere to allow this work to be implemented while @VSadov figured out generic filtering. I knew how to filter properties, but not filter on TraitTag items. I have been assuming @VSadov will remove these when he implements generic filtering.

Get rid of managedBuildOsGroup/Subgroup as it should be superfluous with this change.

I can take this on.

Monitor perf implications of this change and decide whether we want to move OsSpecific test build to run-test-job and revert the pipeline changes you've made or whether we want to keep the separate set of build jobs for the OS specific tests in which case we should ultimately update all our existing pipelines.

I don't really know how to do this monitoring. If someone monitors it, I'd be happy to refactor this.

I was also looking at #33128 & #33067

[trylek]

All your responses sound great to me, thank you!

[safern]

@sdmaclea do we need to build libraries in Release here? We already build it on Debug: https://github.com/dotnet/runtime/blob/master/eng/pipelines/runtime.yml#L382

Do we need the release build?

Also, this will introduce a duplicate job of the libraries on rolling builds because there's already a libraries job for Release in OSX, because https://github.com/dotnet/runtime/blob/master/eng/pipelines/runtime.yml#L382 will be built on release. See, variables.debugOnPrReleaseOnRolling.

@trylek this might be the reason of the break

[trylek]

I agree this is definitely at least part of the problem, thanks for pointing that out. I'm investigating whether it's the only one.

[sdmaclea]

The duplicate job was prevented by the conditional logic below

      condition: >-
        and(
          ne(variables['debugOnPrReleaseOnRolling'], 'Release'),
          or(
            eq(dependencies.checkout.outputs['SetPathVars_coreclr.containsChange'], true),
            eq(variables['isFullMatrix'], true)))