Introduce gitflow-incremental-builder #13243
Conversation
ghost
added
the
area/infra-automation
.github/workflows/ci-actions.yml
Outdated
| @@ -95,6 +95,30 @@ jobs: | |||
| - name: Delete Local Artifacts From Cache | |||
| shell: bash | |||
| run: rm -r ~/.m2/repository/io/quarkus | |||
| - name: Get GIB arguments | |||
There was a problem hiding this comment.
I was looking long and hard for a possibility to have some kind of conditional logic in the env section but it seems this is not supported by GH Workflows.
That's why I ended up with this build step that creates an output that is then exposed as a job output to subsequent jobs.
I could have created a spearate job for this but then I would need to extend needs of each job.
.github/workflows/ci-actions.yml
Outdated
| fetch-depth: 0 | ||
| - name: Add quarkusio remote | ||
| shell: bash | ||
| run: git remote add quarkusio https://github.com/quarkusio/quarkus.git |
There was a problem hiding this comment.
Notes:
- this entire block is now present for almost each job
- I decided to only add the remote, not directly fetch from it, so that GIB can decide whether or not to fetch
masterat all (see the "Get GIB arguments" step further up for the cases where GIB disables itself)
.github/workflows/ci-actions.yml
Outdated
| @@ -154,7 +183,8 @@ jobs: | |||
| shell: bash | |||
| run: tar -xzf maven-repo.tgz -C ~ | |||
| - name: Build with Maven | |||
| run: eval mvn $JVM_TEST_MAVEN_OPTS install -pl !integration-tests/gradle -pl !integration-tests/maven -pl !integration-tests/devtools ${{ matrix.java.maven_args}} | |||
There was a problem hiding this comment.
I had some nasty formatting/escaping problems with eval, especially with -Dgib.disableIfBranchRegex='master|\d+\.\d+|.*backport.*' (see further up).
AFAICT eval ist not required so I simply removed it from every mvn call.
.github/workflows/ci-actions.yml
Outdated
| do modules+=("integration-tests/$i"); done | ||
| IFS=, | ||
| eval mvn -pl "${modules[*]}" $NATIVE_TEST_MAVEN_OPTS | ||
| do modules+="integration-tests/$i,"; done |
There was a problem hiding this comment.
Additionally to eval (see futher up) I also had issues with IFS=,.
Since Maven doesn't mind a trailing ,, I simplified it all a bit.
|
It just occured to me that if we use this locally, we will most likely run into a problem similar to this: #13218 (comment) Of course this PR here is not about parallel builds, but incremental builds are also affected because in case an extension changes (or is added) in a way that is relevant for the platform descriptor generation, an incremetal build will not include In CI we are safe since we have this initial single threaded build that will produce a consistent platform descriptor. PS: I might be able to work around this via/in GIB but then again we also have local builds without GIB, so we need a general solution. |
|
The descriptor in fact does depend on the presence of the extensions it includes. So we could introduce dependencies at least on the POMs of all the extensions. |
Yeah, that would be the hideously verbose but standard Maven solution. I'd like to continue this discussion in #13218. |
|
Rebased & updated:
|
|
I'll finish this after #13218 was merged. |
|
Thanks @famod |
|
@ebullient do you have a clue what's going on? Thanks! |
Hmm. I don't know off the top of my head. The other micrometer tests are ok.. it's just that one? |
The following tests are ok:
But in the module |
It was the other buckets I meant (they all set up the same registries, more or less. The JMX test does not use Resteasy, but the MP Metrics test does. The projects and tests between MP Metrics and Prometheus are pretty similar in structure, so I don't understand why just the Prometheus test would fail.. and I have no idea why one ServerSerialiser would be selected over the other ... (or what the incremental build touches that would be relevant here, as it clearly all passes in the full builds.. ) |
|
https://github.com/vackosar/gitflow-incremental-builder#gibbuildall works around the problem but is not very efficient for 700+ modules. Still digging... |
|
I found a much better alternative to What I don't get is why is a rebuild of this module even needed? The module is already built in the "Initial JDK 11 Build" and is shared via the "persisted" Maven repo. @geoand @FroMage any idea what's going on? I'm talking about #13243 (comment) and subsequent comments. |
|
I've created https://github.com/quarkusio/quarkus/tree/gib-test that is this branch but in quarkusio ..now just need to find a PR to try with ;) |
I don't see why this would cause a problem :(. |
|
@n1hility I'd love to hear your opinion about this! 🙂 |
|
@famod this needs a rebase as I merged your other PR (I think that's the one that created the conflict) So... my take on this is that we should have some shadowing of the current CI for ~ a week to be able to see what's going on. So, I would probably duplicate the workflow first, have it run for a week and then decide depending on the result of it. We will pay a CI price for a week but if GIB works great, it should be sustainable. We don't update the ci-actions workflow that often now that the native test list is in a JSON file so I think it should be doable without too much pain. @famod WDYT about this strategy? |
|
Thanks for your feedback, @gsmet!
So the idea is to have a copy of the pre-GIB state of In the beginning, I actually aimed at making just a few jobs incremental as a start, but with that approach you wouldn't have a full side-by-side comparison. IIRC, @ebullient mentioned that in a past project they had a permanent weekly (?) full build to make people more confident with the incremental approach. This might also be something to consider (additionally). But before we merge this I have to complete the remaining tasks. The one task with the new generated dependencies of A simplistic solution would be to take The alternative to this would be a feature in GIB that can remove certain dependency subpaths from the reactor when certain conditions are met. Doesn't seem easy to implement, especially given the API Maven provides for this. Next best (but slightly weird) solution might be to disable GIB in case certain modules are changed. This is something CI would have to figure out. And of course you would end up with a full build. /cc @aloubyansky What's your take on this? PS: Sorry for the long text but this must be done right. CI/build consistency over efficiency. |
|
I just had another idea reagarding the descriptor dependency graph problem: If we move all those generated extension dependencies to a profile (that is active by default), we could disable it for incremental builds. So changes to the descriptor, its plugin or any of the plugin's upstream dependencies would trigger all ITs, but not a change to a single extension module! /cc @aloubyansky ^ (for now this feels like the best solution but maybe I'm missing something) PS: Just as a recap: The initial JDK build will always install a consistent descriptor artifact and the repo is shared with the subsequent jobs. That's why you could skip the descriptor module entirely in the IT jobs etc. |
I would better create yours as a new one for now. And, yes, we could enable only one JDK for the incremental build. As for CI resources, we will see, it will for sure slow things down but I'm not comfortable merging this without having some sort of shadowing for a while.
I agree. As for the rest, I will let you discuss it with @aloubyansky :). |
ghost
added
the
area/devtools
|
I've rebased and added another commit that shows my idea with the depdencies profile in |
That is true. |
|
I think I will use the There might also be a problem with So, it doesn't get boring! 😉 But better find the problems now than later. |
Confirmed, as expected. I'll add The other adjustments are being tested at the moment in another branch of my fork. "Shadow CI" is still left to do (as is a first iteration to |
|
I kicked off a run of the "Shadow CI" in my fork (including GIB 3.12.2 that I've just released). |
|
All TODOs are done! 🎉 (AFAICS) /cc @aloubyansky @gsmet |
|
EPIC! |
|
FTR, |
|
Websocket extension renaming got in the way, merge conflict resolved. |
|
It's sad that stef wasn't found, but this has nothing to do with GIB 🤓 |
|
|
Relates to #11622 Note: master, release branches (e.g. 1.10) and backport-branches are not built incrementally! Includes some efficiency teaks to bom-descriptor-json and a "shadow CI" for a temporary parallel testing phase.
|
OK, let's merge this and see how it goes for a week! Thanks a lot for your continued work on this, @famod ! Hopefully, it will drastically help with our CI feedback loop! |
Relates to #11622
Fixes #6260
Note: master, release branches (e.g. 1.10) and backport-branches
(without PR)are not built incrementally (yet)!Some general remarks:
quarkusio/quarkus masterand not the fork'smasteras the reference because no one on Zulip had a (strong) opinion about this and since some (most?) devs don't seem to push to their fork'smasterat all, fetchingquarkusio/quarkus masterseemed like the best compromisefetch-depth: 0is required to have a fully usable history and the reference branch available (in case the reference is notquarkusio/quarkus master, which is fetched explicitely)I'll also add some review comments myself.
Edit: List of TODOs for a better overview (updated on demand):
ServerSerialisersfailure inmicrometer-prometheusintegration testsgibtoincrementalor similarCONTRIBUTING.md(as discussed on the mailing list)devtools/bom-descriptor-jsonthat can be disabled in CI to avoid executing all ITs if e.g. only a single extension was changedindependent-projectsare run in case.github/**or.mvn/**is changedIdea for testing this closer to reality:
Someone with write access to
quarkusio/quarkuscould create a branch named e.g.gib-testinquarkusio/quarkusfrom this PR branch and could then branch offgib-testin his/her fork (!) and create a test PR.