Install rumdl from GitHub Releases instead of pip (fixes #709)#712
Conversation
Downloads the prebuilt binary for the runner's OS/arch, verifies its sha256 checksum, and invokes it directly, per rvben's direction on rvben#709: prefer *-unknown-linux-musl on Linux, and resolve "latest" via the unauthenticated releases/latest redirect instead of the REST API (rate-limited across the shared NAT of GH Actions runners). Falls back to the previous pip install only for the 3 cases where no binary can be reliably resolved: 404 on the release asset, an unmapped OS/arch, or a releases/latest redirect that doesn't parse to a tag. A checksum mismatch always hard-fails instead of falling back, since that signals a corrupted or tampered download rather than an unsupported platform. Lint logic (lines 17+ previously) is unchanged except for invoking the resolved rumdl_cmd path instead of the bare `rumdl` name, so the fallback and binary paths share the same call site.
rvben
left a comment
There was a problem hiding this comment.
Thanks for this, and for the level of rigor here. The fork validation runs plus the root-cause on the fixture breakage made this easy to review. I checked the runs myself: at this PR's head commit the Windows job resolves v0.2.30, verifies the checksum, and runs the downloaded binary, with only the four pre-existing fixture jobs failing, identical to unmodified main. I've approved CI on this PR so the matrix runs here too.
One change I need before merging, a couple of hardening suggestions, and a plan for the fixture fix.
Must fix: gate the binary path on curl being available
try_install_binary assumes curl exists. Today the action requires pip, so running it inside a python-only container (e.g. container: python:3.12-slim, which has pip but no curl) is a supported setup. After this change that setup hard-fails at the latest-release resolution with "Could not reach GitHub (curl exit 127)", which reads as a connectivity problem rather than what it is. A command -v curl check before attempting the binary path, falling back to pip when absent, keeps those users working.
Should fix: check mktemp/cd explicitly
Because try_install_binary is invoked as if ! try_install_binary ..., bash suppresses set -e for the entire function body, so errors only stop execution where you check explicitly. You did that everywhere except workdir=$(mktemp -d) and cd "$workdir". If either fails, the subshell keeps going in the workspace directory: the archive downloads into the user's repo and tar extracts a stray rumdl binary there, and $rumdl_cmd then points at a path that doesn't exist. || exit 1 on both lines closes it.
Nits, take or leave
- The published Windows
.zip.sha256ends in CRLF (I checked the v0.2.30 asset bytes). Git Bash's awk strips the\r, which is why your Windows run passed, but BSD awk keeps it (reproduced on macOS), so a self-hosted Windows runner with different tooling could hard-fail the comparison.tr -d '\r'in the expected-hash pipeline makes it robust. /c/Windows/System32/tar.exeassumes the system drive is C:, which holds on hosted runners but not necessarily self-hosted ones. An existence check with a PowerShellExpand-Archivefallback would cover it.
The fixture breakage
Your diagnosis is right; I reproduced it (rumdl 0.2.30 passes the current bad.md clean, and the Test Action run on unmodified main fails exactly the four jobs you listed). Please open your fix/restore-malformed-fixture branch as its own PR, and while you're in there, add __tests__/fixtures/** and .github/workflows/test-action.yml itself to the paths filters in test-action.yml; that gap is why the breakage sat unnoticed since April. I'll merge that first so this PR can rebase and go fully green.
Really appreciate the choice to hard-fail on checksum mismatch instead of falling back to pip, and the exit-code protocol distinguishing graceful fallback from hard failure. This is exactly the shape I hoped this fix would take.
Address rvben's changes-requested review on the binary install path: gate on curl availability with a pip fallback, check mktemp/cd exit codes explicitly (set -e is suppressed inside the if-negated function), strip CRLF from the published Windows checksum before comparing, and fall back to Expand-Archive when tar.exe isn't at the expected path.
|
Really glad this is getting a proper look. I went through your review carefully and did a bit of independent digging on the bash/Docker/awk specifics before agreeing with everything, this whole install-by-binary approach is pretty new territory for me so I like to double check rather than just trust my first instinct. Honestly the curl thing might be the one that stings a little, in hindsight it's such an obvious gap that I'm a bit embarrassed I didn't catch it myself haha. Anyway, here's what changed: Added a Added Added Added an existence check on tar.exe with an Ran shellcheck over the whole file too, no warnings. Will open the fixture-fix branch as its own PR like you suggested. Keeping an eye out for anything else, have a good one. |
rvben
left a comment
There was a problem hiding this comment.
All four points addressed, and I verified the hardening commit: the curl gate falls back to pip (your python:3.12-slim test was exactly the scenario I was worried about), mktemp/cd abort explicitly now, the CRLF strip matches the real v0.2.30 asset bytes, and the Expand-Archive branch you couldn't test is safe by construction: extraction is the subshell's last command, so a failure propagates to the hard-exit path instead of continuing. The remaining Test Action failures here are the pre-existing fixture ones; I'll update this branch after #713 merges and re-run, no action needed from you. Thanks for the fast, careful turnaround.
Closes #709.
Following up on #709:
scripts/rumdl-action.shnow downloads the prebuilt binary for the runner's OS/arch straight from Releases (checksum verified) instead of going through pip, so anyone using this Action doesn't need Python in their CI image just for this.I followed the two things called out in the issue:
*-unknown-linux-muslon Linuxreleases/latestredirect instead of the REST API, to stay clear of the rate limit shared across Actions runnerspip is still there, but only as a fallback for the cases where the binary path genuinely can't work:
A checksum mismatch never falls back to pip though, that's a hard failure, since it points at a corrupted or tampered download rather than an unsupported platform.
While validating this on a fork, I ran into something worth flagging separately:
test-action.yml(test-args,test-expected-failure,test-combined-informational-with-output,test-output-file-with-violations) fail on unmodifiedmaintoo, not just on this branch__tests__/fixtures/malformed/bad.mdinto valid markdown as a side effect of an unrelated commit (the message says "bad.md fixture corrected to valid markdown", but that file exists specifically to be malformed for these tests)test-action.ymlonly runs on changes toaction.yml/scripts/rumdl-action.sh, this PR is just the first one since then to touch that pathLeft that fixture alone here since it's unrelated to the pip issue, but put together a fix for it and confirmed it gets all 15 jobs green. If it's useful, the diff and a green run are here: main...soy-chrislo:rumdl:fix/restore-malformed-fixture. Happy to open it as its own PR, or keep sorting it out on #709.
What's tested:
test-default(the 3-OS matrix: ubuntu/macos/windows): greentest-with-version(pinned0.0.189, exercises the version-pinning path): greenmainand on this branch (compared run to run), so this change isn't what's causing themOpen to splitting anything out differently if that's preferred.