Replace Makefiles with just - #255
Conversation
Convert the three Makefiles (ext, web_ext/sseq_gui, web_ext/steenrod_calculator) to justfiles and update every caller. - ext/justfile: benchmark pattern rules become parametrized recipes (bench/bench-concurrent/fix-bench NAME) plus loop recipes over examples/benchmarks/*. The benchmark first line is expanded unquoted into an inner `bash -c`, reproducing the Makefile's quote-removal of the literal "" tokens. Also fixes the long-standing fix-benchmarks bug (bare BENCHMARKS instead of $(BENCHMARKS)). - web_ext/sseq_gui/justfile: WASM_UNWIND becomes a justfile variable (override via `just WASM_UNWIND=1 wasm`) driving if/else flag vars; test-wasm-unwind re-invokes the lib build with the override. - CI (.github/workflows/ext.yaml): swap `make -C <dir> <tgt>` for `just --working-directory <dir> --justfile <dir>/justfile <tgt>` and add an extractions/setup-just step to every job that runs recipes. - Nix: add pkgs.just to the root flake's devTools (subflakes inherit it) and swap make->just in the ext/sseq_gui app run scripts. - READMEs: update documented commands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DSAx1G1eMeCYPwNH7WNMVo
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR replaces make-based build tooling with just-based commands across ext, web_ext/sseq_gui, and web_ext/steenrod_calculator. It adds new justfiles, removes the old Makefiles, updates CI and flake scripts, and revises README instructions to match. ChangesMake to Just Migration
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ext.yaml:
- Around line 23-25: Update the GitHub Actions workflow to use the newer
setup-just action and Just release. In this workflow, replace every use of
extractions/setup-just@v2 with v4, and change the just-version value in the
setup-just step from 1.34 to 1.54.0 so the action and installed tool stay
current.
In `@web_ext/sseq_gui/justfile`:
- Around line 52-78: The test-wasm-unwind recipe is validating the wrong
artifact by inspecting wasm-lib instead of the shipped wasm output. Update
test-wasm-unwind to check the bindgen/optimized wasm artifact referenced by
WASM_FILE, or invoke the wasm target so the full pipeline is exercised; use the
existing symbols test-wasm-unwind, WASM_LIB, WASM_FILE, and wasm to locate the
change.
In `@web_ext/steenrod_calculator/justfile`:
- Around line 6-10: The WASM output path construction in the justfile has a
redundant slash because WASM_OUT already includes a trailing separator. Update
the WASM_FILE definition so it uses WASM_OUT directly without adding another
"/", and adjust the related command usages that reference WASM_OUT (such as the
wasm-bindgen out-dir and copy destination) to build paths consistently with the
existing trailing slash.
- Around line 20-22: The setup-wasm recipe installs an unpinned
wasm-bindgen-cli, which can drift out of sync with the wasm-bindgen dependency
version used by the project. Update the setup-wasm target in the justfile to
install a specific 0.2.x wasm-bindgen-cli release that matches the crate
constraint, so the build remains reproducible even without a lockfile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d891dee-a51b-4379-bad8-1d2c8357a030
📒 Files selected for processing (12)
.github/workflows/ext.yamlext/Makefileext/README.mdext/flake.nixext/justfileflake.nixweb_ext/sseq_gui/Makefileweb_ext/sseq_gui/README.mdweb_ext/sseq_gui/flake.nixweb_ext/sseq_gui/justfileweb_ext/steenrod_calculator/Makefileweb_ext/steenrod_calculator/justfile
💤 Files with no reviewable changes (3)
- web_ext/steenrod_calculator/Makefile
- ext/Makefile
- web_ext/sseq_gui/Makefile
The migration added `set -euo pipefail` to the sseq_gui `test-wasm-unwind`
recipe, which changed the behavior of
wasm-objdump -h $WASM_LIB | grep -qi '^ *Tag '
wasm-objdump prints the section headers (including `Tag`) to stdout but can
exit non-zero on a later section it cannot parse. Under the old Makefile's
plain `sh` (no pipefail) the pipe's exit came from `grep`, so a present `Tag`
section passed. With `pipefail`, wasm-objdump's non-zero exit propagated and
failed the check even though `Tag` was found. Drop `pipefail` for this recipe
(use `set -eu`) to restore the Makefile behavior.
Also bump the CI tooling as suggested in review: extractions/setup-just@v2 ->
@v4 and just-version 1.34 -> 1.54.0.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DSAx1G1eMeCYPwNH7WNMVo
- test-wasm-unwind (sseq_gui): build the full WASM_UNWIND=1 `wasm` pipeline and inspect the shipped WASM_FILE (wasm-bindgen/wasm-opt output) instead of the raw WASM_LIB, so breakage in the bindgen/opt stage is caught too. - WASM_FILE path: drop the doubled separator (WASM_OUT already names the dir), so it is `dist/<name>_wasm_bg.wasm` rather than `dist//<name>_wasm_bg.wasm`, in both the sseq_gui and steenrod_calculator justfiles. - setup-wasm: pin wasm-bindgen-cli to the version the crate resolves to (derived via `cargo tree`) rather than installing an unpinned CLI that can drift out of sync with the schema-locked wasm-bindgen dependency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DSAx1G1eMeCYPwNH7WNMVo
|
|
||
| - name: Run ext tests | ||
| run: make -C ext test | ||
| run: just --working-directory ext --justfile ext/justfile test |
There was a problem hiding this comment.
Is it necessary to pass both --working-directory and --justfile? Doing cd dir && just test would be shorter...
| lint: | ||
| cargo fmt --all -- --check | ||
| cargo clippy --workspace --no-default-features --profile test | ||
| cargo clippy --workspace --all-targets --profile test | ||
| cargo check --workspace --no-default-features --profile test | ||
| cargo check --workspace --all-targets --all-features --profile test |
There was a problem hiding this comment.
As a followup would be nice to add a variant of this that fixes the problems instead of doing --check.
| fi | ||
|
|
||
| # Rewrite a single benchmark's expected output to the current program output. | ||
| fix-bench NAME: |
There was a problem hiding this comment.
I've seen this called bless-bench in many codebases.
There was a problem hiding this comment.
for "bless the current output as correct"
| just bench resolve-S_2 | ||
| just bench-concurrent resolve-S_2 | ||
| just fix-bench resolve-S_2 |
hoodmane
left a comment
hoodmane
left a comment
There was a problem hiding this comment.
Generally looks good to me. Thanks!
Per maintainer review on the upstream PR:
- CI: replace `just --working-directory <dir> --justfile <dir>/justfile <rec>`
with the shorter `cd <dir> && just <rec>` across ext.yaml.
- ext/justfile: add a `fix` recipe that applies what `lint` only checks
(`cargo fmt --all` + `cargo clippy --fix`).
- ext/justfile: rename the benchmark-updating recipes fix-bench /
fix-benchmarks / fix-benchmarks-nassau to bless-bench / bless-benchmarks /
bless-benchmarks-nassau ("bless the current output as correct"); update
ext/README.md accordingly.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DSAx1G1eMeCYPwNH7WNMVo
|
Thanks for the review, @hoodmane! Addressed the suggestions in b3fcf24:
Separately, the earlier Generated by Claude Code |
In sseq_gui, `wasm-bindgen` is a `wasm32`-only dependency (`[target.'cfg(target_arch = "wasm32")'.dependencies]`), so a host-target `cargo tree` prints nothing and the derived version came out empty — making `cargo install wasm-bindgen-cli --version ''` fail and breaking the webserver / webserver-deploy jobs. Use `--target all` so the wasm-only dependency is seen, and guard against an empty result so any future breakage fails with a clear message instead of a cryptic cargo error. Applied to both wasm justfiles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DSAx1G1eMeCYPwNH7WNMVo
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/README.md (1)
92-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting
just fixalongsidejust lint.The justfile now has a
fixrecipe (apply fmt + clippy autofixes) added per earlier review feedback, but the README's "Linting" section only mentionsjust lint. Worth a one-line addition for discoverability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ext/README.md` around lines 92 - 101, The Linting section only documents the just lint recipe, so add a brief mention of the new just fix recipe for discoverability. Update the README near the linting instructions to reference just fix alongside just lint, and note that it applies formatting plus clippy autofixes so readers can choose the autofix workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ext.yaml:
- Around line 23-25: The new extactions/setup-just@v4 workflow steps are still
referenced by moving tag rather than a fixed SHA, so tighten them for
supply-chain hardening. Update each setup-just usage in the workflow to pin the
action to a commit hash instead of the v4 tag, keeping the existing just-version
inputs unchanged. Make the change consistently across all repeated setup-just
steps in the workflow so the pinning is uniform.
- Around line 380-386: The `calculator` job has a mislabeled workflow step: the
`Lint sseq_gui wasm` name does not match the `just lint` command being run in
`web_ext/steenrod_calculator`. Update that step’s name in the workflow so it
clearly refers to the calculator wasm lint task, matching the surrounding step
naming in the `calculator` job and avoiding confusion with the `webserver` job.
In `@ext/justfile`:
- Around line 25-29: The justfile’s fix recipe only mirrors part of lint, so
update the fix target to cover the same clippy configurations as lint. In fix,
keep the rustfmt step and add clippy autofix runs that match the lint variants
in the same workspace/test profile, including the --no-default-features pass and
the --all-targets pass, so clippy issues caught by lint are actually auto-fixed
by just fix. Use the existing fix and lint recipe names in ext/justfile to align
the commands.
---
Outside diff comments:
In `@ext/README.md`:
- Around line 92-101: The Linting section only documents the just lint recipe,
so add a brief mention of the new just fix recipe for discoverability. Update
the README near the linting instructions to reference just fix alongside just
lint, and note that it applies formatting plus clippy autofixes so readers can
choose the autofix workflow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74b33247-c67f-4e8a-912a-5de3ccf79753
📒 Files selected for processing (5)
.github/workflows/ext.yamlext/README.mdext/justfileweb_ext/sseq_gui/justfileweb_ext/steenrod_calculator/justfile
| - uses: extractions/setup-just@v4 | ||
| with: | ||
| just-version: "1.54.0" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Consider pinning extractions/setup-just to a commit SHA.
zizmor flags each of these extractions/setup-just@v4 uses as unpinned. This mirrors the repo's existing convention of pinning third-party actions by tag rather than SHA (e.g. actions/checkout@v4, dtolnay/rust-toolchain@v1), so it's not a regression introduced by this PR, but since these are new lines, tightening them to a hash would incrementally improve supply-chain hardening.
Also applies to: 71-73, 104-106, 155-158, 214-217, 279-281, 355-357, 413-415
🧰 Tools
🪛 zizmor (1.26.1)
[error] 23-23: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ext.yaml around lines 23 - 25, The new
extactions/setup-just@v4 workflow steps are still referenced by moving tag
rather than a fixed SHA, so tighten them for supply-chain hardening. Update each
setup-just usage in the workflow to pin the action to a commit hash instead of
the v4 tag, keeping the existing just-version inputs unchanged. Make the change
consistently across all repeated setup-just steps in the workflow so the pinning
is uniform.
Source: Linters/SAST tools
- ext.yaml: rename the calculator job's mislabeled "Lint sseq_gui wasm" step to "Lint calculator wasm" (it runs the calculator lint, not sseq_gui). - ext/justfile: make `fix` mirror all of `lint`'s clippy configurations (`--no-default-features --profile test` and `--all-targets --profile test`), so `just fix` autofixes everything `just lint` would flag. - ext/README.md: mention `just fix` alongside `just lint` in the Linting section. Skipped CodeRabbit's suggestion to SHA-pin `extractions/setup-just@v4`: the repo consistently pins actions by tag (actions/checkout@v4, dtolnay/rust-toolchain@v1, actions/cache@v4), so a lone SHA-pinned action would break that convention. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DSAx1G1eMeCYPwNH7WNMVo
3 participants
Converts the three
Makefiles (ext,web_ext/sseq_gui,web_ext/steenrod_calculator) tojustjustfiles and updates every caller. Behavior is preserved — the recipes run the same underlyingcargo/wasm-bindgen/wasm-opt/pytestcommands.Changes
ext/,web_ext/sseq_gui/, andweb_ext/steenrod_calculator/.ext/justfile: the Make pattern-rules overexamples/benchmarks/*becomejust bench NAME,just bench-concurrent NAME,just fix-bench NAME(replacingmake examples/benchmarks/X), with the-nassaufeature auto-selected by name suffix;benchmarks,benchmarks-nassau,benchmarks-concurrent,fix-benchmarks,fix-benchmarks-nassau.The benchmark's first line is expanded unquoted into an inner
bash -c, reproducing the Makefile's quote-removal of the literal""tokens.web_ext/sseq_gui/justfile:WASM_UNWINDis now a justfile variable overridable viajust WASM_UNWIND=1 wasm, drivingif/elseflag vars;test-wasm-unwindre-invokes the lib build with the override..github/workflows/ext.yaml): eachmake -C <dir> <tgt>becomesjust --working-directory <dir> --justfile <dir>/justfile <tgt>, and anextractions/setup-just@v2step is added to every job that runs recipes.pkgs.justadded to the rootflake.nixdevTools(both subflakes inherit it viasuper.defaultPackages.devTools);make→justin theext/sseq_guiapp run scripts.justcommands.Notes
fix-benchmarksMake target (it referenced a bareBENCHMARKSinstead of$(BENCHMARKS), so it did nothing); thejustversion works.just bench resolve-S_2(andjust bench-concurrent/just fix-bench) instead ofmake examples/benchmarks/resolve-S_2. READMEs reflect this.Verification
Validated locally with
justinstalled: all three justfiles parse;just bench resolve-S_2passes (confirming the empty-string arg handling);just fix-benchround-trips without modifying the file; theWASM_UNWINDconditional evaluates to empty flags by default and the unwind flags when set;ext.yamlis valid YAML.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
justcommands.justautomation for WebAssembly builds (including optional unwind verification) and benchmark running/blessing.makewithjust.justrecipes.justto available dev tools in the development shell.