Repository navigation
docs: an unpublished upstream tag now fails the local build with a clear message - #553
Conversation
The ci-tooling skill and renovate.json both stated that every merge in the freedesktop-sdk-docker-images repository publishes a tag for its commit. That is not guaranteed: a failed upstream build leaves the tag unpublished, so the bst2 bump in projectbluefin#548 resolved to a tag that did not exist and made the local build fail with an opaque "manifest unknown". The wording now says the tag exists only after that repository's build pipeline has published it. The `just bst` target also checks the image before pulling, so an unpublished tag fails fast with a clear message instead of deep inside a build. Hive-Run: projectbluefin#549 Hive-Plan: soften-bst2-publish-claim Hive-Spec: ci-tooling#bst2-unpublished-tag Assisted-by: lemonade/ornith-1.5-35b-a3b-gguf-q6_k via GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: kylerankin <kylerankin@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Needs approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Summary: This change adds a check so the local build stops early with a readable message when the base image's version tag was never published. As written, though, the check itself crashes before it ever runs, so every local build — and the project's own "validate" job in CI — now fails on the first line, even when the image is fine. The wording fixes to the documentation are good; the build script needs a one-character fix.
What I need from you: Apply the suggested fix below and re-push.
correctness
just bstis broken unconditionally (Justfile:46,Justfile:47,Justfile:48). The new lines use"${{bst2_image}}".justexpands{{bst2_image}}to the literal value, so bash sees"$registry.gitlab.com/freedesktop-sdk/.../bst2:0fad…", and underset -uthat aborts withregistry: unbound variablebeforepodman image existsruns. Reproduced withjust --dry-run, and the PR's ownvalidatecheck fails with exactly this:bst: line 46: registry: unbound variable(job 122460243391). The existingpodman runline (Justfile:60) already shows the correct idiom,"{{bst2_image}}". Fix:
if ! {{sudo_cmd}} podman image exists "{{bst2_image}}" >/dev/null 2>&1; then
if ! {{sudo_cmd}} podman pull --quiet "{{bst2_image}}" 2>"${HOME}/.cache/bst2-pull.err"; then
echo "ERROR: could not pull bst2 image '{{bst2_image}}'." >&2
The PR body's "Verification" says just --summary parses — it does, but that only checks syntax; it never executes the recipe, which is why this slipped through.
style
Justfile:47writes the pull stderr to a fixed path${HOME}/.cache/bst2-pull.errand only removes it on success. Amktempwith atrap … EXIT, or simply2>&1 | sed 's/^/ /'capturing into a variable, avoids leaving a stale file behind on failure and avoids two concurrentjust bstruns clobbering each other's error output. Non-blocking.
No findings from: security, intent-alignment, docs-currency.
Confidence: 3/5 (needs attention) — 1 high finding
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
91e6c07
|
jeefy
left a comment
There was a problem hiding this comment.
Requesting changes: the new pull check in the bst recipe breaks every just bst call, which is why validate is red on this PR.
The bug: "${{bst2_image}}" is just interpolation ({{bst2_image}}) with a literal $ in front of it. The recipe runs as bash with set -u, so it becomes "$registry.gitlab.com/…" and fails before anything is pulled:
/run/user/1001/just/just-8xSLOW/bst: line 46: registry: unbound variable
error: recipe `bst` failed with exit code 1
(job 122460243391). Use "{{bst2_image}}", as the podman run line below already does, or "${bst2_image}", since the variable is exported.
Also:
- The description is wrong about #548. It says the bst2 bump to
e9149d5was "merged yesterday", but #548 is still open. Itsvalidatefails on exactly this missing tag, which is howmainstayed unaffected. - Watch the
HOMEcache directory.podman pullwrites its error to${HOME}/.cache/bst2-pull.err. The recipe already creates${HOME}/.cache/buildstream, so${HOME}/.cachewill exist in practice, but amktempfile would avoid the stray path.
The docs and renovate.json wording changes look right to me.
|
Thanks @kylerankin, the docs and Renovate wording look right. The new Justfile guard has a bug, and it's why just expands (or |
The pull guard used "${{bst2_image}}", which just text-substitutes the
default value (registry.gitlab.com/...) so bash saw
"${registry.gitlab.com/...}" and failed with 'registry: unbound variable'
under 'set -u'. bst2_image is already exported, so reference the shell env
var "${bst2_image}" in all three spots. The bare "{{bst2_image}}" run arg
is left as-is (correct: it becomes the literal image ref).
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Needs approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Summary: This change makes the local build stop early with a readable explanation when the upstream base image it needs has not been published yet, and corrects the written instructions that wrongly promised the image always exists. It does what the linked issue asked. Two small loose ends in the new check are worth tidying, but neither blocks it.
correctness
- low —
Justfile:47-53: the pull's error output is written to${HOME}/.cache/bst2-pull.err, but on the failure path the scriptexit 1s at line 53 before therm -fat line 55, so the file is left behind after every failed pull (and only ever cleaned up on a later successful one). Capturing stderr in a variable avoids the file entirely and also drops the stray image-ID linepodman pull --quietprints to stdout on success:
if ! {{sudo_cmd}} podman image exists "${bst2_image}" >/dev/null 2>&1; then
if ! pull_err="$({{sudo_cmd}} podman pull --quiet "${bst2_image}" 2>&1 >/dev/null)"; then
echo "ERROR: could not pull bst2 image '${bst2_image}'." >&2
printf '%s\n' "${pull_err}" | sed 's/^/ /' >&2
echo " The tag was likely never published by the FSDK build pipeline" >&2
echo " (a failed upstream build leaves it 'manifest unknown')." >&2
echo " Wait for the tag to be published, or set BST2_IMAGE to a known-good tag." >&2
exit 1
fi
fi
- info — no unit test covers the new guard.
tests/unit/test_ci_workflows.py:156-164already reads the Justfile to assert thebst2_imageline; a one-line assertion that thebstrecipe containspodman image exists "${bst2_image}"would keep the guard from being dropped silently in a future Justfile edit.
docs-currency
- info —
docs/skills/ci-tooling.md:73-74still describes the symptom ("can failjust bstwith "manifest unknown"") but not the new behaviour this PR adds:just bstnow fails fast with a message naming the cause and theBST2_IMAGEoverride (Justfile:48-52). Suggest appending: "just bstchecks for the image first and fails with a message that names the cause; setBST2_IMAGEto a known-good tag to keep working meanwhile."
No findings from: security, intent-alignment, style.
Confidence: 5/5 (safe)
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
@Danathar Fixed — push the exported shell var instead of the
Verified locally with 🐝 Hive Agent: contributor | SHA: 456972e |
jeefy
left a comment
There was a problem hiding this comment.
Approving. The bst recipe now uses the exported ${bst2_image}, validate is green, and the guard skips the pull when the image is already cached. The wording changes in docs/skills/ci-tooling.md and renovate.json match what happened with #548: its new tag (2232beb) is also unpublished, and upstream's master pipelines have failed since Oct 27.
Style nit from the hive review: the previous code wrote podman pull's
stderr to a fixed ${HOME}/.cache/bst2-pull.err and only removed it on
success, leaving a stale file behind on failure and letting two
concurrent 'just bst' runs clobber each other's error output. Capturing
the output in a shell variable removes the file entirely (non-blocking
cleanup suggested in the review).
Reviewed (hive + hanthor/jeefy)The two substantive findings were already addressed by the author before I looked:
One remaining non-blocking style nit from the hive review (the fixed Ready to merge once the consumer-validation gate is satisfied (a fork author cannot produce that check). 🐝 Hive Agent: |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Needs approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Sentinel reviewed and released at 91e6c07: diff read in full; it tightens or removes CI surface and grants no new permissions or secrets. See review notes in the maintainer log. |
Resolve the ci-tooling.md conflict with #553: keep #553's "a failed upstream build leaves it unpublished" and this branch's automerge hold in the same three lines, so the file stays at its 500-line cap. Assisted-by: Claude Opus 5.5 via GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary: Bluefin Server builds inside a base image whose version is identified by a tag — a named pointer to one upstream commit. The project's written instructions claimed that every change merged upstream gets its own fresh tag, so moving to the newest one always worked. That is not true: when the upstream project's own build fails, its new tag is never published, and trying to build locally then fails with a confusing "image not found" message. This change corrects those instructions and makes the local build fail with a clear, actionable message when that happens.
What I need from you: Approve it.
The instructions in
docs/skills/ci-tooling.mdand the Renovate configuration (renovate.json) both said that every merge in thefreedesktop-sdk-docker-imagesrepository publishes a tag for its commit. That assumption broke with thebst2base-image bump merged yesterday (#548): it pointed at a tag that was never published (manifest unknown), sojust bst— the command that runs the build inside that base image — failed deep in a build with an opaque error instead of saying what was wrong.What changed
docs/skills/ci-tooling.mdandrenovate.json: the wording now says the tag exists only after the upstream project's build pipeline has published it, and that a fresh base-image bump can failjust bstwith "manifest unknown" until the tag is actually available.Justfile: thebsttarget now checks that the base image is available before pulling. When the tag is missing it exits immediately with a message pointing at the upstream build, rather than failing later inside BuildStream.Why it matters
A failed upstream build is common and transient. Before this, reaching for the newest base image could look like a local-environment problem and send the user deep into a container error. Now the failure names the real cause on the first line.
Verification
just --justfile Justfile --summaryparses; thebstrecipe is intact.python3 .github/scripts/docs-checks.pypasses (the skill stays within its 500-line cap).python3 .github/scripts/check-release-version.pyandcheck-k0s-version.pypass.The image-existence guard could not be integration-tested here (no podman on this host), but the happy path is unchanged when the image is cached, and the new branch only adds a fast-fail error path.
Closes #549.
— hive: backend=pi model=lemonade/ornith-1.5-35b-a3b-gguf-q6_k
🐝 Hive Agent:
contributor| SHA:06f53ce