Use a single pipeline with runtime conditioned jobs in PRs #1473
Conversation
safern
force-pushed
the
SinglePipeline
branch
13 times, most recently
from
de62bfc
to
bc6fe28
Compare
safern
force-pushed
the
SinglePipeline
branch
14 times, most recently
from
c20d9c3
to
105ce20
Compare
Actually from my tests I've seen that running tests against a Checked runtime takes similar time to release, so if we run tests for OSX x64 and Windows x86 it shouldn't increase time significantly. Specially because we build libraries tests once for each OS, so that saves some time. I think it is worth the try, we can always remove them, but is good extra coverage. |
Looks like you are right. In coreclr release/3.1 on a random build, corefx checked takes about 1h 10min: https://dev.azure.com/dnceng/public/_build/results?buildId=483366&view=logs&jobId=dbf66e29-15d1-5c85-f25f-a8bda8f9b70c. |
Right and I believe that's dependent on how long it takes to provision a helix machines, for example in this run for this current PR it took 11 mins. |
|
Looks like you're matrix is missing: There are a lot of CoreCLR Release builds. Are these required for some non-CoreCLR test runs? They aren't used for any CoreCLR test runs, and it seems like we could save CI resources by only doing one or two as Release build spot-checks in PRs unless they are otherwise required. There is no in the table. Is that an intentional pruning of the full matrix? |
Yes these are required to run libraries and installer tests... we want to run tests against a release CoreCLR. We will only use the Checked ones for CoreCLR changes, however as discussed here, I will add a couple that will also happen when there are libraries changes. However, I will see in the test matrix for installer and libraries and see if we can prune some CoreCLR Release builds.
Thanks 😄
Yes, we try to balance out the Debug/Release builds we do by covering in other arches, OSs, I basically just ported over what we've been testing in Libraries from the corefx world. |
Just my 5c. I don't think we should trim builds further. I would even go into the opposite direction and always build everything (still with a good mix of Debug vs Release). With the single pipeline we are already saving costs in comparison to what we had before if you combine coreclr + corefx + core-setup. Running only a specific set of tests (libraries vs coreclr) makes sense as that's the most resource intensive phase. |
We need to be careful with builds that use hardware resources like ARM. These are much more limited than our VM builds. The intent going into this is we would exclude ARM unless CLR changed to ensure they had sufficient resources to make progress on ARM specific changes. |
AFAIK we always want to build on arm but limit testing on it. |
Building on arm doesn't take any arm resources, so it's fine to build for arm, for windows we build on x64 machines but target arm as the architecture. For Linux, we use docker images with a cross rootfs directory that contains all the arm binaries, and build using that as the root file system. |
|
I'm fine with building everything as long as it doesn't cost too many resources we care about, doesn't increase long pole times, and doesn't cause resource exhaustion when many PR's happen simultaneously. With one caveat: the more jobs we have, the more likely it seems we will have a "random" failure in one or more of the jobs. With so many moving parts in this system, this seems inevitable. This because super frustrating for users. |
|
Yeah I totally agree and I tried to keep the matrix as minimal as possible, but building release coreclr is required to run libraries and installer tests. I think I came up with the minimal set of the matrix. We could evaluate, but something that we could do is add more conditions and for example if coreclr is the only partition touched, only run libraries and installer tests on a checked runtime and that would avoid building release coreclr, but I thought that was more prone to errors and missing the right coverage that we've been having for years already. |
safern
force-pushed
the
SinglePipeline
branch
from
f4e0047
to
295de41
Compare
|
CoreCLR failure is: https://github.com/dotnet/coreclr/issues/26241 I added a commit to disable the Libraries test on a checked runtime: db5e58b Merging as everything else was green. |
|
Thanks @safern , that looks great. The only question is about "CoreCLR Pri0 Test Run Windows NT arm64 Checked job", I believe we do not run tests on arm64 windows (do not have enough machines). Historically that job existed only to build tests (because there was no separation between "Build tests" and "Run tests"), but now that probably should be deleted to avoid confusion. |
|
CoreCLR failure should be resolved by #1794! |
|
I believe we'll need a bit to iterate on the ideal platform combo. @jkotas is specifically asking for runs against checked builds to catch assert failures and I believe he's got a very valid point. I continue working with dnceng and DDFUN to make them provide us with reasonable HW. |
Right, it currently doesn't run any tests as it is disabled, same for arm (which @trylek is working on). I'll put up a PR to remove it from the test runs to not confuse people as it will basically just be a no-op. |
ghost
locked as resolved and limited conversation to collaborators
The GVM analysis within the compiler assumes we can do analysis on top of canonical forms and we get analysis holes if the canonical form doesn't exist at compile time. GVM dispatch at runtime might end up building new types as well, so the extra template isn't strictly just bloat but I do admit there's some laziness in just throwing the extra template in to fix the problem. This wasn't hittable outside reflection-free mode because we generate canonical forms of everything outside reflection-free mode anyway. As I was looking for a place to put this in, I realized the `TypeNeedsGVMTableEntries` logic needs to run on canonical types as well (those are not `ConstructedEETypeNode`), so I moved it to a common spot. That part was not necessary to fix the reported problem, but it provides a convenient spot to put the canonical type dependency. Fixes dotnet#1473.
This PR deletes all the individual per-partition builds and adds all the builds to
runtime.ymland conditions tests and builds based on paths changed in a PR.The result of this PR is as follows:
CoreCLR Builds
Libraries Builds
Installer Build and Test
CoreCLR Test Builds
Libraries Test Build
CoreCLR Test Runs
Libraries Test Runs (Release CoreCLR)
Libraries Test Run (Checked CoreCLR)
cc: @jkotas @stephentoub @danmosemsft @BruceForstall