fix(gh-r): detect musl from its loader, not from curl (#576) - #581
Conversation
.zi-get-latest-gh-r-url-part set HAS_MUSL to linux-musl whenever curl was installed, and its fallback probe could never be false: find exits 0 whether or not anything matches. On a glibc host every release that publishes both builds therefore resolved to the musl asset. Detect musl by globbing /lib for its loader instead. A regression test serves a gnu and a musl asset offline and checks that installing curl does not change the choice, and that a host with no musl loader picks the glibc build. Closes #576
ss-o
left a comment
There was a problem hiding this comment.
Fallback review under ADR-0026: Copilot request not registered on 15f1e70
Findings
Changes needed, minor only. The fix is correct and meets #576. There are no blockers and no majors. Four small findings on the test and on how precise the detection is will be fixed in one push. Each was rerun by the author's session on this head.
- Minor,
tests/gh-r-musl-detection.zsh:57: the "glibc build must win" claim depends on asset order. On glibc,HAS_MUSLis now$MACHTYPE, which matches both assets, so the first listed asset wins. Rerun: gnu first picks gnu, musl first picks musl. The assertion still separates old from new code, because the old code narrowed to musl in either order. What it proves is that the musl filter no longer fires, not that glibc is preferred. Remedy: state that in the comment and the PR body. Actively preferring glibc is outside #576. - Minor,
lib/zsh/install.zsh:1656:/lib/*musl*is broader than its comment. It also matches amusldirectory, such as a musl toolchain under/usr/libreached through a symlinked/lib, which makes a glibc host look like musl. Rerun: a fake/libholdingmusl/andld-musl-x86_64.so.1gives 2 matches for*musl*and 1 forld-musl-*. Remedy: globld-musl-*. - Nit,
tests/gh-r-musl-detection.zsh:16: the function under test is defined only as a side effect..zi-prepare-homesourcesinstall.zshonly when the home is new. Remedy: sourcelib/zsh/install.zshexplicitly, astests/hook-ownership.zshdoes. - Nit,
tests/gh-r-musl-detection.zsh:28: stale comment. It says the function runsfind, which it no longer does. Remedy: dropfindfrom the comment and the linked tools.
Scope: commit 15f1e70, the complete diff against next (85214aa), for #576.
Note
This is the maintainer-elected fallback review of record, based on a separate read-only review session and on reruns by the author's session. It is not an independent human review, and it is not approval to merge.
Validation
- Passed, hosted on this head: 142 of 142 check runs.
- Passed, local on this head: the new test; all 39
tests/*.zsh;zsh -nonzi.zshandlib/zsh/*.zsh; Trunk on the changed files; 3 of 3 hand mutants killed. - Measured:
find /lib/ -maxdepth 1 -name '*musl*'exits 0 with no output on a glibc host, and exits 1 only when/libis missing.
Checklist verdicts
- Acceptance (#576): met. The curl operand is gone, the always-true
findis replaced, and the:1710question is answered by measurement (a harmless no-op on real names).Closes #576links the Development sidebar; the issue is closed by hand at promotion. - Zsh quality: good. Runs under
emulate -LR zshwithextended_glob;musl_libsis local;(N)keepsnomatchsafe. - Public contract, loading, filesystem: not affected. Both callers read only
reply; no new writes or network access; a subprocess is replaced by a glob. - Test hermeticity: good.
mktemproot with a trap, a temporary HOME and ZI directories, a stubbed download, and a minimalPATHper case. On a musl host the glibc assertion is skipped; CI runs on glibc.
Limits and follow-up
Not measured on a real musl host or macOS. A new fallback review follows on the fixed head. Merging needs the maintainer's decision.
Review of the first head found that /lib/*musl* also matched a musl toolchain directory, which makes a glibc host with a musl toolchain look like musl. Detection now globs ld-musl-* through a small helper, .zi-has-musl-loader, so it can be tested against fixture directories. The regression test sources install.zsh explicitly, checks the helper against a loader, a toolchain directory and an empty directory, and checks that a musl host picks the musl build even when the glibc build is listed first. Its comments no longer claim that glibc is actively preferred: without a musl loader the musl filter simply does not apply, and the asset order decides between two matching builds. Refs #576
ss-o
left a comment
There was a problem hiding this comment.
Fallback review under ADR-0026: Copilot request not registered on d69ea82
Findings
No blocker or major findings. The change meets #576, Closes #576 is justified (closed by hand at promotion, per AGENTS.md), and the four round-1 threads are fixed on this head. One minor wording point is fixed in the PR body without a new commit; two nits are left with the dispositions below.
- Minor, fixed in the PR body: "absent on glibc" has an exception. A glibc host with a musl runtime installed (Debian's
muslpackage installs/lib/ld-musl-<arch>.so.1) is detected as musl and prefers musl builds, which run there because the loader exists. The body now says so; changing what is detected (the shell's own C library) is outside #576. From a web source, not measured here. - Nit, left as is:
tests/gh-r-musl-detection.zsh:58stubs the helper without checking its argument, so a caller probing the wrong directory would pass the musl-host case. The helper's own fixture checks and the glibc assertion still pin the behaviour on CI; tightening the stub is optional. - Nit, left as is: the
INT TERMtrap cleans up without exiting. It is the pattern 31 other tests intests/use.
Scope: commit d69ea82, the complete diff against next (85214aa), for #576.
Note
This is the maintainer-elected fallback review of record, based on a separate read-only review session and on reruns by the author's session. It is not an independent human review, and it is not approval to merge.
Validation
- Passed, hosted on this head: 142 check runs succeeded; 6 cancelled runs belong to the superseded push, and each of those checks also has a passing run.
- Passed, local on this head: the new test; all 39
tests/*.zsh, includingci-registration.zshandsource-hygiene.zsh;zsh -n; Trunk on the changed files. - Mutants: 5 of 5 killed by the author's session, and 5 of 5 killed again by the reviewer (curl operand,
findprobe,*musl*helper, detection ignored, detection forced true). - Measured:
findexits 0 with no match and 1 only for a missing directory;ld-musl-*ignores amusl/toolchain directory and a barelibc.musl-*.
Checklist verdicts
- Acceptance (#576): met, including the
:1710question (a no-op on real archive names). - Zsh quality: good. The helper runs under
emulate -LR zshwith a local array and(N); safe under a caller'sglob_subst; no global leaks. - Public contract, loading, filesystem: not affected. Callers read only
reply; no writes or network access. - Test: hermetic and not vacuous on CI's glibc runner.
Limits and follow-up
Not measured on a real musl host or macOS. Merging needs the maintainer's decision.
Description
.zi-get-latest-gh-r-url-partdecides whether to prefer musl release assets withHAS_MUSL. That value was set tolinux-muslon almost every host, for two reasons:(( ${+commands[curl]} ))made any host with curl count as musl, as fix(gh-r): musl detection is true whenever curl is installed #576 reports.find /lib/ -maxdepth 1 -name '*musl*', can never be false:findexits 0 whether or not anything matches. Measured on a glibc host with no/lib/*musl*, it returns 0 with empty output.So removing only the curl operand would not have fixed the bug. With both causes present, a
gh-rinstall on glibc picked the musl build of every release that publishes both. The fix detects musl through a small helper,.zi-has-musl-loader, which globs/libfor the musl loader (ld-musl-<arch>.so.1). That file is absent on glibc and macOS unless a musl runtime is installed alongside (Debian'smuslpackage, for example, installs/lib/ld-musl-<arch>.so.1); such a host then prefers musl builds, which run there because the loader exists. Detecting the C library the shell itself uses would be a separate change. It globsld-musl-*rather than*musl*, so a musl toolchain directory on a glibc host is not mistaken for a musl host.Without a musl loader, the musl-preference filter no longer applies. When two builds both match the host, the asset order decides between them, as it does for every other filter in the function. Actively preferring glibc would be a separate change.
The issue also asks whether the anchored filter at
install.zsh:1710(*/$~HAS_MUSL) matches a real asset. Measured: it only matches an asset whose whole file name islinux-muslor the bare$MACHTYPE, so it is a no-op on real archive names. It is harmless, because it only narrows the list when something matches, and it is left unchanged here.tests/gh-r-musl-detection.zshruns offline and makes three checks:curlonPATHdoes not change the chosen asset, and a Linux host without a musl loader does not narrow to the musl build.It is registered in
zsh-n.yml, astests/ci-registration.zshrequires.Related issues
Closes #576
Type of change
fix- bug fix (non-breaking)feat- new feature (non-breaking)feat!/fix!- breaking changeperf- performance improvementrefactor- code change with no functional impactdocs- documentation onlytest- test addition or correctionbuild- build system or dependency changeci- CI/workflow changestyle- formatting with no behavior changechore- maintenance / dependency bumprevert- revert of an earlier changeChecklist
next; only same-repository hotfixes targetmainnexttomainpromotion uses a merge commit and records both parent SHAs (not a promotion)Co-authored-bytrailer credits a real human, never a bot, AI agent, or automation (none)zsh -n zi.zsh/ Trunk checks)Verification
All runs are local on
next85214aaplus this branch, with zsh 5.9.2 on glibc 2.44 Linux x86_64.glibc host chose ...-linux-musl.tar.gz, andHAS_MUSL=linux-muslshowed in a trace with no curl onPATH.findprobe restored; the first head's*musl*glob; detection forced true; detection forced false; detection result ignored.zsh -nonzi.zshandlib/zsh/*.zsh: pass.rpm2cpio.zsh:54printsredirection with no commandwith status 0, onnextas well.tests/*.zshpasses,ci-registration.zshincluded (39 tests).self-update-reload.zshandversion-reporting.zshneed a Git checkout, so they ran in the worktree.Migration plan
Not applicable: no public contract changes.