fix: resolve Linux package output path before cd-ing into build dir#2548
fix: resolve Linux package output path before cd-ing into build dir#2548
Conversation
The first successful develop snapshot run (25637518317) published all
zip/tar.gz artifacts to wheels-dev/wheels-snapshots correctly, but the
.deb (54 MB) and .rpm (56 MB) were missing. Build log line 4453 shows
nfpm wrote them inside .linux-pkg-build/artifacts/wheels/${VERSION}/
instead of the repo-root artifacts/wheels/${VERSION}/.
Root cause: build-linux-packages.sh did `cd "${BUILD_DIR}"` BEFORE
resolving `NFPM_OUT="$(pwd)/${OUT_DIR}"`. After the cd, `pwd` was
.linux-pkg-build/ and OUT_DIR (workflow passes a repo-root-relative
path) was re-rooted under .linux-pkg-build/.
GitHub Release upload then silently matched nothing — softprops/
action-gh-release doesn't fail on empty globs.
Fix: resolve OUT_DIR to absolute path BEFORE the cd. The cd is still
needed so nfpm can find the staged content (./build/lucli, ./build/
module/, etc.) referenced relatively in the nfpm config.
Without this, Tuesday's v4.0.0 GA release would also be missing its
.deb / .rpm packages — same nfpm step runs on main.
ae90e73 to
4b128ac
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is a minimal, correct fix for a real path-resolution bug that caused .deb/.rpm packages to land in the wrong directory, silently omitting them from GitHub Releases. The root cause, fix, and evidence are all clearly documented. I have one minor comment about inline comment hygiene, and one non-blocking suggestion for defence-in-depth in the workflow.
Correctness
The fix is sound. Walking through the logic:
Before:
cd "${BUILD_DIR}" # pwd is now <repo>/.linux-pkg-build
mkdir -p "${OUT_DIR}" # creates .linux-pkg-build/artifacts/wheels/VERSION/ (wrong)
NFPM_OUT="$(pwd)/${OUT_DIR}" # resolves to .linux-pkg-build/artifacts/… (wrong)After:
mkdir -p "${OUT_DIR}" # creates <repo-root>/artifacts/wheels/VERSION/
NFPM_OUT="$(cd "${OUT_DIR}" && pwd)" # absolute path captured before any cd
cd "${BUILD_DIR}" # now safe — NFPM_OUT is already absoluteThe subshell $(cd … && pwd) correctly leaves the parent shell in the repo root. The --config "../${NFPM_CONFIG}" path in the nfpm invocations still resolves correctly after cd "${BUILD_DIR}" because BUILD_DIR is already captured as an absolute path at line 29 (BUILD_DIR="$(pwd)/.linux-pkg-build"). All edge cases (pre-existing OUT_DIR, absolute OUT_DIR override) work correctly; set -euo pipefail ensures failures surface immediately.
Conventions
Minor nit — inline comment references a specific run ID (tools/distribution-drafts/linux-packages/build-linux-packages.sh, line 153):
# (Yes, this was a real bug. Run 25637518317.)Per CLAUDE.md: "Don't reference the current task, fix, or callers … since those belong in the PR description and rot as the codebase evolves." The build log link will stop being meaningful in six months. The rest of the comment block is genuinely valuable — the ordering constraint (resolve BEFORE cd) and the reason the cd is still required are non-obvious. Suggest trimming that one parenthetical sentence.
Docs
Non-blocking suggestion — add fail_on_unmatched_files: true to the softprops step (.github/workflows/release.yml):
The current workflow configuration has no guard against empty glob matches, which is what allowed this bug to surface as a silent 18-asset release instead of a hard CI failure. Neither the stable nor snapshot softprops invocations set this option. Adding it would make the next path-resolution regression fail loudly rather than silently ship an incomplete release. This is a separate PR concern, not a blocker here — just worth noting while this area is being touched.
Commits
Commit ae90e734f — fix: resolve Linux package output path before cd-ing into build dir
Valid type (fix), no scope (appropriate — no clean scope in the allowlist maps to CI build tooling), subject under 100 chars, not ALL-CAPS. Commit body is thorough and explains the why. Passes commitlint.
Tests
No tests, as expected. This is a CI build script; testing it requires a real nfpm binary and artifact bundle. The fix was validated by the failing run documented in the PR body.
Overall: the fix is correct, the PR is well-documented, and the commit is clean. The inline run-ID reference is a minor cosmetic issue, and the fail_on_unmatched_files suggestion is a follow-up quality-of-life improvement. Neither is a blocker.
Wheels Bot — Reviewer B (round 1)A's review is substantively correct. The fix logic is sound, the nits are real but non-blocking, and the verdict (COMMENTED rather than blocking changes) is consistent with the findings. One bookkeeping error in A's own marker is worth flagging. SycophancyNone detected. A walked through the before/after logic step by step before concluding the fix is correct. The positive framing is evidence-backed, not reflexive. False positivesA cited the wrong commit SHA in the Commits section. A wrote:
The actual HEAD commit is The review's Missed issuesNone detected. The diff is bash-only; no CFML, no cross-engine concerns. A correctly identified the one code-comment CLAUDE.md violation (the run ID parenthetical) and the one workflow-level defence-in-depth gap ( Verdict alignmentA filed the review as COMMENTED with two non-blocking findings. That verdict is consistent with the findings — neither issue warrants blocking the merge. ConvergenceAligned. A's analysis of the fix is correct, both findings are real, and the non-blocking classification is appropriate. The commit SHA mismatch in A's marker is a self-referential error that does not affect review content. Joint recommendation: approve and merge. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is the same correct fix from the previous review cycle, rebased/force-pushed to a new SHA. The fix itself is sound. The one minor nit raised in the prior review — the inline run-ID comment — has not been addressed. No new issues found.
Correctness
The fix is correct and the logic is unchanged from the previous review.
Before the fix, cd "${BUILD_DIR}" ran first so pwd pointed inside .linux-pkg-build/, causing NFPM_OUT to resolve there. The GitHub Release upload glob (repo-root-relative) matched nothing and softprops/action-gh-release silently no-oped.
After the fix, mkdir -p "${OUT_DIR}" and NFPM_OUT="$(cd "${OUT_DIR}" && pwd)" run while still in the repo root, capturing the absolute path in a subshell that leaves the parent shell unaffected. The cd "${BUILD_DIR}" follows safely.
set -euo pipefail ensures a failed mkdir or cd in the subshell aborts immediately. The --config "../${NFPM_CONFIG}" paths in subsequent nfpm invocations still resolve correctly because BUILD_DIR is itself absolute (captured earlier as $(pwd)/.linux-pkg-build).
Conventions
Unresolved nit from prior review — inline run-ID reference in tools/distribution-drafts/linux-packages/build-linux-packages.sh:
# (Yes, this was a real bug. Run 25637518317.)Per CLAUDE.md: "Do not reference the current task, fix, or callers ... since those belong in the PR description and rot as the codebase evolves." The CI run link will be opaque in six months and adds no value to a reader understanding the ordering constraint. The rest of the comment block is valuable — only this sentence should be trimmed.
This was raised in the previous review cycle (against SHA ae90e73) and is still present in the force-pushed commit. Not a blocker, but please remove it before merge.
Commits
fix: resolve Linux package output path before cd-ing into build dir — type fix, no scope (CI tooling does not map cleanly to the commitlint scope allowlist), subject under 100 chars, not ALL-CAPS. Commit body explains the root cause. Passes commitlint.
Minor note: the commit body says "Tuesday's v4.0.0 GA release" — a day-of-week reference that will lose meaning in the git log within days. Non-blocker, but prefer an absolute version string.
No correctness, cross-engine, security, or test-coverage findings. Ready to merge once the run-ID sentence is removed from the inline comment.
Documents the new bleeding-edge channel (wheels-be) alongside stable (wheels). Covers: - Comparison table: package name, source repo, cadence, version shape, channel-aware --version output - Install steps for each channel on macOS / Windows / Linux - Switching channels (uninstall one, install the other — they're mutually exclusive via brew's conflicts_with) - "Already-scaffolded apps are unaffected" reassurance - Decision guide: when to pick stable vs bleeding-edge - "How channels work under the hood" optional section explaining the snapshot version format, where snapshots live (wheels-snapshots repo), and the auto-update propagation chain Also adds a "Choose a channel" section to installing.mdx pointing at the new page so first-time installers see the option before they brew install. Doesn't disrupt the happy path: stable remains the default and the install steps below the new section are unchanged. Sidebar order: release-channels.mdx is order 5, after the tutorial happy path (installing → first-15-minutes → tutorial), so users new to Wheels don't get distracted by channel discussion before they've seen the framework run. Doubles as a soft-launch validation: this PR's merge to develop will fire the snapshot pipeline that just went live (PR #2545, #2547, #2548 + homebrew-wheels#174). If the dispatch chain works end-to-end, this commit's snapshot will land at wheels-dev/wheels-snapshots and the brew tap's bleeding-edge-update.yml will auto-bump wheels-be.rb to the new version within ~5 minutes.
Summary
The first successful develop snapshot publish (run 25637518317) landed at https://github.com/wheels-dev/wheels-snapshots/releases/tag/v4.0.0-snapshot.1786 with 18 assets ✅ — but the .deb and .rpm Linux packages are missing from the asset list. This PR fixes the path-resolution bug that caused them to land in the wrong directory.
Root cause
build-linux-packages.shlines 147-149 (pre-fix):OUT_DIRis documented as repo-root-relative (workflow passesartifacts/wheels/${WHEELS_VERSION}). But the scriptcds into${BUILD_DIR}(which is absolute,${repo}/.linux-pkg-build) BEFORE resolvingNFPM_OUT. After the cd:pwdreturns${repo}/.linux-pkg-buildmkdir -p "${OUT_DIR}"creates${repo}/.linux-pkg-build/artifacts/wheels/${VERSION}/(wrong place)NFPM_OUTbecomes${repo}/.linux-pkg-build/artifacts/wheels/${VERSION}(wrong place)artifacts/wheels/${VERSION}/wheels_*_amd64.deb(repo-root-relative) matches nothingBuild log line 4453 confirms:
Fix
Resolve
OUT_DIRto absolute path BEFORE thecd. Thecdis still required so nfpm can find staged content referenced relatively in the nfpm YAML config (./build/lucli,./build/module/, etc.).Why this matters for Tuesday
Stable (main) runs the same nfpm step. Without this fix,
v4.0.0GA would also be missing its.deb/.rpmLinux packages — Linux users would have no install path on day one.Refs
#2545 (introduced the bug —
build-linux-packages.sh)#2547 (fixed an unrelated URL bug in the same script)