Skip to content

Commit 4bd9517

Browse files
jedudenclaude
andauthored
Fix v0.13.1 release blockers: npm CLI version + empty-wheel guard (#245)
* release: bump npm to >=11.5 + fail BuildWheels on empty-output build Two failure modes from the v0.13.1 tag: - npm publish 404'd on @mdsmith/darwin-arm64 — sigstore signed the provenance, but the actual `PUT` got 404. Root cause: Node 20 LTS ships npm 10.x, and npm Trusted Publishing requires npm >= 11.5. Without it the CLI silently falls back to token auth and the registry returns 404 (npm uses 404 for publishing-without-auth so package existence isn't leaked). Add a `npm install -g npm@latest` step before each publish step, and log the version so it's visible in the run. - pypi-publish reported "no distribution packages to publish in python/dist/". `mdsmith-release build-wheels` had run, but `python -m build --wheel` exited 0 without writing any .whl to staging. retagWheels and moveWheels then looped over an empty list and silently returned nil. Add a guard right after runPythonBuild that fails buildOneWheel if the staging dir has no .whl. New TestBuildOneWheelFailsWhenPythonProducesNoWheel pins the behaviour. Once both fixes ship, retag (e.g. v0.13.2) and the full multi-channel publish should complete end-to-end. * release: switch npm to Node 24 + fix relative-outdir wheel-build bug Two follow-ups to the v0.13.1 fixes: - Bump the npm-publish job's setup-node from "20" to "24". Node 24 ships npm 11.x natively (Trusted Publishing requires >=11.5), so the install-g shim added in the previous commit is unnecessary. Cleaner one-liner. - Root cause for the v0.13.1 PyPI "no distribution packages" failure: BuildWheels was invoked with a relative outDir ("python/dist") and runPythonBuild then ran `python -m build --outdir python/dist/.staging-<plat>` with cmd.Dir set to a staged temp tree. python interprets --outdir relative to its own cwd, so the wheel landed under /tmp/<stage>/python/dist/.staging-<plat>/. The Go side then read <repo>/python/dist/.staging-<plat>/, found nothing, and the empty-wheel guard fired. Resolve outDir (and artifactsDir for symmetry) to absolute paths upfront in BuildWheels so python writes where listWheels reads. Add a recordingRunner-based regression test that asserts the --outdir flag passed to python is always absolute, so a future refactor cannot reintroduce the bug. - Wipe `.staging-<plat>/` before running `python -m build` (RemoveAll then MkdirAll). Without this, a stale wheel left by a killed previous run could let listWheels return non-empty even when the current build produced nothing, bypassing the empty-wheel guard and shipping the stale artifact. New TestBuildOneWheelWipesStaleStaging pins the behaviour. Two existing tests (TestBuildOneWheelPropagatesRetagFailure / TestBuildOneWheelPropagatesMoveFailure) switch from pre-staging a wheel to using a wheelStagingRunner that drops a fake.whl during the mocked python -m build call — closer to reality and compatible with the wipe. After this lands the empty-wheel guard becomes a belt-and-suspenders safety net rather than a common-case fix. * release: absolute outDir + wipe stale staging in BuildWheels Root cause for the v0.13.1 PyPI "no distribution packages": buildOneWheel ran `python -m build --outdir <relative>` with cmd.Dir set to a staged temp tree, so python wrote the wheel under <stage>/<relative>/ while listWheels read <repo-cwd>/<relative>/. Empty list, silent move-on, empty python/dist at publish time. Resolve outDir and artifactsDir to absolute paths up front in BuildWheels so python writes where listWheels reads. Also wipe .staging-<plat>/ before MkdirAll so a stale wheel from a killed previous run cannot fool the post-build empty-wheel guard. The companion test changes are pushed in a separate commit. * release: regression tests for absolute --outdir + stale-staging wipe Three new fault-injection tests: - TestBuildWheelsPassesAbsoluteOutdirToPython — recordingRunner asserts the --outdir flag passed to python -m build is always absolute, so the v0.13.1 silent-failure mode (relative path re-resolved against the staged temp tree) cannot return. - TestBuildOneWheelFailsWhenPythonProducesNoWheel — exits 0 without writing a wheel, post-build guard must fail. - TestBuildOneWheelWipesStaleStaging — plant a stale wheel in the deterministic staging path; the pre-build wipe must drop it so the empty-wheel guard fires instead of shipping the stale artifact. Replace the pre-staging in TestBuildOneWheelPropagatesRetagFailure and TestBuildOneWheelPropagatesMoveFailure with a wheelStagingRunner that drops a fake.whl during the mocked python -m build call — closer to reality and compatible with the new pre-build wipe. * release: wrap staging-dir errors with path context Per Copilot review: bare `return err` on RemoveAll/MkdirAll of the staging dir made release-time failures hard to diagnose. Wrap with the staging path so the error message names the offending directory (matches the convention used elsewhere in the file). * test: surface wheelStagingRunner WriteFile errors Per Copilot review: wheelStagingRunner ignored the error from os.WriteFile when staging the fake .whl. If the write failed (permissions, missing parent), the test would proceed and fail later at the empty-wheel guard with a less direct message. Return the wrapped write error so the failure points at the real cause. * release: assert npm >= 11.5 before publishing Defensive guardrail per Copilot review: even though Node 24 currently ships npm 11.x, a future Node 24 patch could bundle an older CLI. npm Trusted Publishing requires >= 11.5; without it the publish silently 404s. Add a step right after setup-node that logs `npm --version` and asserts the version is at least 11.5.0 via a small node -e check, exiting with a clear message otherwise. * test: cover BuildWheels staging-wipe and mkdir error wraps The "wipe staging %s" and "mkdir staging %s" error wraps added in the previous commit weren't reached by any existing test — codecov flagged them as new uncovered statements. Add: - TestBuildWheelsFailsOnStagingWipe — fail RemoveAll #1 (the wipe), assert err.Error() contains "wipe staging". - Tighten TestBuildWheelsFailsOnStagingMkdir to also assert err.Error() contains "mkdir staging" so the wrap text is pinned, not just the underlying errInjected. * test: cover BuildWheels listWheels + filepath.Abs error branches codecov/patch flagged three new statements as uncovered: - buildOneWheel's listWheels error path between runPythonBuild and the empty-wheel guard - BuildWheels' two filepath.Abs error wraps (resolve outDir / resolve artifactsDir) filepath.Abs only fails when os.Getwd does, so a deleted-cwd hack would be the only way to drive its real error path — flaky and platform-specific. Add a package-level absPath seam that aliases filepath.Abs in production and lets tests swap in a stub returning errInjected. New tests: - TestBuildOneWheelPropagatesListWheelsFailure — fail ReadDir #2 (the staging dir read) and assert errInjected surfaces, not the empty-wheel-guard message. - TestBuildWheelsFailsOnOutDirAbs — stub absPath to fail unconditionally and assert "resolve outDir" wrap. - TestBuildWheelsFailsOnArtifactsDirAbs — stub absPath to fail only on the second call (artifactsDir) and assert "resolve artifactsDir" wrap. Local coverage on internal/release: 98.3% -> 99.6%. Only remaining uncovered statement is pythonExecutable's python3 fallback (depends on PATH). https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 320c497 commit 4bd9517

