Skip to content
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

CI: cache syntest docker image #16

Merged
merged 12 commits into from
Nov 1, 2022
Merged

Conversation

victor-gp
Copy link
Owner

Motivation: syntax tests in CI took 3-5 minutes, because it recompiled syntest. (The Rust compiler is well known to be slow).

By caching syntest, tests take < 20s, like the 2 regression tests. Which is nice because it's a "timeout" you can wait for, instead of going to do other things. It allows you to push & receive feedback in the same instance. Nice.

This was quite tricky to get right because I didn't RTFM in one sitting. Also there were two relevant manuals or more: build-push-action, GitHub Actions cache, partly buildkit and its buildctl... The whole ordeal is documented in the commit messages.

Btw, the GitHub Actions cache is an experimental option in build-push-action. Works for me tho. 🤷

Just troubleshooting, don't mind me.

Buildkit is disabled in Docker, by default. You have to enable it with
that env.

Because the cache is associated to buildkit, of course it should be
enabled in the docker build of syntest.
Try with this install parameter to setup-buildx, that should achieve the
same as DOCKER_BUILDKIT=1 and is referenced in the documentation for
that action:

https://github.com/docker/setup-buildx-action/blob/master/docs/advanced/install-default.md

Also comments the scope param to build-push-action because the runner
was complaining about it.
So syntax.py has an escape hatch to not build the image if it already
exists. (Which is probably bad because it won't be rebuilt on syntect
version changes but whatever.)

By tagging the image with the name syntax.py expects, we can probably
avoid the rebuild?
By setting an explicit image tag, with version (latest) and all, on both
the python script and build-push-action.
After reviewing [^1].

Because it seems that only buildctl can import the cache, so let's use
that command.

[^1] https://github.com/moby/buildkit/tree/master#github-actions-cache-experimental
Instead of performing acrobatics / architecture astronautics to have the
original script (syntax.py) fit both use cases (feedback loop in the
dev's local + automated tests in CI).

It's probably the right way to use build-push-action. Build, then run,
instead of re-building again in a later step.

I tried placing `syntax_ci.sh` in `.github/workflows/` but then it's not
visible to the Github Actions runtime.
buildkit seems to be built for versatility, agnostic of the underlying
engine / runtime / client / whatever Docker is supposed to be.

So you need to explicitly tell it where you want the output of the build
to be saved.

load = true "is a shorthand for --output=type=docker"
The previous commit worked! The syntest image is properly generated and
registered with `docker images`!

But the job still fails because we need the part of syntax.py where it
downloads the man syntax. Some syntax tests depend on that syntax being
available in syntaxes/.

Now, as the syntest image is already built when we hit syntax.py, it
skips rebuilding the image. Which is par for the course: caching, based
on docker/build-push-action.

ETA: indeed, this version passes 🎉 🚀🚀
Remove failed/unneeded experiments from all the troubleshooting.

The `with: install: true` in docker/setup-buildx-action is not needed
because it aliases `docker build` to `docker buildx` which we don't use
(not explicitly).
So the Github Actions cache prevents sharing a cache across branches,
with some exceptions:

Only the cache for the current branch, the base branch and the default
branch is accessible by a workflow.

I believe that by setting the cache scope to "main", all workflows,
including those that run on main (default & usually base branch) will
use the same scope. And such a scope should be accessible to every
workflow because it's the default branch's also.

ETA: yup, this works. After the first workflow run for the main branch,
every other branch (in a PR) can use its cached syntest image.

This fucking works!! 🚀🚀🚀

References:

https://github.com/docker/buildx/blob/eab3f704f53577310c2d05700ab663f8a7ad4c8e/docs/guides/cache/gha.md#scope
https://github.com/docker/buildx/blob/eab3f704f53577310c2d05700ab663f8a7ad4c8e/docs/guides/cache/gha.md#synopsis
https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#matching-a-cache-key
Moved fast and broke things...
@victor-gp victor-gp merged commit 50d2313 into main Nov 1, 2022
@victor-gp victor-gp deleted the ci-cache-syntest-docker-image branch November 1, 2022 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant