Repository navigation
Conversation
codysoyland
previously approved these changes
Oct 26, 2026
codysoyland
left a comment
codysoyland
left a comment
There was a problem hiding this comment.
LGTM, with four non-blocking nits.
Nits (non-blocking)
- Pin the downloaded actiond binary digest:
.github/workflows/ci.yaml:173. - Exercise tree-artifact config staging in a regression test:
apko/private/apko_config.bzl:119. - Describe the actual HOME fallback condition:
apko/private/apko_image.bzl:116. - Remove the issue-history reference from the regression-gate comment:
.github/workflows/ci.yaml:215.
What I verified
- At
2fbfbce, WORKSPACE registration follows dependency creation ine2e/smoke/WORKSPACE.bazel:28; CI run 40409341233 logs show all four smoke tests passing in WORKSPACE mode on Bazel 7.6.1, 8.3.0, and 8.6.0. - The same run's actiond job completed with five remote actions;
.github/workflows/ci.yaml:230disables local fallback. - Tested uutils 0.0.16 and 0.1.0 directly: copying a read-only source directory with
cp -Rscreates a writable destination with per-file symlinks, matchingapko/private/apko_image.bzl:133. - Scratch cleanup uses an absolute tool path and an EXIT trap, and apko arguments retain their boundaries at
apko/private/apko_image.bzl:128-136. e2e/smoke/BUILD:15exercisesuse_default_shell_env = False. No local Bazel run was possible because Bazel is not installed.
Security
- CI: cleared; read-only token, no persisted checkout credentials, localhost worker, and bounded runtime (
.github/workflows/ci.yaml:131-147). Independent check confirmed. - Dependencies: cleared at blocking severity within the read-only CI job. The actiond checksum detects corruption but is fetched beside a mutable release asset; pinning its digest is a non-blocking improvement. Independent check confirmed.
Contributor
Author
|
Thanks for the review. I am not folding the four nits into this PR. Any push dismisses the existing approval (
|
The apko build action was pinned to local execution in #332 to work around #331. The underlying incompatibility is that apko writes into its --cache-dir even with --offline (it creates directories on cache lookup and expands packages into the cache), while Bazel provides the prepopulated cache tree artifact as a read-only action input, which remote executors enforce strictly. Local sandboxes keep directories writable, so this only surfaced under remote execution. - Copy the cache input into a writable scratch directory inside the action and point --cache-dir at it, so the action works on read-only input trees. - Give HOME a writable fallback when the executor does not provide one. - Bind the apko_image and apko_show_config actions to the resolved apko toolchain so an executor never receives a binary built for a different platform than the action is scheduled on. - Remove the no-remote-exec execution requirement from #332. - Add an opt-in use_default_shell_env attribute (default True, the previous behavior) so users who want a hermetic action environment can drop host env inheritance per target. Verified locally (all tests, examples, and e2e/smoke) and against a Buildbarn-based remote executor: both e2e/smoke image targets build with the apko actions executing on remote linux-amd64 workers from a macOS host.
apko cross-builds images for any target arch, so the comment's framing about arch mismatches was wrong; the toolchain param needs no explanation beyond what it says.
Mirror the read-only cache tree artifact into the writable scratch directory as real directories containing per-file symlinks rather than cp -RL'ing the whole cache. apko only adds new entries (package expansions) to its --cache-dir and never rewrites existing files, so new writes land in the scratch directories while existing entries are read through the symlinks. This avoids duplicating the full APK cache on every image build. The chmod is no longer needed since apko never writes to the symlinked files. Verified locally (darwin-sandbox) and on a Buildbarn-based remote executor with linux-amd64 workers; layer digests are identical to the copy-based version.
Adds an x86_64-only workflow that starts actiond, a local hermetic Remote Execution API worker that runs Bazel actions inside a Linux VM, and points Bazel at it to build //examples/wolfi-base. x86_64 only: GitHub's arm64 Linux runners have no /dev/kvm because the underlying Azure DPDsv6 SKU does not support nested virtualization, and the macOS arm64 runners cannot nest under Apple's Virtualization Framework. actiond's own CI has the same split.
CopyToWorkdir fails at exit 127 before the apko action is ever scheduled, which hides what #379 changed. Add a non-blocking follow-up build that runs the staging actions locally, so the apko action reaches the VM and its own missing-coreutils failure shows up in the log. Also trigger on actiond** branches so experiments do not have to land on main.
So the workflow triggers on the branch #379 is developed on, not just on main and actiond** experiment branches.
The run_shell actions in copy_to_workdir and apko_image call mkdir, cp, mktemp, find, ln, and rm, all resolved from the executor's PATH. That holds on a conventional RBE image and fails on an executor that ships no userspace: against actiond, a local RE worker that runs each action in an empty chroot, staging dies with "mkdir: command not found" (exit 127) and the apko action with "mktemp: command not found". Take the binary from aspect_bazel_lib's coreutils toolchain and declare it in tools, so it is uploaded as an action input and travels to whichever executor the action lands on. aspect_bazel_lib is already a bazel_dep and registers the toolchain itself, so this adds no dependency and needs no registration. The Linux assets are statically linked against musl, so they need nothing from the executor either. The find/ln mirror collapses into a single cp -Rs, which has the same effect - real directories, per-file symlinks into the read-only input tree - without needing find, which uutils does not ship. These actions still need a shell; making them fully shell-free would mean replacing copy_to_workdir with copy_to_directory and giving the apko action a wrapper binary.
Without a pull_request trigger the job never runs on a PR: a fork PR generates no push event in the upstream repo, so the workflow this PR proposes as a check would not have fired on the PR proposing it.
At the default spawn strategy an action that opts out of remote execution just runs on the host and the build still passes, so a regression of the kind #332 introduced would not have been caught. --spawn_strategy=remote with no local fallback turns that into a hard failure. Drop the diagnostic step: it existed to surface the apko action's failure when staging failed first, which no longer happens, and a continue-on-error step reads as a passing check when it is not one.
needs: can only reference jobs in the same workflow, so a separate actiond.yaml could never be a dependency of the summary gate. Moving the job into ci.yaml and adding it to test-roundup's needs means branch protection keeps requiring exactly one context while remote execution becomes part of what that context covers.
aspect_bazel_lib 1.x, the version rules_apko_dependencies() fetches for WORKSPACE users, registers uutils coreutils 0.0.16. That release treats "cp -R src/. dst" as a copy of a directory named after src and nests it at dst/<basename>. With it, the apko action's cache mirror lands one level too deep and apko fails at --offline with "failed to read ... in offline cache", and copy_to_workdir nests directory inputs the same way. uutils 0.0.23 and later, and GNU cp, are not affected. Copy to a destination that does not exist yet instead, which every uutils release aspect_bazel_lib has shipped (0.0.16, 0.0.23, 0.0.26, 0.0.27, 0.1.0) and GNU cp handle the same way: the apko action no longer pre-creates the cache mirror directory, and copy_to_workdir removes the pre-created output directory before copying a directory into its place. Resolve the cache source with pwd -P so cp -Rs always starts from a real directory rather than copying a symlink to one.
Taking mkdir/cp/mktemp from aspect_bazel_lib's coreutils toolchain makes analysis fail in WORKSPACE mode with "No matching toolchains found for @@aspect_bazel_lib//lib:coreutils_toolchain_type", because nothing in the documented WORKSPACE setup registers it. Bzlmod users are not affected: aspect_bazel_lib registers it from its MODULE.bazel. The aspect_bazel_lib 1.34.5 that rules_apko_dependencies() fetches already defines the toolchain and register_coreutils_toolchains(), so no version bump is needed. Registration cannot move into rules_apko_dependencies() itself, since repositories.bzl is loaded before @aspect_bazel_lib exists, so the setup gains one load and one call. Also load local_repository from @bazel_tools. @bazel does not exist in WORKSPACE mode, so this file could not be loaded at all.
CI only ever ran with --enable_bzlmod, so neither the missing coreutils registration nor the unloadable e2e/smoke WORKSPACE file showed up. Run e2e/smoke a second time with --noenable_bzlmod --enable_workspace on the Bazel versions that still support WORKSPACE. Also fix the setup-bazel pin comments: the action's tags have no "v" prefix, so zizmor reported "v0.19.0" as an unknown ref.
Pin the actiond binary by its SHA-256 in ci.yaml instead of reading the checksum from a file served beside the release asset. Add e2e/smoke tree_staging, which stages a directory through copy_to_workdir and checks its contents land at the destination; it fails with the previous "src/." copy form on uutils 0.0.16. Describe the condition under which apko_image's HOME fallback applies, and drop the issue reference from the remote-execution gate comment.
Member
|
i actually dislike the coreutils toolchain thing. I wonder if instead apko should learn to deal with "rules_apko generated cache which is readonly". Cause it can handle "apko generated cache readonly". As in we might be able to fix all of this with adding test case in apko, that generates cache with rules_apko, hops into a docker or bubble wrap container with said rules_apko style cache mounted readonly. That way we might avoid the need for all the coreutils toolchain shenanigans. |
codysoyland
reviewed
Oct 27, 2026
codysoyland
left a comment
codysoyland
left a comment
There was a problem hiding this comment.
Automated review by unblok using Codex · gpt-6-astra · medium reasoning effort
Fixing apko could remove the writable cache mirror, but it would not by itself remove the current staging actions. Those still need tools on an empty remote executor. Can the proposed approach also replace that staging?
Verified at 2fbfbce: apko_config.bzl:116 uses mkdir/cp for config staging; apko_image.bzl:104-105 stages the binary separately from the cache mirror. I haven't tested the proposed apko change.
shimmerjs
force-pushed
the
fix-380-workspace-coreutils
branch
from
October 27, 2026 16:08
2fbfbce to
87b9ff9
Compare
Contributor
Author
|
The four review nits are now in this PR (87b9ff9), so there is no follow-up PR:
I also stripped the |
codysoyland
approved these changes
Oct 27, 2026
codysoyland
left a comment
codysoyland
left a comment
There was a problem hiding this comment.
Automated review by unblok using Codex · gpt-6-astra · medium reasoning effort
Re-reviewed after 87b9ff9. LGTM; all four implementation nits are addressed.
- Digest pin: addressed at
.github/workflows/ci.yaml:136,173; the checked-in SHA-256 is verified before execution and matches the release asset digest. - Directory regression test: addressed at
e2e/smoke/tree_staging/tree.bzl:20,28; it exercises the helper with a tree artifact and checks the expected path and absence of nesting. - HOME comment: addressed at
apko/private/apko_image.bzl:116; it now describes unset or empty HOME. - History reference: addressed at
.github/workflows/ci.yaml:215; removed. - The earlier staging-design question remains open and non-blocking; this push keeps the existing approach.
What I verified
//tree_staging:testdepends on the marker action consuming the staged tree, so failed path checks fail the build. CI run 40538033389 for this head reports five passing smoke tests in both bzlmod and WORKSPACE mode on Bazel 7.6.1, including this new test.- The download step uses
set -euxo pipefailand checks the pinned digest before chmod or execution. The pin matches the GitHub release asset API. - The version-related changes match current main; the substantive update is limited to the workflow pin, comment corrections, and tree fixture. No local suite or build was run.
Security
Contributor
Author
|
Closing. #411 merged and shipped in v1.5.59: it removes the If someone hits an executor with no |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please read: this stacks on #380
This branch contains #380 (@xnox) unchanged, including the #379 commits it carries. Everything after 2500938 is new: three commits (c36c17f, f4ffb22, 2fbfbce). If #380 lands first, this rebases down to those three. If it is easier, the three commits can be cherry-picked into #380 instead.
#380 makes the apko and staging actions take mkdir/cp/mktemp/rm from aspect_bazel_lib's coreutils toolchain. That works under bzlmod. It breaks WORKSPACE users on Bazel 7 and 8 in two ways, and this fixes both.
No matching toolchains found for types @@aspect_bazel_lib//lib:coreutils_toolchain_type, because the documented WORKSPACE setup never registers the toolchain. The aspect_bazel_lib 1.34.5 thatrules_apko_dependencies()fetches already defines the toolchain andregister_coreutils_toolchains(), so no version bump is needed. The fix adds one load and one call to the setup in e2e/smoke/WORKSPACE.bazel. Registration can't move intorules_apko_dependencies()itself, because repositories.bzl is loaded before@aspect_bazel_libexists.failed to read ".../cache/https%3A%2F%2Fdl-cdn.alpinelinux.org%2F.../busybox-1.37.0-r14.apk" in offline cache: ... no such file or directory. aspect_bazel_lib 1.x registers uutils coreutils 0.0.16. That release copiescp -R src/. dsttodst/<basename>, so the cache mirror lands one level too deep.copy_to_workdirnests directory inputs the same way. The fix copies to a destination that doesn't exist yet. That form behaves the same on every uutils release aspect_bazel_lib has shipped (0.0.16, 0.0.23, 0.0.26, 0.0.27, 0.1.0) and on GNU cp. The mirror is still one symlink per file inside real directories.Also:
local_repositoryfrom@bazel//..., which does not exist in WORKSPACE mode, so the file could not load at all. It now loads from@bazel_tools.--enable_bzlmod. A new step runs e2e/smoke with--noenable_bzlmod --enable_workspaceon Bazel 7.6.1, 8.3.0 and 8.6.0.vprefix, so the# v0.19.0pin comments pointed at an unknown ref (zizmor ref-version-mismatch). They now read# 0.19.0. That also clears the same open alert on main (ci.yaml:89).Breaking change? WORKSPACE users who do not already register the coreutils toolchain must add two lines. Analysis fails until they do, and the error names the toolchain type. Users who call
aspect_bazel_lib_dependencies()from aspect_bazel_lib 1.x already have it, because that function always registers coreutils. Bzlmod users are unaffected. Bumping the pinned aspect_bazel_lib to 2.x would not avoid the edit. In 2.x,@aspect_bazel_lib//lib:coreutils_toolchain_typeis an alias into@bazel_lib, andmaybe()means a user's existing pin wins anyway.Evidence (macOS arm64 host and an arm64 Linux container):
Before, #380 head 2500938, e2e/smoke, Bazel 7.6.1,
--noenable_bzlmod --enable_workspace:Before, same, with
register_coreutils_toolchains()added to the WORKSPACE:After, this branch, e2e/smoke:
Root workspace, Bazel 9.1.0, Linux container with clang:
//docs:update_0_test,//docs:update_1_test,//apko/tests:versions_test_test_0,//examples/lock:testand//examples/wolfi-base:testpass.//examples/simple:test_apko_genrulefails there, and fails the same way on main, because the container's busybox grep rejects--fixed-strings. On macOS,//apko/... //examples/...gives 4/4.bazel run //.github/workflows:buildifier.checkis clean.Hermeticity probe, Linux container, Bazel 9.1.0: fetch, delete mkdir/mktemp/ln/find from PATH, then
bazel build //... --spawn_strategy=localin e2e/smoke. This branch builds (INFO: Build completed successfully, 61 total actions). main fails (/bin/bash: line 2: mkdir: command not found). cp and rm could not be removed in that container, so the probe covers the other four tools only.Not verified here: the actiond remote-execution job (it needs x86_64 KVM; CI runs it). The directory branch of
copy_to_workdirwas checked with each uutils binary directly, not through a Bazel target, because no test target stages a tree-artifact config input.