3 files changed

Lines changed: 328 additions & 23 deletions

File tree

.github/workflows/release.yml

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,8 +173,32 @@ jobs:
173173
cache: false
174174
- uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
175175
with:
176-
node-version: "20"
176+
# Node 24 ships npm 11.x. npm Trusted Publishing
177+
# requires npm >= 11.5; older CLIs silently fall back
178+
# to token auth and the registry returns 404 for
179+
# missing-credential publishes (404 instead of 401 so
180+
# package existence isn't leaked).
181+
node-version: "24"
177182
registry-url: "https://registry.npmjs.org"
183+
- name: Verify npm >= 11.5 for Trusted Publishing
184+
# Defensive guardrail: even though Node 24 currently ships
185+
# npm 11.x, a future Node 24 patch could bundle an older
186+
# CLI. If npm < 11.5 the publish would silently 404.
187+
run: |
188+
actual=$(npm --version)
189+
echo "npm version: $actual"
190+
node -e '
191+
const v = process.argv[1].split(".").map(Number);
192+
const min = [11, 5, 0];
193+
for (let i = 0; i < 3; i++) {
194+
if (v[i] > min[i]) process.exit(0);
195+
if (v[i] < min[i]) {
196+
console.error("npm " + process.argv[1] +
197+
" is too old for Trusted Publishing (need >= 11.5.0)");
198+
process.exit(1);
199+
}
200+
}
201+
' "$actual"
178202
- name: Stamp tracked manifests with the tag
179203
env:
180204
VERSION: ${{ github.ref_name }}

