(locally) Build & push lading container script#1827
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
preinlein
changed the title
There was a problem hiding this comment.
This skips over the use of sccache (wanted to push off integrating with it as it adds some complexity) but even without it, this builds both architectures in ~8 min on my local laptop.
Good enough for v1. As long as we're <10 min for a step, the LLM loops handle that gracefully enough.
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
3088726 to
9696ba7
Compare
preinlein
force-pushed
the
paul.reinlein/build-container-locally
branch
2 times, most recently
from
95fb647 to
ea138f4
Compare
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
9696ba7 to
530bc15
Compare
preinlein
changed the title
Dockerfile
Outdated
| WORKDIR /app | ||
| RUN apt-get update && apt-get install -y \ | ||
| pkg-config=1.8.1-1 \ | ||
| libssl-dev=3.0.17-1~deb12u3 \ |
There was a problem hiding this comment.
this version is no longer available (builds were only working before because it was cached)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea138f4c39
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| jq 'del(.credsStore, .credHelpers)' "${REAL_DOCKER_CONFIG}/config.json" \ | ||
| > "${DOCKER_CONFIG_DIR}/config.json" |
There was a problem hiding this comment.
During push, the script unconditionally runs jq ... "${REAL_DOCKER_CONFIG}/config.json"; on fresh Docker setups where ~/.docker/config.json has not been created yet, jq fails and set -o errexit aborts before docker login can create credentials, so pushes fail for first-time users. Adding a missing-file fallback (for example initializing with {}) avoids this hard failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
imo an edge case that I'm willing to forego for now. I'd prefer for this to fail in those cases and to debug it then.
Since I've been running this on my laptop (and that's what I expect outside of workspaces), I expect docker to be configured/setup already.
This build container script hasn't been tested in workspaces yet.
preinlein
force-pushed
the
paul.reinlein/build-container-locally
branch
from
ea138f4 to
dee5150
Compare
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
530bc15 to
20ad18b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dee5150523
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
preinlein
force-pushed
the
paul.reinlein/build-container-locally
branch
from
dee5150 to
86828f8
Compare
|
updated and ran the script again successfully:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86828f82e3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if [[ -z "$TAG" ]]; then | ||
| TAG="sha-$(git rev-parse HEAD)" | ||
| echo "Using git SHA tag: ${TAG}" |
There was a problem hiding this comment.
If the checkout has uncommitted changes, this still tags the image as sha-<HEAD>. In the default push flow that makes a dirty local build indistinguishable from an image built from the clean commit, and it can overwrite the tag other people expect to mean "exactly commit X". That breaks traceability/reproducibility whenever someone publishes local edits without also remembering to pass an explicit --tag.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This looks worth addressing. The troublesome scenario I imagine here is two people try different changes on main at the same commit. What do you think about adding a unique value to this slug?
There was a problem hiding this comment.
Ah less of a worry with immutable tags. It'll fail loudly if it happens.
There was a problem hiding this comment.
Exactly :)
imo, tempted to leave as is until we hit too much pain that it warrants a change.
Because I kind of like having a commit sha as the ultimate source of truth
| if [[ "$MULTI_ARCH" == "true" ]]; then | ||
| echo "Setting up QEMU for multi-arch builds..." | ||
| docker run --rm --privileged tonistiigi/binfmt --install all >/dev/null 2>&1 || true |
There was a problem hiding this comment.
This only installs tonistiigi/binfmt when more than one platform is requested, but a single-platform foreign-arch build (--arch arm64 on x86, or --arch amd64 on Apple Silicon) still needs QEMU or a native builder. The Dockerfile runs target-arch apt-get/cargo commands instead of cross-compiling via BUILDPLATFORM/TARGETARCH, so the advertised single-arch cross-build mode fails on clean Linux hosts unless they were preconfigured out-of-band.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤷 not sure if this is really a concern
preinlein
force-pushed
the
paul.reinlein/build-container-locally
branch
from
86828f8 to
52504cb
Compare
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
20ad18b to
b8a9099
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52504cb025
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| --build-arg SCCACHE_BUCKET="" \ | ||
| --build-arg SCCACHE_REGION="" \ |
There was a problem hiding this comment.
The new Dockerfile guards only enable RUSTC_WRAPPER=sccache when SCCACHE_BUCKET is non-empty, but this script hard-codes --build-arg SCCACHE_BUCKET="" on every invocation and mounts empty AWS secrets. As a result, even the --push path that already runs under aws-vault exec never uses the shared compiler cache, so cold builds regress to recompiling the dependency graph from scratch instead of benefiting from the sccache work added in the previous container-build changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not too worried about that. If build times become an issue, we may do well to try using cargo-chef.
There was a problem hiding this comment.
Agreed. I think my last local build took <8 min for both release versions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52504cb025
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
GeorgeHahn
left a comment
GeorgeHahn
left a comment
There was a problem hiding this comment.
Some nits, no dealbreakers. This is very cool to see.
preinlein
force-pushed
the
paul.reinlein/build-container-locally
branch
from
52504cb to
b1390b6
Compare
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
b8a9099 to
89f0f93
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1390b6516
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if ! docker buildx inspect "$BUILDER_NAME" &>/dev/null; then | ||
| echo "Creating buildx builder: ${BUILDER_NAME}" | ||
| docker buildx create --name "$BUILDER_NAME" --driver docker-container --use | ||
| else | ||
| docker buildx use "$BUILDER_NAME" |
There was a problem hiding this comment.
Docker's buildx docs say docker buildx use "switches the current builder instance" and that subsequent buildx build commands use the selected builder by default. Because this helper calls docker buildx create --use / docker buildx use but never restores the previous selection or scopes its own build with --builder, running it permanently rewrites the user's default buildx builder. Anyone who already relies on a different selected builder (for example a remote/cloud builder or one with custom networking/certs) will find later builds silently running against lading-builder instead.
Useful? React with 👍 / 👎.
preinlein
force-pushed
the
paul.reinlein/build-container-locally
branch
from
b1390b6 to
d8a67d1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8a67d1427
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| jq 'del(.credsStore, .credHelpers)' "${REAL_DOCKER_CONFIG}/config.json" \ | ||
| > "${DOCKER_CONFIG_DIR}/config.json" |
There was a problem hiding this comment.
jq 'del(.credsStore, .credHelpers)' removes every non-ECR credential source from the temp config that line 143 later exports to docker buildx build. Docker's docker login docs say credsStore/credHelpers are how the CLI retrieves saved registry credentials, so the push path now pulls the Dockerfile's docker.io/rust and docker.io/debian bases anonymously. On hosts that rely on Docker Desktop's keychain-backed Docker Hub login or a helper for a private mirror, builds regress to 429/no basic auth credentials even though the user's normal Docker config works.
Useful? React with 👍 / 👎.
| for subdir in cli-plugins contexts; do | ||
| if [[ -d "${REAL_DOCKER_CONFIG}/${subdir}" ]]; then | ||
| ln -s "${REAL_DOCKER_CONFIG}/${subdir}" "${DOCKER_CONFIG_DIR}/${subdir}" | ||
| fi |
There was a problem hiding this comment.
Push mode swaps in a fresh DOCKER_CONFIG, but this loop only preserves cli-plugins and contexts. Docker's Buildx variables docs state that builder state and buildkitd.default.toml live under $DOCKER_CONFIG/buildx, so any existing builder settings needed for custom certs, registry mirrors, or network access disappear only on --push. The later inspect/create/use logic then runs against an empty Buildx config and silently falls back to a plain docker-container builder instead of the one the user already configured.
Useful? React with 👍 / 👎.
preinlein
force-pushed
the
paul.reinlein/build-container-locally
branch
from
d8a67d1 to
4982bc5
Compare
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
89f0f93 to
2f5b785
Compare
preinlein
force-pushed
the
paul.reinlein/build-container-locally
branch
from
4982bc5 to
eb3881f
Compare
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
2f5b785 to
be1c3d4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb3881f59b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| --tag) TAG="$2"; shift 2 ;; | ||
| --arch) ARCH="$2"; shift 2 ;; | ||
| --profile) AWS_VAULT_PROFILE="$2"; shift 2 ;; |
There was a problem hiding this comment.
--tag, --arch, and --profile unconditionally read $2, so a missing value silently consumes the following option instead of erroring. In ci/container-build-and-push --profile --no-push, for example, --no-push becomes the profile string and PUSH stays true, so the script can attempt an ECR push even though the caller explicitly asked for a local-only build. Rejecting missing values (or a next token that starts with -) avoids this misparse.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb3881f59b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Set up QEMU for cross-arch builds | ||
| if [[ "$MULTI_ARCH" == "true" ]]; then | ||
| echo "Setting up QEMU for multi-arch builds..." | ||
| docker run --rm --privileged tonistiigi/binfmt --install all >/dev/null 2>&1 || true |
There was a problem hiding this comment.
On hosts that actually depend on this emulation step for foreign-arch builds (for example the default amd64,arm64 path on a single-arch machine), docker run --privileged ... --install all can fail because privileged containers are disallowed or the helper image cannot be pulled. The trailing || true hides that prerequisite failure, so the script continues until docker buildx build dies later with a much less actionable platform/exec-format error instead of failing fast at the real cause.
Useful? React with 👍 / 👎.
Merge activity
|
preinlein
force-pushed
the
paul.reinlein/build-container-locally
branch
from
eb3881f to
db7a110
Compare
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
be1c3d4 to
3656530
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db7a110daf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| -f Dockerfile \ | ||
| . |
There was a problem hiding this comment.
This buildx invocation uses Dockerfile and . relative to the caller's working directory, so the script only works when it is launched from the repository root. Running ci/container-build-and-push from ci/, another workspace subdirectory, or via an absolute path from elsewhere makes Buildx read the wrong context (often omitting most of the repo) and typically fail with a missing Dockerfile or COPY source error. Anchoring both paths to git rev-parse --show-toplevel would avoid that brittle cwd dependency.
Useful? React with 👍 / 👎.
preinlein
changed the base branch from
paul.reinlein/introduce-smp-tests
to
graphite-base/1827
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db7a110daf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| -f Dockerfile \ | ||
| . |
There was a problem hiding this comment.
I checked the new ci/container-build-and-push path by invoking it from /workspace/lading/docs, and the final docker buildx build -f Dockerfile . still uses the caller's working directory as both the Dockerfile lookup path and build context. That makes a common ../ci/container-build-and-push invocation fail even when you're inside this repo, because docs/ does not contain the root Dockerfile or the full source tree needed by the build.
Useful? React with 👍 / 👎.
preinlein
force-pushed
the
paul.reinlein/build-container-locally
branch
from
db7a110 to
26ad209
Compare
preinlein
force-pushed
the
graphite-base/1827
branch
from
3656530 to
3febd2b
Compare
preinlein
force-pushed
the
paul.reinlein/build-container-locally
branch
from
26ad209 to
ebd05fd
Compare
preinlein
force-pushed
the
paul.reinlein/build-container-locally
branch
from
ebd05fd to
f86bdf6
Compare
preinlein
force-pushed
the
paul.reinlein/build-container-locally
branch
from
f86bdf6 to
861c9a1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 861c9a1681
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fi | ||
|
|
||
| if [[ "$RANDOM_TAG" == "true" ]]; then | ||
| TAG="$(uuidgen | tr '[:upper:]' '[:lower:]')" |
There was a problem hiding this comment.
uuidgen for --random-tag
In the repo's current workspace container, uuidgen is not installed, and this branch calls it unconditionally when --random-tag is passed. That makes the advertised flag exit with command not found before any Docker logic runs, so the script's local/agentic workflow cannot use random tags unless extra OS packages happen to be present. Please either gate this behind a prerequisite check or generate the tag with a tool that is already required here.
Useful? React with 👍 / 👎.
2 participants
What does this PR do?
Adds a
ci/container-build-and-pushscript for building and pushing lading Docker images locally, targeting thelading-devECR repository to avoid accidental overwrites of released images. Also updates the Dockerfile to work without sccache and bumpslibssl-dev.Motivation
Enable local and agentic-workflow Docker builds of lading without depending on the full CI pipeline. Uses a separate
lading-devECR repo so dev builds are isolated from production releases.Related issues
lading-devECR repo)Additional Notes
lading-devECR (notlading) per reviewer feedback to prevent accidental overwrites of released versionsaws,aws-vault,jq) are only required when pushing;--no-pushonly needsdocker--no-pushbuilds stay in cache only — use a single--archfor a loadable local imageSCCACHE_BUCKETbeing set, so local builds skip itlibssl-devpinned version bumped from3.0.17-1~deb12u3to3.0.18-1~deb12u2