Uh oh!
There was an error while loading. Please reload this page.
fix(lint): let make lint bootstrap golangci-lint on a fresh machine - #1041
Conversation
make lint could not run at all on a machine that had never installed golangci-lint: brew upgrade requires the formula to be present already, so the target died with "Error: golangci-lint not installed" before the linter ran. Checking for the formula and installing it when absent makes the target idempotent. The env var was also never reaching brew. Make expands $H (undefined) and leaves the literal OMEBREW_NO_AUTO_UPDATE, so every make lint triggered a full brew update, mutating the developer's brew installation as a side effect of linting. Dropping the $ passes the assignment to the shell. Presence-only, with no upgrade, also keeps local behaviour aligned with CI, which uses golangci-lint-action rather than brew. Closes#1038
Claude finished @JonJagger's task in 1m 16s —— View job Review: PR #1041 — |
Uh oh!
There was an error while loading. Please reload this page.
The defect is the leading $: make reads $H (undefined) and leaves the literal OMEBREW_NO_AUTO_UPDATE, so brew never sees the env var and every make lint triggers a full brew update, mutating the developer's brew installation as a side effect of linting. Dropping the $ passes the assignment to the shell. Replacing upgrade with a presence-only install went beyond that defect and removed any way of keeping golangci-lint current, which is what review flagged. brew upgrade keeps the local linter moving with CI, which runs golangci-lint-action at version: latest. Bootstrapping on a machine that has never installed golangci-lint is a separate concern and stays out of scope here.
| ensure_golangci-lint: | ||
| @$HOMEBREW_NO_AUTO_UPDATE=1 brew upgrade golangci-lint | ||
| @HOMEBREW_NO_AUTO_UPDATE=1 brew upgrade golangci-lint |
There was a problem hiding this comment.
The $ expansion fix here is correct and worth keeping. In a Make recipe $H expands the (undefined) Make variable H to empty, leaving the literal OMEBREW_NO_AUTO_UPDATE=1 brew upgrade …. That stray token never sets the env var, so every make lint was actually triggering a full Homebrew auto-update. Dropping the $ passes the assignment through to the shell. Good catch. 👍
One caveat now that commit c2f232b3 reverted back to brew upgrade: on a machine that has never installed golangci-lint, brew upgrade golangci-lint still fails with Error: golangci-lint not installed — which is exactly the fresh-machine scenario in #1038. So with the current diff this target is not idempotent on a clean machine.
If keeping brew upgrade is the intended scope, consider making it self-bootstrapping without losing the upgrade behaviour:
| @HOMEBREW_NO_AUTO_UPDATE=1 brew upgrade golangci-lint | |
| @HOMEBREW_NO_AUTO_UPDATE=1 brew list golangci-lint >/dev/null 2>&1 \ | |
| && HOMEBREW_NO_AUTO_UPDATE=1 brew upgrade golangci-lint \ | |
| || HOMEBREW_NO_AUTO_UPDATE=1 brew install golangci-lint |
Re: @gsavage's questions — with brew upgrade kept, the linter does stay current (mechanism for keeping it up-to-date), and it's unpinned, which matches CI's golangci-lint-action at version: latest. Pinning to a specific version would be a deliberate separate change if you want local == a fixed CI version.
make lint could not run at all on a machine that had never installed
golangci-lint: brew upgrade requires the formula to be present already, so
the target died with "Error: golangci-lint not installed" before the linter
ran. Checking for the formula and installing it when absent makes the target
idempotent.
The env var was also never reaching brew. Make expands $H (undefined) and
leaves the literal OMEBREW_NO_AUTO_UPDATE, so every make lint triggered a
full brew update, mutating the developer's brew installation as a side
effect of linting. Dropping the $ passes the assignment to the shell.
Presence-only, with no upgrade, also keeps local behaviour aligned with CI,
which uses golangci-lint-action rather than brew.
Closes#1038