[CI] Add GitHub Actions workflow to check stdlib/Manifest.toml #56976
Conversation
|
Any particular reason for doing this in GitHub Actions (instead of Buildkite)? |
|
Two:
|
|
I think I'd prefer that we put this in Buildkite. If it lives on GitHub Actions, then it's going to create a new commit status - there's no way to prevent that with GHA. In the past, we've gotten feedback from committers that having too many commit statuses makes things noisier and harder to read. If we put this job into Buildkite, then instead of creating a new commit status, we just group it under the existing |
By "hosted runners" I assume you mean the machines that the JuliaLang project hosts (as opposed to GitHub-hosted runners). Ultimately, a PR to be merged, it needs normal CI to run, which requires JuliaLang-hosted machines. So I'm not sure if we end up getting any benefit by having some jobs not require JuliaLang-hosted machines. |
The benefit is reducing the queue, especially for such short lived jobs, see for example #55281: the check whitespace job on GHA is 6x faster just because BK has a relatively large latency for short jobs. |
In case it wasn't obvious from the code, this job is run only on PRs which modifies files in the stdlib dir: julia/.github/workflows/CheckStdlibManifest.yml Lines 4 to 6 in f0e768f stdlib/*/Project.toml and stdlib/*.version.
Also, if BumpStdlibs.jl also updated the manifest that'd further reduce the need to manually update it (or even better, the manifest wasn't needed in the first place). |
|
What's the actual time being saved here? Is it e.g. 30 seconds vs 60 seconds? Does the amount of time being saved outweigh the cost of adding more noise (by adding another commit status) - and thus the increased risk of a committer merging a PR with failing CI (because they get overwhelmed by all of the commit statuses)? |
FWIW, I suspect that the current issue is not with our specific JuliaLang-hosted machines, but rather a Buildkite service degradation. GitHub Actions frequently experiences service degradations, so this point doesn't really go to an advantage for GitHub Actions over Buildkite. |
This likely won't be too difficult to implement. |
Plenty of PRs touch If the job only runs on the project and |
Looks to me like BumpStdlibs is a major offender, and last 34 commits (halve that number without BumpStdlibs commits) go back to 1.5 months ago. |
There was a problem hiding this comment.
With the triggering paths being restricted further, this probably will have a more limited blast radius. I still would probably lean towards putting it in Buildkite, but given that this job is triggered in so few circumstances, it might not end up making too much difference either way.
We can move this to Buildkite later if we want.
|
Should we hold off merging this until BumpStdlibs is updated to update the Manifest? |
Co-authored-by: Mosè Giordano <765740+giordano@users.noreply.github.com>
| pull_request: | ||
| paths: | ||
| - 'stdlib/*.version' | ||
| - 'stdlib/*/Project.toml' |
There was a problem hiding this comment.
Presumably we want to also trigger this if stdlib/Project.toml is changed:
| - 'stdlib/*/Project.toml' | |
| - 'stdlib/*/Project.toml' | |
| - 'stdlib/Project.toml' |
Fix JuliaCI/julia-buildkite#417.
I first tried with a workflow to automatically open a PR to update the manifest, via workflow dispatch and/or schedule, but it's complicated to do it with limited permissions:
peter-evans/create-pull-requestallows creating PRs from forks, but that's finicky because it requires a PAT which can push to the fork and for the workflow dispatch we'd need to keep the list of committers in sync across different organisations (GitHub doesn't allow creating forks within the same org, even with different name), and also schedule would be useless because for a seldom used repo that'd go stale very quickly.Adding a check in PRs is probably the easiest option with least permissions required. This needs #56972 to be merged first.