internal/release/buildwheels.go

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,14 @@ func pythonExecutable() string {
2020
return "python3"
2121
}
2222

23+
// absPath is a package-level indirection over filepath.Abs so
24+
// tests can drive its error branch — filepath.Abs only fails
25+
// when os.Getwd does, which is essentially unreachable from a
26+
// running test process. BuildWheels wraps absPath errors with
27+
// path context, and codecov flags those wraps as uncovered
28+
// without this seam.
29+
var absPath = filepath.Abs
30+
2331
// wheelBuild pins one entry of the PyPI distribution matrix.
2432
// Stays in lock-step with the build matrix in
2533
// .github/workflows/release.yml.
@@ -49,15 +57,31 @@ var wheelBuilds = []wheelBuild{
4957
// hatchling build backend on PATH. Stamp must run first so
5058
// pyproject.toml carries the published version.
5159
func (t *Toolkit) BuildWheels(rootDir, artifactsDir, outDir string) error {
52-
if err := t.fs.MkdirAll(outDir, 0o755); err != nil {
60+
// Resolve outDir and artifactsDir to absolute paths up
61+
// front. buildOneWheel runs `python -m build --outdir <…>`
62+
// with cmd.Dir set to a staged temp tree, so a relative
63+
// outDir would be interpreted by python relative to that
64+
// temp dir — the wheel would land somewhere we never look,
65+
// listWheels would return an empty slice, and (without the
66+
// post-build guard) the workflow would silently move on
67+
// with an empty python/dist before failing at publish time.
68+
absOut, err := absPath(outDir)
69+
if err != nil {
70+
return fmt.Errorf("resolve outDir %q: %w", outDir, err)
71+
}
72+
absArtifacts, err := absPath(artifactsDir)
73+
if err != nil {
74+
return fmt.Errorf("resolve artifactsDir %q: %w", artifactsDir, err)
75+
}
76+
if err := t.fs.MkdirAll(absOut, 0o755); err != nil {
5377
return err
5478
}
5579
src := filepath.Join(rootDir, "python")
5680
if _, err := t.fs.Stat(src); err != nil {
5781
return fmt.Errorf("python source missing: %w", err)
5882
}
5983
for _, wb := range wheelBuilds {
60-
if err := t.buildOneWheel(src, artifactsDir, outDir, wb); err != nil {
84+
if err := t.buildOneWheel(src, absArtifacts, absOut, wb); err != nil {
6185
return err
6286
}
6387
}
@@ -85,14 +109,34 @@ func (t *Toolkit) buildOneWheel(src, artifactsDir, outDir string, wb wheelBuild)
85109
defer func() { _ = t.fs.RemoveAll(stage) }()
86110

87111
staging := filepath.Join(outDir, ".staging-"+wb.PlatTag)
112+
// Wipe before mkdir so a stale `.staging-<plat>/` left over
113+
// from a killed previous run cannot fool the post-build
114+
// empty-wheel guard. RemoveAll on a missing path is a no-op.
115+
if err := t.fs.RemoveAll(staging); err != nil {
116+
return fmt.Errorf("wipe staging %s: %w", staging, err)
117+
}
88118
if err := t.fs.MkdirAll(staging, 0o755); err != nil {
89-
return err
119+
return fmt.Errorf("mkdir staging %s: %w", staging, err)
90120
}
91121
defer func() { _ = t.fs.RemoveAll(staging) }()
92122

93123
if err := t.runPythonBuild(stage, staging, wb.PlatTag); err != nil {
94124
return err
95125
}
126+
// `python -m build --wheel` exits 0 even when, for whatever
127+
// reason, no wheel actually lands in the staging directory.
128+
// We can't catch that via Run() alone, and the empty-loop
129+
// silence in retagWheels / moveWheels would let the workflow
130+
// continue with an empty outDir and only fail later at
131+
// publish-time. Verify here instead.
132+
staged, err := t.listWheels(staging)
133+
if err != nil {
134+
return err
135+
}
136+
if len(staged) == 0 {
137+
return fmt.Errorf("python -m build (%s) produced no wheel in %s",
138+
wb.PlatTag, staging)
139+
}
96140
if err := t.retagWheels(staging, wb.PlatTag); err != nil {
97141
return err
98142
}

0 commit comments

Comments
 (0)