Skip to content

ci: catch cross-platform build breaks before release#433

Merged
lizhengfeng101 merged 1 commit into
alibaba:mainfrom
chethanuk:ci/cross-compile-guard
Jul 22, 2026
Merged

ci: catch cross-platform build breaks before release#433
lizhengfeng101 merged 1 commit into
alibaba:mainfrom
chethanuk:ci/cross-compile-guard

Conversation

@chethanuk

@chethanuk chethanuk commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Description

CI builds one platform. ci.yml:52 is go build -o /dev/null ./cmd/opencodereview inside
container: golang:1.26.5, i.e. linux/amd64. release.yml:15-29 cross-compiles six, and only
fires on push: tags: ['v*'].

That gap has teeth because the repo carries platform-gated source CI never compiles:

cmd/opencodereview/shell_windows.go:1     //go:build windows
cmd/opencodereview/procattr_windows.go:1  //go:build windows
cmd/opencodereview/shell_unix.go:1        //go:build !windows
cmd/opencodereview/procattr_unix.go:1     //go:build !windows

A signature change to shellCommand or configureProcessGroup compiles green on main and
breaks at tag time, after the tag is already public. All six targets build clean today, so this
is a missing guard rather than a live bug.

This adds a cross-compile matrix job covering the five targets test does not. linux/amd64
is excluded because vet and test -race already compile all 23 packages there.

Verification

A demonstration of the gap. Appending var _ = thisSymbolDoesNotExist to shell_windows.go:

Gate Exit
go build -o /dev/null ./cmd/opencodereview — today's Build step 0
go build -o /dev/null ./... — linux/amd64, whole tree 0
go vet ./... 0
GOOS=windows GOARCH=amd64 go build ./... — new leg 1
GOOS=darwin GOARCH=arm64 go build ./... — new leg 0, correctly unaffected

The first three rows are the argument: a break that ships broken binaries is invisible to every
check this repo runs today.

The job needs the same Trust workspace step as test: go build stamps VCS metadata into main
packages, and git refuses a dubiously-owned checkout with error obtaining VCS status: exit status 128. I did not reach for -buildvcs=false, since the repo already solves this the other way and
matching it keeps stamping consistent with the release builds.

On cost, this PR's own checks answer it: the five legs ran on your self-hosted pool in
15-16 seconds each, in parallel. That is cheaper than release.yml:11-29, which already runs
a near-identical 6-way matrix on the same pool at roughly 35s per leg.

Also in this PR: gofmt and go mod tidy gates

Two steps added to the existing test job in ci.yml. Both are gates the repo cannot currently
enforce, because the Makefile targets mutate rather than fail:

  • gofmt drift. CI runs go vet only; make fmt rewrites files and never fails.
  • go mod tidy drift. No check exists. Makefile:67 runs tidy but mutates.

Both print their remedy via ::error:: so it lands as a PR annotation. I falsified both rather
than trusting a clean tree: an unformatted file trips the first, and importing
github.com/atotto/clipboard (currently indirect) forces tidy to promote it and trips the second.
gofmt -s -l . is empty today, so -s adds no churn.

Limitations

I could not run the job on self-hosted runners before opening this, since my fork has none, so
I approximated by building in the same image as root over a foreign-owned checkout — which is
what caught the VCS-stamping failure. That gap is now closed: the checks on this PR are the job's
first real execution, and all five legs pass.

The tidy gate reaches proxy.golang.org, so a proxy outage turns a green PR red. ci.yml:33
already has that property, so it adds no new class of failure. timeout-minutes: 20 on the new
job is a guess rather than a measurement.

One divergence worth naming: the gate uses gofmt -s but Makefile:61 runs $(GO) fmt, which
has no -s. They agree on today's tree. I left the Makefile alone deliberately.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • Mutation test: injected a Windows-only break; every existing gate stayed green while both
    Windows legs failed and named the file and line
  • Confirmed excluding linux/amd64 leaves no gap (go vet ./... exits 1 on a non-cmd break)
  • Falsified both new gates — each fails on injected drift and prints its remedy
  • Confirmed the job fails EXIT=1 on VCS stamping without Trust workspace, EXIT=0 with it
  • Cross-compiled all six targets locally with a cold GOCACHE — all clean today
  • actionlint v1.7.12 — exit 0
  • All five cross-compile legs pass on this PR's own checks, on the real self-hosted pool

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective — n/a, workflow config
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (not applicable)
  • I have signed the CLA

AI assistance: Claude Code helped research and verify this change. I reviewed the full diff
and take responsibility for it.

CI builds one platform (linux/amd64) while release.yml ships six. The
repo has platform-gated source - shell_windows.go, procattr_windows.go
and their _unix twins - that CI never compiles, so a signature change to
shellCommand or configureProcessGroup compiles green on main and breaks
at tag time, after the tag is public.

Add a cross-compile matrix job covering the five targets the test job
does not, mirroring the shape release.yml:11-29 already runs on this
same runner pool. fail-fast: false so a failure names the target.

The job trusts the workspace the same way the test job does. The
container runs as root over a checkout owned by another uid, so git
reports dubious ownership and Go's VCS stamping - which runs when
building main packages - fails the build with "error obtaining VCS
status: exit status 128". Marking the checkout safe keeps stamping on
and consistent with the test job and the release builds, rather than
papering over it with -buildvcs=false.

Also add two cheap gates the repo lacked: gofmt -s drift (make fmt
rewrites and can never fail CI) and go mod tidy drift (make check
mutates, so it cannot gate either). Both print the remedy.

All six targets build clean today - this is a missing guard, not a fix.
@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 2 issue(s) in this PR.

  • ✅ Successfully posted inline: 2 comment(s)

Comment thread .github/workflows/ci.yml
Comment on lines +80 to +85
include:
- {goos: linux, goarch: arm64}
- {goos: darwin, goarch: amd64}
- {goos: darwin, goarch: arm64}
- {goos: windows, goarch: amd64}
- {goos: windows, goarch: arm64}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The cross-compile matrix is missing linux/amd64. While the test job builds for the native platform (presumably linux/amd64), having it in the cross-compile matrix ensures consistent coverage and makes the matrix self-contained. If the intent is to rely on the test job for linux/amd64, consider adding a comment explaining why it's excluded here.

Comment thread .github/workflows/ci.yml
Comment on lines +92 to +97
- name: Build ${{ matrix.goos }}/${{ matrix.goarch }}
env:
GOOS: ${{ matrix.goos }}
GOARCH: ${{ matrix.goarch }}
CGO_ENABLED: '0'
run: go build -o /dev/null ./...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The cross-compile job runs go build which will download dependencies each time on self-hosted runners. Consider adding Go module caching (e.g., using actions/cache for the Go module cache directory) to speed up builds and reduce network usage, similar to what you might want for the test job as well.

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@lizhengfeng101
lizhengfeng101 merged commit a4c9f2e into alibaba:main Jul 22, 2026
8 checks passed
lizhengfeng101 added a commit that referenced this pull request Jul 22, 2026
…ut (#439)

- Change Makefile fmt/check targets from `go fmt` to `gofmt -s -w .`
  to match the CI gate introduced in #433, preventing drift where
  local `make fmt` passes but CI fails on simplification opportunities.

- Reduce cross-compile job timeout from 20 to 10 minutes, since each
  leg completes in ~16 seconds on self-hosted runners.
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.

2 participants