Refactor Linux CI to use pre-built Docker image from GHCR - #21972
Refactor Linux CI to use pre-built Docker image from GHCR#21972kadykov wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Linux CI pipeline to run inside a pre-built Docker image published on GHCR (built from the devcontainer Dockerfile), removing per-run dependency installation and aligning CI + devcontainer on a single build-environment definition.
Changes:
- Add a dedicated build→test→push workflow to build and validate a candidate image before publishing to GHCR.
- Update Linux CI jobs to use
ghcr.io/darktable-org/darktable-build:latestand drop in-job package installation. - Switch the devcontainer configuration and docs to consume the published image; remove the legacy
.ci/Dockerfile; ignore locally-createdinstall/output.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/build-docker.yml |
Introduces a build/test/publish pipeline for the CI image. |
.github/scripts/test-image.sh |
Adds a helper script to run the Linux CI matrix against a candidate image. |
.github/workflows/ci.yml |
Moves Linux CI jobs to a GHCR-hosted container image and removes apt install steps. |
.devcontainer/devcontainer.json |
Switches devcontainer from local build to pulling the published image. |
.devcontainer/README.md |
Updates documentation to reflect the GHCR-based CI image workflow and usage. |
.ci/Dockerfile |
Removes an obsolete legacy Dockerfile. |
.gitignore |
Ignores install/ artifacts created by local image testing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Notes and caveats for reviewersA few things worth knowing before this lands: Splitting environment and code changesWith this approach, a PR that adds a new build dependency to the Dockerfile and code that uses it must be structured as two sequential PRs: first a Dockerfile-only PR (which, when merged, triggers Image freshness — no automatic base-image updates
Either approach is a small follow-up; I'm happy to add a What has been tested
|
|
I have some questions...
|
|
@wpferguson — thanks for taking the time to ask these. They're fair concerns and deserve a thorough answer rather than a quick dismissal. On the two questions about workload and not wanting to touch the DockerfileLet me start here because these are the most important, and because understanding the why changes how the other questions look. The honest answer is: yes, for the small subset of PRs that add a new build dependency, you now need two PRs instead of one. We didn't design it that way out of preference — it's a constraint imposed by GitHub's security model. The obvious alternative is: on every PR that modifies the Dockerfile, build a fresh image, push it to GHCR, and use it in the matrix jobs — no split needed. We implemented exactly this in an earlier draft in 87fc0c8. It failed immediately with: GitHub deliberately prevents pushing to organisation-level container packages during So the real choice is between three options:
The current approach (installing everything fresh on every CI run) is more fragile than it looks. Package updates in the Ubuntu repos could occasionally break builds in ways that are confusing and hard to debug because nobody changed any darktable code. This PR eliminates that class of failure entirely. As for the Dockerfile itself: it contains nothing new or Docker-specific. It's the same flat On dependencies that rely on other dependencies
If you're thinking of git submodules (e.g., rawspeed): those aren't in the Docker image and are handled identically to today. On dependencies that build correctly but are brokenFrom your earlier message in the devcontainer thread I think you're describing a real and frustrating problem: code that compiles and passes CI but has runtime bugs because it wasn't tested by the author on actual images. That's a genuine concern — but it's entirely separate from how the build environment is provisioned. The same test suite runs before and after this PR; we're not adding or removing any tests. If there are specific integration or runtime checks you'd like to add, that would be a great PR of its own. On the concurrent-PR scenarioI want to make sure I understand the scenario correctly. If the concern is: "what if an image update merges while another PR is open, and breaks that PR's CI?" — that's a real possibility, but it's a smaller problem than today. Currently, the environment can silently change between any two CI runs of the same PR (a new apt package version lands in Ubuntu repos) with no record of what changed and no reviewed approval. With this PR, the environment changes only when a Dockerfile change is explicitly reviewed and merged, and the full test suite must pass before the new image is published. What's still openThe one legitimate limitation — split PRs for new dependencies — we can partially address. Two options we're happy to add in a follow-up if the project wants them:
Either of these could be a small, focused follow-up once this lands. Or if you prefer, we can do it in this PR. |
|
@kadykov I'm sorry but I have to say this: If I voiced my concerns here like @wpferguson did, I'd prefer an obviously human answer, not an AI one... Because of how obviously AI your answer is, it feels at least to me more like a quick dismissal rather than an proper answer... |
|
@anoderay, I understand your concern that you don't like the style of the AI messages. Especially for this particular message, I will not use AI. Technically, I still use AI because I use TTS, but I don't use LLM polishing in this particular case. So my current workflow is like this. I have an AI agent that has access to all the source code. Then, using TTS, I dictate my thoughts on the problem. And usually it is quite a lengthy document. For instance, here is the gist that I have prepared in order to answer to the last message. Then I have an AI agent that has access to the all source code, including my recent modifications of the code base. And here is what I actually got from the LLM AI agent. As you see, both of these documents are quite far from what I have actually sent as an answer to @wpferguson. And personally, I would much prefer the LLM-polished answers than the raw output after TTS. The truth is that I am not a native English speaker, and I am not a specialist in all the fields. Indeed, I can write a nice, well-structured answer by myself, but it would take much more longer. That's why I prefer to draft the messages with TTS, and then polish and rewrite them with LLM. If you have any advices about in what style should I use for LLM rewriting, then please tell me, and I will use it for my future messages. And speaking about my previous message, I have read the LLM-polished version, and I agree with all the points. So, if you have any suggestion how we can improve this pull request, please tell me. But in my opinion, freezing the environment in the GitHub container registry would benefit to the stability of CI. |
I wasn't talking about OS stuff, I was talking about darktable stuff. For instance the lua-scripts, an external module, rely on the Lua external module. When we upgrade to Lua 5.5, it will break some of the existing lua-scripts. So, IIUC I would have to
Whereas today I could do this in one PR with 5 commits and all the operating systems covered and nothing broken. |
- Remove .ci/Dockerfile (2016-era Jenkins artefact, superseded) - .devcontainer/devcontainer.json: switch to pre-built GHCR image - .github/workflows/build-docker.yml: implement build→test→push workflow that builds from .devcontainer/Dockerfile, runs all Linux CI matrix configurations via .github/scripts/test-image.sh, and only pushes to GHCR if every build succeeds - .github/workflows/ci.yml: Linux jobs pull ghcr.io/darktable-org/ darktable-build:latest; permissions scoped to contents:read only - .github/scripts/test-image.sh: new helper script that runs all four Linux CI matrix configurations (GNU16 Release, LLVM22 Release, GNU16 Debug, GNU16 Release+tests) against a candidate Docker image; also callable locally for manual validation - .devcontainer/README.md: update CI environment section to describe the build→test→push workflow and correct the GHCR image update cadence - .gitignore: add install/ (created by test-image.sh during local runs)
…docker-build workflow, authenticate in GHCR for pulling images in CI workflow
…patch Add a pull_request trigger on .devcontainer/Dockerfile changes so that a Dockerfile-modifying PR gets a full build-test cycle before merge. The "Tag and push" step is guarded by `github.event_name != 'pull_request'` so nothing is ever published to GHCR during a PR run.
66f6694 to
d1aa1e9
Compare
|
@wpferguson First, you don't need a separate pull request for modifying Windows or macOS dependencies because we change only how we store Linux dependencies. But overall, now I think I understand your concern. So you are worrying about updating dependencies that we cannot submit as a separate pull request because it will break the CI for others, like transition from Lua 5.4 to 5.5. In this case, you don't have other choices rather than submit all the changes in one pull request as you would do it now. Nothing changes on the macOS side and Windows side, but you should expect that all Linux CI checks will be red, because they still use the old image from the GitHub container registry with the old version of Lua. However, because you have modified the Docker file, then the So overall, you will have four failing Linux CI checks and one additional "Build, test and push CI Docker image", like in #21978 that replaces them. And when this workflow is green, you can merge the PR, ignoring that the other four Linux CI checks are still red. And like that, you will update the image on the GitHub container registry and the code at the same time. However, if you just need to add a package to the environment, or you want to do a harmless change to the environment that will not break anything, then you should just first create a separate pull request with only environment changes. In this case, |
|
Why are these AI 'supported' texts always blown up to 10times what's required? |
|
@jenshannoschwalm I believe it is because we train LLMs this way. The longer the output, the more quality response we have at the end. That's why we encourage LLMs to produce some thinking before giving the definitive answer. But it is just my opinion. And speaking about this pull request, do you have any suggestions, or thoughts that are not related to the AI usage? |
|
Follows up on #19597 (devcontainer configuration).
What this changes
Replaces the per-run
apt-get installsteps in the Linux CI jobs with a pre-built Docker image hosted on GHCR. The image is built from.devcontainer/Dockerfile, which was introduced in the prerequisite PR and already mirrors the CI environment exactly.Files changed
.github/workflows/build-docker.yml.github/scripts/test-image.sh, and pushes to GHCR only if every build succeeds.github/scripts/test-image.shdocker run+.ci/ci-script.sh. Also callable locally for manual image validation.github/workflows/ci.ymlghcr.io/darktable-org/darktable-build:latest; remove compiler and dependency install steps; permissions scoped tocontents: readonly.devcontainer/devcontainer.json"build":(local Dockerfile build) to"image":(pull pre-built image — faster for devcontainer users).devcontainer/README.md.ci/Dockerfile.devcontainer/Dockerfile.gitignoreinstall/(created bytest-image.shduring local runs)Why
Single source of truth
.devcontainer/Dockerfileis the only place that defines the Linux build environment.ci.ymland the devcontainer both consume it. No more risk of drift between them.Only tested images are published
The
build-docker.ymlworkflow implements a build→test→push sequence. It verifies every Linux CI matrix configuration against the candidate image before updating:latest. An image that cannot build darktable is never published.Faster CI feedback
The dependency install steps currently add several minutes to every Linux job. Pre-built images eliminate that overhead.
Easier contributor onboarding
devcontainer.jsonnow uses"image":so opening the devcontainer is adocker pullrather than a full local build.What does NOT change
nightly.yml— the AppImage build deliberately targetsubuntu-22.04for glibc compatibility and builds libs from source; it stays as-isCC/CXXenv varsImage update cadence
:latestis updated only when.devcontainer/Dockerfilechanges onmaster(triggeringbuild-docker.ymlautomatically) or when a maintainer triggers a manualworkflow_dispatch(e.g. to pick up upstream Ubuntu security patches). Between those events the image is frozen, which gives full control and reproducibility over the CI environment.Image tagging strategy
Images are pushed as:
ghcr.io/darktable-org/darktable-build:latest— rolling, used by CI and devcontainerghcr.io/darktable-org/darktable-build:YYYY-MM-DD-SHORTSHA— pinned, for auditingNotes
This PR replaces #21909 (same changes, cleaner git history — the earlier PR accumulated merge commits during development; please see that thread for full review context)
test-image.shcan also be used locally to validate a Dockerfile change before submitting a PR: