-
Notifications
You must be signed in to change notification settings - Fork 461
ci: avoid unnecessary builds and test runs #1866
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
3393554 to
9b6e6a7
Compare
mjcheetham
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No doubt this works, but I'm very concerned about the fragility of it all, and how verbose and complicated some of the repeated expressions and inline GitHub scripts are.
Can we not do this in any other, simpler, way? Even if not optimal, but just some modest improvement. Would rather have the have the workflow/build change than workaround Actions limitations.
.github/workflows/build.yaml
Outdated
| skip: ${{ steps.check.outputs.result }} | ||
|
|
||
| steps: | ||
| - name: look for prior successful runs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- - name: look for prior successful runs
+ - name: Look for prior successful runsGit simply has so strong an opinion about line endings that it is not worth fighting. Git will passive-aggressively color the Carriage Return in the diff output until you give up, and that's what I'm doing here. Der Klügere gibt nach. Technically, this is far from the only file that has this issue. But I do not want to interfere with other ongoing work in the CI area, so I'll refrain from touching files other than the one I am about to modify. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Yes, yes, CodeQL. It is totally _possible_ that someone with admin privileges will change the default back to the unsafe `write` permission. Even if unlikely, let's make it explicit that the `build` workflow only requires `read` permission. Even in a PR that is only modifying that workflow, and whose purpose is a totally different one. Let's conflate separations of concerns. Whatever. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Most of the failures we encounter in VFSforGit's CI runs are flakes. In those instances, it is not helpful but rather annoying when long-running jobs are canceled after more than half an hour when they could have run to completion and succeeded instead. Because when those runs fail, the first thing I do is to hit "Re-run" to see whether it _is_ a flake. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
GitHub Actions sadly lacks the feature where you can reuse prior runs that were successful on the same `head_sha`. Similar to what I did in git/git@99fe06cbfd6 (ci: avoid building from the same commit in parallel, 2023-08-23), let's simply skip the CI run if the same commit has been tested successfully before. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
As far as VFSforGit's CI is concerned, there is no difference between commits when their _files_ have not changed (it's not like the tip commit's SHA is persisted in some version number or some such). Therefore, if there is already a successful run for another commit whose top-level tree is identical to the one that is about to be tested, skip the current run in favor of that one. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
To increase confidence in Pull Requests, a couple of Checks are required, for example the PR build must pass. However, due to one of GitHub Actions' limitations that seem to stay with us forever, it is impossible to require a specific _workflow_ to succeed for a PR before it can be merged. You have to specify workflow _jobs_ that are required. Therefore, the changes I just made to skip runs when there already exists a successful run for the same commit (or at least a tree-same one), would eternally prevent affected PRs from being merged. While the _workflow_ succeeds in such instances, the (required) workflow _jobs_ would be skipped (and therefore the required checks would not be fulfilled). Let's bite the bullet and patch through the information whether to skip re-running the tests _to the individual jobs_, so that they do run, and the do get marked as successful, and the Pull Requests can proceed. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is always nice when one is given a good reason for something. For example, when a CI run is skipped, it is nice to not only to know that there _has_ been a previous successful run, but also _which one_. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The Functional Test jobs takes long enough (35 minutes for x86_64, 1 hour 15 minutes for ARM64) that it is worth waiting for them to finish before kicking off a matrix build that tests the exact same code. So let's do that, wait for any in-progress run that might result in some saved time. Granted, this spends cycles of GitHub runners waiting idly, but GitHub does not provide for a better way. Note: The `concurrency` attribute _might_ look as helpful in this context at first sight. But is comically limited: It can only let a _single_ job wait in a queue, subsequent jobs get canceled (and if you have notifications turned on, in a loud manner!). And even letting the job wait would not help here because we also want to handle _tree-same_ commits, not only identical commits, and GitHub does not provide for a way to identify them in GitHub workflow expressions, either. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
There might be runs for the very same commit, those should be looked at first. Then there might be runs that have not yet finished; Let's first look at the finished ones. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
9b6e6a7 to
6575572
Compare
Range-diff
|
In `workflow_dispatch`-triggered runs, the user can specify a Git version to use for testing, which means that something different is actually tested than in regular, non-`workflow_dispatch` runs. So let's neither skip the build/test when triggered thusly, nor reuse any results from thusly-triggered runs. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
mjcheetham
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! This should really help improve run times of the build+test workflow.
In #1866 I introduced logic to skip builds and tests when a previous workflow run already succeeded for the same commit. However, the original revision of that Pull Request tried something _even more_ elaborate, and when I dropped that elaborate logic (because it is a bit fragile), I made a mistake in the replacement logic. The `run.status` can never be `success`, it can be `completed` and _`run.conclusion`_ can be `success`. So let's test for that instead ;-) Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In #1866 I introduced logic to skip builds and tests when a previous workflow run already succeeded for the same commit. However, the original revision of that Pull Request tried something _even more_ elaborate, and when I dropped that elaborate logic (because it is a bit fragile), I made a mistake in the replacement logic. The `run.status` can never be `success`, it can be `completed` and _`run.conclusion`_ can be `success`. So let's test for that instead ;-) Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Sadly, VFSforGit's CI and PR builds are not exactly cheap. Not only in terms of combined runtime (where VFSforGit's 3 hours 45 minutes is not even close runner-up to
git/git's 8.5 hours (!!!). A Functional Test job typically runs for 35 minutes to test on x86_64, and an even worse 1 hour and 15 minutes on arm64. The wall-clock time for a CI or PR build of VFSforGit therefore weighs around 1 hour and 20 minutes.This hurts a lot when trying to fast-track a change from a PR targeting
masterto a full VFSforGit release because not only does the entire build run again on the (tree-same)masterafter merging, the next step (thanks to the original VFSforGit maintainer's decision to go for a Git Flow-like setup) requires the build to run a third-time on the (also tree-same) PR merge commit.Let's try to reuse build and test results from previous, successful runs on what is essentially the same code.