Skip to content

ci: fix Alpine ownership during LLGo release packaging - #3

Merged
cpunion merged 3 commits into
xgo-dev:llgofrom
cpunion:codex/llgo-release-permissions
Sep 23, 2026
Merged

cpunion merged 3 commits into
xgo-dev:llgofrom
cpunion:codex/llgo-release-permissions

Conversation

@cpunion

@cpunion cpunion commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

The first llgo-v132.1 release run exposed two packaging failures after its source verification passed:

  • ARM64 Alpine passed the Binaryen test suite, then could not copy LICENSE and LLGO.md into the archive. The container installed into the bind-mounted workspace as root; return ownership of install/ to the host runner before adding notices and packing. This also covers x86-64 Linux.
  • Windows ARM64 configured CMake in out but tried to build out-arm64. Configure and build the same directory.

Release packaging now runs on every PR targeting llgo: the platform builds, tests, archive creation, and checksum generation all run before merge. PR archives use a safe pr-<number> version. Only upload and publication are skipped on PRs; tag builds still verify that the tag points to the llgo branch tip and publish after all packaging jobs pass.

Binaryen source and compiler behavior are unchanged. Validation: local Alpine bind-mount ownership and notice-copy smoke passed; actionlint -shellcheck= and git diff --check pass. The original failed release jobs are Alpine ARM64 and Windows ARM64.

After this PR merges and its packaging checks pass, a new llgo-v132.2 tag at the reviewed llgo tip can publish the corrected archives. The failed llgo-v132.1 tag remains unpublished.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: LLGo release permissions fix

The change adds a chown -R "$(id -u):$(id -g)" install inside the Alpine container during the archive step. This correctly fixes the real problem where ninja install runs as root inside the bind-mounted container (-v $PWD:/src, no --user), leaving install/ root-owned so subsequent host-side steps (mv, scripts/llgo-copy-notices.sh, tar) can't write into it.

Correctness: $(id -u):$(id -g) is expanded by the host shell into numeric IDs and passed to chown running as root in the container, so it applies regardless of whether a matching account exists in node:lts-alpine. Placement (after strip, before the first host write) is correct.

Security / performance: No issues. IDs are numeric and not attacker-controlled; the chown -R is a one-time bounded operation on the install tree.

One optional clarity note left inline. Looks good to merge.

TARBALL=$PKGNAME.tar.gz
SHASUM=$PKGNAME.tar.gz.sha256
./alpine.sh find install/ -type f -perm -u=x -exec strip {} +
# The container installs as root into the bind-mounted workspace.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] Optional: clarify that chown runs inside the container

Nit / optional. The comment reads as if chown runs on the host, but ./alpine.sh is docker exec alpine "$@", so the chown executes inside the container as root — it works because $(id -u):$(id -g) is expanded by the host shell to numeric IDs. A future maintainer might mistakenly rewrite this as a username (e.g. $(whoami)), which would fail since that account doesn't exist in node:lts-alpine. Consider a one-line note that numeric host IDs are intentional and the command runs in the container, e.g.:

# Run chown in the container (as root), targeting the host runner's
# numeric UID:GID so it works even without a matching account in Alpine.

No change required for correctness.

@cpunion
cpunion merged commit e8939a2 into xgo-dev:llgo Sep 23, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant