chore: adopt checkmake + extract procedural Makefile targets to scripts/#193
Conversation
Makefile hygiene, triggered by a checkmake finding on benchmark-compare — but the finding was the tip of a config mismatch (checkmake's default maxbodylength=5 flags 33 of 74 targets), so we adopt the linter tuned to the repo rather than contort idiomatic targets. Adopt checkmake (checkmake.ini): disable maxbodylength (the repo's `cmd 2>&1 | tee log; if-fail; exit 1` idiom, the `help` echo-menu, and $(MAKE)-orchestration targets are legitimately >5 lines) and minphony (repo uses `.DEFAULT_GOAL := help`, not `all`); keep phonydeclared. Wired as `make lint-makefile` + a pre-commit hook (requires `go install github.com/checkmake/checkmake/cmd/checkmake@latest`). Fix the 2 genuine phonydeclared findings: add test-critical and security-deep to .PHONY. Extract the 5 targets that were genuinely procedural shell scripts (loops, multi-branch logic) into scripts/ with behavior parity — NOT the ~20 command-wrapper / make-orchestration targets, which stay in the Makefile (extracting those would be the anti-pattern). Each becomes a thin target calling its script: - security-install -> scripts/install-security-tools.sh - security-report -> scripts/security-report.sh - version-check -> scripts/version-check.sh - fuzz-quick (Python arm)-> scripts/fuzz-python.sh (rust arm stays in target) - build-multiarch-linux -> scripts/build-multiarch-linux.sh scripts/_common.sh provides shared ANSI colors. Makefile -134 lines. checkmake passes clean (0 violations) under checkmake.ini; make version-check and make security-install verified behavior-equivalent.
|
Warning Review limit reached
More reviews will be available in 1 hour, 41 minutes, and 47 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
1 participant
Why
A CodeRabbit/checkmake nit on
benchmark-compare(PR #188) exceedingmaxbodylength=5turned out to be the tip of a config mismatch: checkmake's default flags 33 of 74 targets, because this Makefile's idioms (cmd 2>&1 | tee log; if-fail; exit 1, thehelpecho-menu,$(MAKE)-orchestration) are legitimately longer than 5 lines. The right response to a linter whose defaults don't fit is to configure it and fix the findings that are real — not contort 33 targets.What's here
Adopt checkmake, tuned to the repo (
checkmake.ini)maxbodylength(doesn't fit the idiom — see above) andminphony(repo uses.DEFAULT_GOAL := help, notall). Keepphonydeclared.make lint-makefile+ a pre-commit hook (language: system, consistent with the existing cargo/basedpyright hooks). Requiresgo install github.com/checkmake/checkmake/cmd/checkmake@latest.Fix the 2 genuine
phonydeclaredfindings — addtest-criticalandsecurity-deepto.PHONY(alias/dependency-only targets that were silently missing).Extract the 5 genuinely-procedural targets to
scripts/(loops, multi-branch logic — script-shaped), each now a thin target calling a behavior-equivalent script:security-install(58 lines)scripts/install-security-tools.shsecurity-report(31)scripts/security-report.shversion-check(11)scripts/version-check.shfuzz-quick(Python arm)scripts/fuzz-python.sh(rust arm stays in the target)build-multiarch-linux(22)scripts/build-multiarch-linux.shscripts/_common.shprovides shared ANSI colors. The Makefile shrinks by 134 lines.Deliberately NOT extracted: the ~20 command-wrapper targets (
test-*,lint-*,format-*,build,ci, …) and make-orchestration (release,release-check). They're long but flat — extracting them would create dozens of trivial one-line scripts (the actual anti-pattern). The tell for "should be a script" is control flow, not line count.Known follow-ups (out of scope here)
build-pgocallsscripts/build_with_pgo.sh, which doesn't exist — somake build-pgosilently falls back to a plainuv buildand does no PGO. Pre-existing latent bug; reviving real PGO needs careful authoring, tracked separately.make lint-makefile). A CI step is a possible follow-up.Verification
checkmake --config checkmake.ini Makefile→ 0 violations;make lint-makefile→ passes.make version-check→ behavior-identical (0.10.1/0.10.1 ✓).make security-install→ all branches exercised, exit 0 (idempotent).bash -nclean on all 6 scripts; executables are0755.## helptext andsetup-logsprereqs.main(f35d0cd, 0.10.1).