docker: Add HEADS_FORCE_DOCKER_REBUILD and improve reproducibility checks#2081
Conversation
tlaurion
force-pushed
the
docker_scripts_improvements-local_dev_reconstruct_from_nix
branch
from
8f2510a to
8ee7c58
Compare
There was a problem hiding this comment.
Pull request overview
Adds a forced rebuild option to the Docker/Nix developer workflow and refines the reproducibility check to be more transparent about which digests are being compared and how they were obtained.
Changes:
- Introduces
HEADS_FORCE_DOCKER_REBUILD=1to rebuild fromflake.nix/flake.lockregardless of git status (and attempts to clear cached build outputs). - Improves Nix build visibility (
--print-build-logs) and refactors reproducibility-check messaging / helper functions. - Updates Docker documentation to explain config digest vs manifest digest and updates example output.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
docker/common.sh |
Adds forced rebuild behavior, changes nix build/load invocation, and extends reproducibility-check helpers/output. |
docker_local_dev.sh |
Documents/exposes HEADS_FORCE_DOCKER_REBUILD and keeps dev wrapper aligned with common.sh. |
docker_latest.sh |
Indentation normalization; continues to rely on shared helpers. |
docker_repro.sh |
Indentation normalization; continues to enforce digest-pinned images. |
doc/docker.md |
Documents the new env var and expands digest/reproducibility explanations and example output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tlaurion
force-pushed
the
docker_scripts_improvements-local_dev_reconstruct_from_nix
branch
2 times, most recently
from
7af1497 to
d1ba3c4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tlaurion
force-pushed
the
docker_scripts_improvements-local_dev_reconstruct_from_nix
branch
2 times, most recently
from
cb2beb7 to
0d9c44f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tlaurion
force-pushed
the
docker_scripts_improvements-local_dev_reconstruct_from_nix
branch
2 times, most recently
from
6a7b71f to
b7a7409
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tlaurion
force-pushed
the
docker_scripts_improvements-local_dev_reconstruct_from_nix
branch
from
b7a7409 to
b9854e5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tlaurion
force-pushed
the
docker_scripts_improvements-local_dev_reconstruct_from_nix
branch
from
b9854e5 to
0afb709
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tlaurion
force-pushed
the
docker_scripts_improvements-local_dev_reconstruct_from_nix
branch
from
0afb709 to
bf27183
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tlaurion
force-pushed
the
docker_scripts_improvements-local_dev_reconstruct_from_nix
branch
from
bf27183 to
2fa819e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Get auth token (Docker Hub auth endpoint). Ignore failures silently. | ||
| local auth_url token manifest | ||
| auth_url="https://auth.docker.io/token?service=registry.docker.io&scope=repository:${repo}:pull" | ||
| if command -v jq >/dev/null 2>&1; then | ||
| token=$(curl -fsSL "${auth_url}" 2>/dev/null | jq -r '.token // empty' 2>/dev/null || true) | ||
| else | ||
| token=$(curl -fsSL "${auth_url}" 2>/dev/null | tr -d '\n' | sed -nE 's/.*"token"\s*:\s*"([^\"]+)".*/\1/p' || true) | ||
| fi | ||
| if [ -n "${token}" ]; then | ||
| if command -v jq >/dev/null 2>&1; then | ||
| manifest=$(curl -fsSL -H "Accept: application/vnd.docker.distribution.manifest.v2+json" -H "Authorization: Bearer ${token}" "https://${host}/v2/${repo}/manifests/${tag}" 2>/dev/null || true) | ||
| digest=$(printf '%s' "${manifest}" | jq -r '.config.digest // empty' 2>/dev/null || true) | ||
| method="registry+jq" | ||
|
|
||
| else | ||
| manifest=$(curl -fsSL -H "Accept: application/vnd.docker.distribution.manifest.v2+json" -H "Authorization: Bearer ${token}" "https://${host}/v2/${repo}/manifests/${tag}" 2>/dev/null | tr -d '\n' || true) | ||
| digest=$(printf '%s' "${manifest}" | sed -nE 's/.*"config"[^}]*"digest"\s*:\s*"([^" ]+)".*/\1/p' || true) | ||
| method="registry+sed" | ||
|
|
||
| fi | ||
| if [ -n "${digest}" ]; then | ||
| printf '%s\t%s\n' "${digest}" "${method}" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
This curl-based registry fallback always fetches a token from https://auth.docker.io/... even when remote_image specifies a non–Docker Hub registry host. That will never work for registries like ghcr.io/quay.io and just adds confusing network calls before falling back to docker pull. Consider short-circuiting this method unless host resolves to Docker Hub (e.g., registry-1.docker.io / docker.io aliases), or implement per-registry auth/token discovery.
| # Get auth token (Docker Hub auth endpoint). Ignore failures silently. | |
| local auth_url token manifest | |
| auth_url="https://auth.docker.io/token?service=registry.docker.io&scope=repository:${repo}:pull" | |
| if command -v jq >/dev/null 2>&1; then | |
| token=$(curl -fsSL "${auth_url}" 2>/dev/null | jq -r '.token // empty' 2>/dev/null || true) | |
| else | |
| token=$(curl -fsSL "${auth_url}" 2>/dev/null | tr -d '\n' | sed -nE 's/.*"token"\s*:\s*"([^\"]+)".*/\1/p' || true) | |
| fi | |
| if [ -n "${token}" ]; then | |
| if command -v jq >/dev/null 2>&1; then | |
| manifest=$(curl -fsSL -H "Accept: application/vnd.docker.distribution.manifest.v2+json" -H "Authorization: Bearer ${token}" "https://${host}/v2/${repo}/manifests/${tag}" 2>/dev/null || true) | |
| digest=$(printf '%s' "${manifest}" | jq -r '.config.digest // empty' 2>/dev/null || true) | |
| method="registry+jq" | |
| else | |
| manifest=$(curl -fsSL -H "Accept: application/vnd.docker.distribution.manifest.v2+json" -H "Authorization: Bearer ${token}" "https://${host}/v2/${repo}/manifests/${tag}" 2>/dev/null | tr -d '\n' || true) | |
| digest=$(printf '%s' "${manifest}" | sed -nE 's/.*"config"[^}]*"digest"\s*:\s*"([^" ]+)".*/\1/p' || true) | |
| method="registry+sed" | |
| fi | |
| if [ -n "${digest}" ]; then | |
| printf '%s\t%s\n' "${digest}" "${method}" | |
| return 0 | |
| fi | |
| # The token endpoint below is Docker Hub-specific. Skip this curl-based | |
| # registry lookup for non-Docker Hub registries and let later fallbacks | |
| # (for example docker pull / inspect) handle those cases. | |
| local is_docker_hub auth_url token manifest | |
| is_docker_hub=0 | |
| case "${host}" in | |
| registry-1.docker.io|docker.io|index.docker.io) | |
| is_docker_hub=1 | |
| ;; | |
| esac | |
| if [ "${is_docker_hub}" -eq 1 ]; then | |
| # Get auth token (Docker Hub auth endpoint). Ignore failures silently. | |
| auth_url="https://auth.docker.io/token?service=registry.docker.io&scope=repository:${repo}:pull" | |
| if command -v jq >/dev/null 2>&1; then | |
| token=$(curl -fsSL "${auth_url}" 2>/dev/null | jq -r '.token // empty' 2>/dev/null || true) | |
| else | |
| token=$(curl -fsSL "${auth_url}" 2>/dev/null | tr -d '\n' | sed -nE 's/.*"token"\s*:\s*"([^\"]+)".*/\1/p' || true) | |
| fi | |
| if [ -n "${token}" ]; then | |
| if command -v jq >/dev/null 2>&1; then | |
| manifest=$(curl -fsSL -H "Accept: application/vnd.docker.distribution.manifest.v2+json" -H "Authorization: Bearer ${token}" "https://${host}/v2/${repo}/manifests/${tag}" 2>/dev/null || true) | |
| digest=$(printf '%s' "${manifest}" | jq -r '.config.digest // empty' 2>/dev/null || true) | |
| method="registry+jq" | |
| else | |
| manifest=$(curl -fsSL -H "Accept: application/vnd.docker.distribution.manifest.v2+json" -H "Authorization: Bearer ${token}" "https://${host}/v2/${repo}/manifests/${tag}" 2>/dev/null | tr -d '\n' || true) | |
| digest=$(printf '%s' "${manifest}" | sed -nE 's/.*"config"[^}]*"digest"\s*:\s*"([^" ]+)".*/\1/p' || true) | |
| method="registry+sed" | |
| fi | |
| if [ -n "${digest}" ]; then | |
| printf '%s\t%s\n' "${digest}" "${method}" | |
| return 0 | |
| fi | |
| fi |
docker/common.sh
Outdated
| local host repo tag repo_with_tag first | ||
| repo_with_tag="${remote_image}" | ||
| tag="${repo_with_tag##*:}" | ||
| repo="${repo_with_tag%:*}" |
There was a problem hiding this comment.
The registry-API fallback parses tag/repo using ${repo_with_tag##*:} and ${repo_with_tag%:*}, which breaks for image refs that include a registry host:port but no explicit tag (e.g., localhost:5000/repo becomes tag 5000/repo and repo localhost). Parse the tag only from the last path component (after the final /) and default to latest when no tag is present, similar to the logic used in get_remote_manifest_digest().
| local host repo tag repo_with_tag first | |
| repo_with_tag="${remote_image}" | |
| tag="${repo_with_tag##*:}" | |
| repo="${repo_with_tag%:*}" | |
| local host repo tag repo_with_tag first last_component | |
| repo_with_tag="${remote_image}" | |
| last_component="${repo_with_tag##*/}" | |
| if [[ "${last_component}" == *:* ]]; then | |
| tag="${last_component##*:}" | |
| repo="${repo_with_tag%:*}" | |
| else | |
| tag="latest" | |
| repo="${repo_with_tag}" | |
| fi |
tlaurion
force-pushed
the
docker_scripts_improvements-local_dev_reconstruct_from_nix
branch
from
2fa819e to
9afe3e8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docker/common.sh
Outdated
| # Only delete from nix store if it's actually a valid store path | ||
| if [[ "$result_target" == /nix/store/* ]]; then | ||
| echo "Deleting cached nix result: $result_target" >&2 | ||
| nix store delete "$result_target" 2>/dev/null || true |
There was a problem hiding this comment.
In the HEADS_FORCE_DOCKER_REBUILD path, the script prints that it is deleting the cached nix result but then runs nix store delete ... 2>/dev/null || true, so failures (including nix not installed yet, permission issues, or liveness preventing deletion) are silently ignored. That can defeat the goal of “force rebuild” (Nix may reuse the existing store path) and mislead users into thinking the cache was cleared. Consider guarding with command -v nix, and if deletion fails, emit a warning and/or fall back to just removing the result link while explaining that the store path could not be removed.
| nix store delete "$result_target" 2>/dev/null || true | |
| if command -v nix >/dev/null 2>&1; then | |
| if ! nix store delete "$result_target"; then | |
| echo "Warning: failed to delete nix store path: $result_target" >&2 | |
| echo "The result symlink was removed, but the store path could not be deleted and may still be reused." >&2 | |
| fi | |
| else | |
| echo "Warning: nix is not available; removed the result symlink but could not delete store path: $result_target" >&2 | |
| fi |
docker/common.sh
Outdated
| # Get manifest headers and capture the Docker-Content-Digest header without reading the body into memory | ||
| manifest_headers=$(curl -fsSL -D - -o /dev/null -H "Accept: application/vnd.docker.distribution.manifest.v2+json" -H "Authorization: Bearer ${token}" "https://${host}/v2/${repo}/manifests/${tag}" 2>/dev/null || true) | ||
| manifest_digest=$(echo "${manifest_headers}" | grep -i "docker-content-digest:" | head -1 | tr -d '\r' | sed -nE 's/.*:\s*(sha256:([a-fA-F0-9]{64})).*/\1/p' || true) |
There was a problem hiding this comment.
get_remote_manifest_digest captures headers with curl -L (-fsSL) and then selects the first docker-content-digest header via head -1. If redirects occur, curl will include multiple header blocks and the first match may be from an intermediate response (or none), producing the wrong/empty digest. Prefer selecting the last matching header (e.g., tail -1) or avoid -L here and explicitly handle non-200 responses.
| # Get manifest headers and capture the Docker-Content-Digest header without reading the body into memory | |
| manifest_headers=$(curl -fsSL -D - -o /dev/null -H "Accept: application/vnd.docker.distribution.manifest.v2+json" -H "Authorization: Bearer ${token}" "https://${host}/v2/${repo}/manifests/${tag}" 2>/dev/null || true) | |
| manifest_digest=$(echo "${manifest_headers}" | grep -i "docker-content-digest:" | head -1 | tr -d '\r' | sed -nE 's/.*:\s*(sha256:([a-fA-F0-9]{64})).*/\1/p' || true) | |
| # Get manifest headers and capture the Docker-Content-Digest header from the final response | |
| # without reading the body into memory. curl -L can emit multiple header blocks when | |
| # redirects occur, so select the last matching digest header. | |
| manifest_headers=$(curl -fsSL -D - -o /dev/null -H "Accept: application/vnd.docker.distribution.manifest.v2+json" -H "Authorization: Bearer ${token}" "https://${host}/v2/${repo}/manifests/${tag}" 2>/dev/null || true) | |
| manifest_digest=$(echo "${manifest_headers}" | grep -i "docker-content-digest:" | tail -1 | tr -d '\r' | sed -nE 's/.*:\s*(sha256:([a-fA-F0-9]{64})).*/\1/p' || true) |
…ecks - Add HEADS_FORCE_DOCKER_REBUILD=1 to force rebuild from flake.nix/flake.lock - Delete cached nix store result when forcing rebuild - Add --print-build-logs to nix build for visibility - Use docker load -i instead of docker load < for consistency - Improve reproducibility check: explain config vs manifest digests - Show method used (registry+jq, registry+sed, or pulled) - Add tip to install jq/curl for faster checks - Add get_remote_manifest_digest() with correct Docker Hub URL format - Update doc/docker.md explaining config vs manifest digests - Normalize indentation to tabs across docker scripts Fixes: - local result_target declaration in force rebuild - handle regular file case for result (not just symlink) - use printf instead of echo in hash computation - fall back to shasum when sha256sum unavailable - ensure temp directory cleanup on all paths - handle @digest references in get_remote_manifest_digest - restrict sha256 regex to exactly 64 hex chars - use remote_method instead of hardcoded message - Docker Hub URL uses sha256-{digest} not sha256:{digest} - fix regex in get_remote_config_digest: use \. not \. for dot matching - remove unused get_local_manifest_digest function - move End marker to actual end points - distinguish fetch_failed from mismatch in fallback message - update documentation mismatch example to match current output - check curl availability in get_remote_config_digest - only show Docker Hub URL for Docker Hub images - add curl availability check to get_remote_manifest_digest Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion
force-pushed
the
docker_scripts_improvements-local_dev_reconstruct_from_nix
branch
from
9afe3e8 to
caf4cd5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fi | ||
|
|
||
| # Show the installer URL and attempt to detect a version string from the installer contents. | ||
| echo "Installer URL: ${installer_url}" >&2 |
There was a problem hiding this comment.
installer_detected_version is assigned without being declared local. Since common.sh is meant to be sourced as a library, this leaks a global variable into the caller’s shell and can cause hard-to-debug collisions. Declare it as a local variable (e.g., local installer_detected_version) before assignment.
| echo "Installer URL: ${installer_url}" >&2 | |
| echo "Installer URL: ${installer_url}" >&2 | |
| local installer_detected_version |
| remote_inst_sha="" | ||
| # Prefer explicit sha_url (set via HEADS_NIX_INSTALLER_VERSION or HEADS_NIX_INSTALLER_URL override) | ||
| candidate_sha_urls=() |
There was a problem hiding this comment.
remote_inst_sha and candidate_sha_urls are assigned/used without local declarations. Because this script is sourced, these become globals and can unexpectedly override variables in the parent shell or other helpers. Please make them local to ensure_nix_and_flakes (e.g., local remote_inst_sha and local -a candidate_sha_urls).
| remote_inst_sha="" | |
| # Prefer explicit sha_url (set via HEADS_NIX_INSTALLER_VERSION or HEADS_NIX_INSTALLER_URL override) | |
| candidate_sha_urls=() | |
| local remote_inst_sha="" | |
| # Prefer explicit sha_url (set via HEADS_NIX_INSTALLER_VERSION or HEADS_NIX_INSTALLER_URL override) | |
| local -a candidate_sha_urls=() |
| *) | ||
| # Get command name and match exactly 'scdaemon' or 'pcscd' | ||
| cmd=$(ps -p "${_pid}" -o comm= 2>/dev/null || true) | ||
| if printf '%s' "${cmd}" | grep -qE '^scdaemon$|^pcscd$'; then | ||
| matched_pids+=("${_pid}") |
There was a problem hiding this comment.
In kill_usb_processes, cmd is assigned inside the PID loop but is never declared local, so it becomes (or overwrites) a global variable when common.sh is sourced. Declare it as a local (e.g., local cmd) to avoid polluting the caller’s environment.
| fi | ||
| local hub_manifest="${remote_manifest#sha256:}" | ||
| local hub_url="https://hub.docker.com/layers/${remote_repo}/${remote_tag}/images/sha256-${hub_manifest}" | ||
| echo "Note: manifest digest differs from config (normal)" >&2 |
There was a problem hiding this comment.
The comment above says the manifest/config mismatch message should mention metadata, and doc/docker.md’s example output also includes that explanation, but the actual output here only prints “(normal)”. Please make the wording consistent (either expand the echo to include the explanation, or update the comment/docs to match the shorter message).
| echo "Note: manifest digest differs from config (normal)" >&2 | |
| echo "Note: manifest digest differs from config (normal - manifest includes metadata)" >&2 |
@JohnAZoidberg
Then check reproducibility: