Repository navigation
feat: Add hull version on Linux and test the Linux CLI on arm64 - #128
Conversation
|
@claude-fleet review |
|
Review task queued. Task: 850b3579-9bcf-42c9-91bb-60f2fb4f4841 |
There was a problem hiding this comment.
Approve: no blocking or should_fix findings. Each of the three commits builds and the claims in the description hold against the code.
Findings:
- nit, .github/workflows/ci.yml:214 — the check rename is recorded only in the PR body, not in ci.yml next to the file's existing branch-protection note
Verified:
version.goandversion_test.godepend on nothing darwin-only:buildexists on both platforms (cmd/hull/main.go:39, cmd/hull/main_linux.go:35) andprintJSONis untagged (cmd/hull/jsonout.go:38).GOOS=linux go vet ./cmd/hull/type-checks the package and its tests on both architectures.hull --versionon Linux goes through hull's printer:initat cmd/hull/version.go:31 is now compiled on Linux, and cmd/hull/main_linux.go:50 setsVersion: build.String(), so the flag exists. TestBothVersionSpellingsAgree (cmd/hull/version_test.go:48-63) now runs on Linux and asserts the printer is installed before calling it.- The code
archive.Applyruns is identical between v1.7.36 and v2.3.5:tar_unix.go,tar_opts.go,tar_mostunix.go,link_default.goandtime.godiff empty.tar.godiffers only in import paths,any/De Morgan rewrites at lines 308 and 353, and theChangeWriterwhiteout-timestamp change — all on the diff-writing path, which pkg/ociclient/unpack.go:222 does not use. The%w-wrappedos.Lchownerror that unpack.go:232-253 relies on is unchanged. - No
github.com/containerd/containerdv1 import remains anywhere in the tree (grep -rn 'github.com/containerd/containerd' --include='*.go'minus/v2/is empty), andgo mod tidy -diffis clean, so v1 is correctly indirect-only. - Both containerd 2.3.5 checksums in the matrix match what containerd publishes.
ubuntu-24.04-armis already in use at .github/workflows/build-images.yml:36.- Nothing
needs:unit-linuxand no other file references the old check name, so the rename breaks no in-repo dependency. - Docs: the two places that list the Linux command set (docs/build.md:17, docs/build.md:105) are both updated; no other file carries such a list.
Checks run:
GOOS=linux GOARCH=arm64 go build ./...andGOOS=linux GOARCH=amd64 go build ./...→ both okGOOS=linux GOARCH=arm64 go vet ./cmd/hull/ ./pkg/ociclient/→ okgo test -count=1 -run 'TestVersion|TestBothVersion' ./cmd/hull/→ okgo mod tidy -diff→ no diffcurlof containerd's published.sha256sumfor linux/amd64 and linux/arm64 at v2.3.5 → both match the matrix valuesdiffofcontainerd@v1.7.36/archiveagainstcontainerd/v2@v2.3.5/pkg/archive
Smaller things:
printVersion(cmd/hull/version.go:34) writes toos.Stdoutrather than the command's writer, so a test driving the Linux tree the way cmd/hull/images_linux_test.go:62-70 does (app.Writer = &out) would not capturehull version. Pre-existing and identical on macOS, and version_test.go works around it by swappingos.Stdout. Worth knowing before someone adds a Linux end-to-end test for the verb.- The one changed line in cmd/hull/main_linux.go:75 — the registration itself — has no test on either platform; version_test.go calls
versionCommand()directly. That matches how the darwin registration at cmd/hull/main.go:110 is covered, so it is a consistent gap rather than a new one. - The file-level comment at .github/workflows/ci.yml:30-31 still says
unit-linuxruns "on a Linux runner", singular. Accurate enough, but it is the one other sentence describing the job.
Task 850b3579-9bcf-42c9-91bb-60f2fb4f4841 on pmoust-mbp, model claude-opus-5, effort medium.
|
|
||
| unit-linux: | ||
| name: unit tests (linux) | ||
| name: unit tests (linux, ${{ matrix.arch }}) |
There was a problem hiding this comment.
nit: The check rename is recorded only in the PR body, not in ci.yml. This file already carries an "ACTION REQUIRED IN BRANCH PROTECTION" block at lines 363-388 for exactly this situation, with the reasoning that "nothing in this file can read the current setting ... so this note is the only place the handover can be written down". Failure: unit tests (linux) stops reporting and unit tests (linux, amd64) / unit tests (linux, arm64) start; a required-check rule still naming the old one waits forever on a check no run will report, and after merge the PR body is no longer where anyone looks. Evidence: .github/workflows/ci.yml:214 renames the check; .github/workflows/ci.yml:363-388 is the repo's own precedent for recording a rename in-file. Fix: a short note above unit-linux giving the old and the two new names.
There was a problem hiding this comment.
This is not required. The file is just a few hours old.
ananos
left a comment
There was a problem hiding this comment.
thanks @cmainas, looks good!
The arm64 checksum matches the published one. The archive.Apply path is the same in v1.7.36 and v2.3.5: two boolean rewrites and a comment. Only DCO sign-off is a required check on main, so renaming unit tests (linux) needs no branch protection update.
Heads-up: this conflicts with #127 in main_linux.go and docs/build.md, so whichever lands second needs a quick rebase.
UnpackLayers was the last import of the containerd v1 module in hull, while the Linux client already uses v2. The archive.Apply extraction path, including the chown in tar_unix.go, is the same in v1.7.36 and v2.3.5. v1 stays in go.mod as an indirect requirement: urunc's pkg/unikontainers still imports github.com/containerd/containerd/mount. Signed-off-by: Charalampos Mainas <cmainas@nofire.ai>
The Linux CLI builds for amd64 and arm64, but unit-linux ran only on an amd64 runner, and lint cross-builds only for linux/amd64, so nothing compiled or tested the Linux code on arm64. unit-linux becomes a matrix over the two architectures, each on a runner of its own architecture, with that architecture's containerd release and checksum. A native runner rather than a cross-build, because only a test run shows the code works there. Refs: #105 Signed-off-by: Charalampos Mainas <cmainas@nofire.ai>
version.go and its tests carried a darwin tag, but nothing in them depends on macOS: they need only the buildinfo the binary carries and the JSON writer both builds share. Untagged, the Linux CLI gets `hull version` and `hull version --json` from the same code. The file's init installs printVersion as urfave's version printer, so `hull --version` on Linux now prints "hull <v>", the line macOS prints, instead of urfave's "hull version <v>". Refs: #105 Signed-off-by: Charalampos Mainas <cmainas@nofire.ai>
bad5cf3 to
015abcb
Compare
Summary
The Linux CLI has a version flag but no
hull version. Its tests run onamd64 only, though it is built for arm64 too. And hull still imports the
containerd v1 module for one function, while its Linux client uses v2. This
PR makes three small changes, each in its own commit. Each commit builds and
passes the tests on its own.
version.goand its tests carried a darwin tag, but nothing in them dependson macOS. Untagged, the Linux CLI gets
hull versionandhull version --jsonfrom the same code as macOS. The file'sinitinstalls hull's version printer, so
hull --versionon Linux now printshull <v>, the macOS line, instead of urfave'shull version <v>.unit-linuxran only on an amd64 runner, andlintcross-builds and lintsonly for linux/amd64, so nothing compiled or tested the Linux code on arm64.
unit-linuxis now a matrix over both architectures, each on a runner ofthat architecture with containerd's release for it. Lint stays amd64 only.
The arm64 test run compiles every package and test, and
go testruns somevet checks. The check name changes from
unit tests (linux)tounit tests (linux, amd64)andunit tests (linux, arm64). If branchprotection requires the old name, it needs updating before merge.
The layer unpacker in
pkg/ociclientnow imports containerd's archivepackage from v2. The code
archive.Applyruns to extract a layer is thesame in v1.7.36 and v2.3.5, including the chown in
tar_unix.gothat thecomments in
unpack.gorely on. The v2 changes are in diff writing(
ChangeWriter), which hull does not use. v1 does not leave the build:urunc's
pkg/unikontainersimportsgithub.com/containerd/containerd/mount,so
go mod tidykeeps v1.7.36 as an indirect requirement until urunc movesto v2.
Related issues
Refs #105
Changes
pkg/ociclient/unpack.go(refactor, its own commit): importsgithub.com/containerd/containerd/v2/pkg/archiveinstead ofgithub.com/containerd/containerd/archive.go.mod:github.com/containerd/containerdv1.7.36 moves from direct toindirect.
.github/workflows/ci.yml(its own commit):unit-linuxruns as a matrixover amd64 on
ubuntu-latestand arm64 onubuntu-24.04-arm, the runnerbuild-images.ymlalready uses. Each run downloads containerd 2.3.5 forits architecture and checks it against the checksum containerd publishes.
fail-fastis off, so a failure on one architecture does not cancel theother.
cmd/hull/version.go,cmd/hull/version_test.go: the darwin build tag isremoved. Nothing else in them changes.
cmd/hull/main_linux.go: registersversionCommand().docs/build.md: listshull versionamong the Linux commands.Checklist
make testpassesmake macosbuilds hull, vz-runner and hvi, if Go, Swift or Rust code changedtest/*.py) for changes touching boot, console, or the run pathdocs/)