chore(shellcheck): add yarn shellcheck script and CI workflow#401
chore(shellcheck): add yarn shellcheck script and CI workflow#401kriscendobot wants to merge 5 commits into
Conversation
| # can branch on emptiness. xargs is not used directly because BSD xargs | ||
| # (macOS) lacks `-r` and runs the command on an empty list, which would | ||
| # invoke `shellcheck` with no positional arguments and read from stdin. | ||
| files=$(git ls-files -z '*.sh' | tr '\0' '\n') |
There was a problem hiding this comment.
This is an oversize variable for bash. Consider using HASH=$(git hash-object -w --stdin) and git cat-file blob $HASH to ensure we do not run through the maximum command line argument length.
There was a problem hiding this comment.
Addressed in 46ba165 (fix(scripts): pass shellcheck targets via git blob to avoid argv limit). The file list now streams through git hash-object -w --stdin then git cat-file blob $HASH per your suggestion; the intermediate $files variable is gone. Local ./scripts/shellcheck.sh runs clean.
There was a problem hiding this comment.
Please verify that this is not duplicative with shellcheck work in the lint job. If so, please consolidate. This appears to be more effective than whatever’s latent.
There was a problem hiding this comment.
Verified before pushing: grep -rni shellcheck .github/ package.json shows the new files added by this PR are the only shellcheck references in the repo. Root package.json defines "lint": "yarn lint:prettier && yarn lint:eslint" and the lint job in .github/workflows/ci.yml runs yarn lint, so neither invokes shellcheck. The new workflow is the only shellcheck gate in CI; no consolidation needed.
Per kriskowal review on PR #401, stream the .sh file list through a git blob (`git hash-object -w --stdin` then `git cat-file blob $HASH`) instead of stuffing the list into a shell variable. This keeps us clear of ARG_MAX limits on hosts with many tracked .sh files.
|
Thanks for the review @kriskowal. Addressed both items:
Local |
|
Please rebase on actual master and shepherd through CI. |
|
Mirror of endojs/endo#3300 (head 32d76f46b). |
| #!/bin/bash | ||
| git clone https://github.com/AgoricBot/nat.git | ||
| cd nat | ||
| cd nat || exit |
There was a problem hiding this comment.
Early termination should exit non-zero and emit an error message to stderr, as with the die pattern from Perl. Please research the appropriate idiom and apply throughout.
There was a problem hiding this comment.
Addressed in a7b9b9a ("chore(shellcheck): adopt die idiom for early termination in touched .sh files").
Adopted the canonical POSIX shell die shape, defined per-script near the top:
die() { printf '%s\n' "$*" >&2; exit 1; }(printf rather than echo -e to keep behavior portable across /bin/sh and /bin/bash.) I checked the tree for an existing project helper (die/fail/error_exit); none exists, so each touched script defines its own.
Applied throughout the .sh files this PR touched at every early-termination site:
packages/nat/scripts/npm-audit-fix.sh:5—cd nat || die "cd: nat failed"packages/nat/scripts/npm-audit-fix.sh:30—cd integration-test || die "cd: integration-test failed"scripts/npm-audit-fix.sh:5—cd ??? || die "cd: ??? failed"scripts/posttypedoc.sh:5—cd docs || die "cd: docs failed"scripts/posttypedoc.sh:9—cd api-docs || die "cd: api-docs failed"
scripts/check-packages.sh's terminal exit "$EXIT" and scripts/shellcheck.sh's exit 0 skip-when-empty are orderly success/aggregated-status exits rather than the silent-non-zero early termination the comment objects to, so they stay as-is. Let me know if you want either upgraded too.
yarn shellcheck still passes on the new tip.
…sh files Address kriskowal's CHANGES_REQUESTED review on #401: replace bare `|| exit` after `cd` with `|| die "<context>"` so early-termination sites emit a context line to stderr and exit non-zero, as with Perl's `die`. Each touched script defines its own helper near the top: die() { printf '%s\n' "$*" >&2; exit 1; } `printf` (not `echo -e`) avoids portability traps under /bin/sh. Sites upgraded across the .sh files this PR touched: - packages/nat/scripts/npm-audit-fix.sh: `cd nat`, `cd integration-test` - scripts/npm-audit-fix.sh: `cd ???` - scripts/posttypedoc.sh: `cd docs`, `cd api-docs` scripts/check-packages.sh's terminal `exit "$EXIT"` and scripts/ shellcheck.sh's `exit 0` skip-when-empty are intentional orderly exits, not the early-termination pattern the review objects to. Left as-is. The "throughout" scope is the .sh files this PR touched (the seven plus the new scripts/shellcheck.sh); broader cross-tree adoption is left to a separate dispatch if the maintainer wants it.
|
@kriskowal addressed in a7b9b9a ("chore(shellcheck): adopt die idiom for early termination in touched .sh files"). Adopted the canonical POSIX shell die() { printf '%s\n' "$*" >&2; exit 1; }
Replaced bare
|
Address every finding shellcheck emits at -S warning over the repository's tracked shell scripts. The changes are mechanical and do not alter runtime behavior: - Add missing `#!/bin/sh` / `#!/bin/bash` shebangs (SC2148). - Append `|| exit` to bare `cd` invocations whose failure should abort (SC2164). - Convert `for X in $(find ...)` to `while read; do ...; done < <(find ...)` so filenames with whitespace survive (SC2044). - Convert `find ... | xargs` to `find ... -print0 | xargs -0` for the same reason (SC2038). - Replace `CDPATH=` with `CDPATH=''` so the assignment is unambiguous (SC1007). - Drop an unused `DIR=$(dirname ...)` assignment (SC2034). - Add `-r` to `read` calls so backslashes round-trip through the pipe. These prepare the tree for the `yarn shellcheck` gate that lands in the same PR.
Adds a deterministic lint gate over every tracked .sh file in the repository. - `scripts/shellcheck.sh` enumerates `git ls-files '*.sh'` and runs `shellcheck -S warning` over the list, forwarding any extra args to the underlying checker. Exits 0 when the list is empty so forks or branches that have not yet adopted a shell script stay green. - `yarn shellcheck` wires the wrapper into the root `package.json` scripts table. - `.github/workflows/shellcheck.yml` runs the gate on push to master and on any pull_request that touches a `.sh` file (the workflow's `paths:` filter). A PR that touches no shell scripts does not trigger the workflow at all, matching the maintainer's "skip when no .sh in the diff from the base" requirement. shellcheck ships preinstalled on `ubuntu-latest` runners so no additional dependency or apt install is needed. Action pins follow the project's existing zizmor-pedantic posture (`actions/checkout@de0fac2e...`); the workflow scope grants `contents: read` and the job runs with `persist-credentials: false`.
Per kriskowal review on PR #401, stream the .sh file list through a git blob (`git hash-object -w --stdin` then `git cat-file blob $HASH`) instead of stuffing the list into a shell variable. This keeps us clear of ARG_MAX limits on hosts with many tracked .sh files.
…sh files Address kriskowal's CHANGES_REQUESTED review on #401: replace bare `|| exit` after `cd` with `|| die "<context>"` so early-termination sites emit a context line to stderr and exit non-zero, as with Perl's `die`. Each touched script defines its own helper near the top: die() { printf '%s\n' "$*" >&2; exit 1; } `printf` (not `echo -e`) avoids portability traps under /bin/sh. Sites upgraded across the .sh files this PR touched: - packages/nat/scripts/npm-audit-fix.sh: `cd nat`, `cd integration-test` - scripts/npm-audit-fix.sh: `cd ???` - scripts/posttypedoc.sh: `cd docs`, `cd api-docs` scripts/check-packages.sh's terminal `exit "$EXIT"` and scripts/ shellcheck.sh's `exit 0` skip-when-empty are intentional orderly exits, not the early-termination pattern the review objects to. Left as-is. The "throughout" scope is the .sh files this PR touched (the seven plus the new scripts/shellcheck.sh); broader cross-tree adoption is left to a separate dispatch if the maintainer wants it.
kriscendobot
force-pushed
the
chore/shellcheck-ci
branch
from
8fcb241 to
85ebc88
Compare
|
Rebased onto fresh frozen base |
|
@kriscendobot Please integrate the shellcheck step into the lint job instead of a fresh job. |
Per kriskowal's directive on PR #401 (issuecomment-4676193609), integrate the shellcheck step into the existing CI lint job instead of running it from a separate workflow. The standalone `Shell script lint` workflow used a `paths:` filter on `**/*.sh` so PRs touching no shell scripts skipped it entirely. The lint job in `ci.yml` has no such filter, so shellcheck now runs on every PR. The wrapper `scripts/shellcheck.sh` already short-circuits to exit 0 when no `.sh` files are tracked, so the cost on a `.sh`-free diff is a fast no-op enumeration. Gate semantics are preserved: every .sh-touching diff still hits shellcheck, and master pushes still hit it unconditionally via the lint job's own master trigger. - `.github/workflows/ci.yml`: add `Show shellcheck version` and `Run yarn shellcheck` steps to the lint job, between `yarn lint` and `yarn build:types:check`. - `.github/workflows/shellcheck.yml`: deleted.
|
Addressed in 2ee513b. Folded the shellcheck step into the Gate-semantics note: the standalone workflow had a |
2 participants
Summary
Adds
yarn shellcheckplus a CI workflow running shellcheck against every.shfile tracked in the repo. A PR that touches no.shfiles does not trigger the workflow at all (the workflow'spaths:filter on**/*.shprovides the skip-when-no-.sh-touched behavior).What lands
package.json:"shellcheck": "scripts/shellcheck.sh"script.scripts/shellcheck.sh: wrapper that enumeratesgit ls-files '*.sh', exits 0 when empty, otherwise runsshellcheck -S warningover the list (forwarding any extra args to the underlying checker)..github/workflows/shellcheck.yml: workflow withpaths: ['**/*.sh', ...]onpull_requestand an unfiltered run onpushtomaster.packages/compartment-mapper/test/neutralize.shpackages/nat/scripts/npm-audit-fix.shscripts/check-packages.shscripts/maintenance/check-unused-deps.shscripts/npm-audit-fix.shscripts/posttypedoc.shscripts/set-versions.shPre-existing shellcheck findings and how they were handled
Running
shellcheck -S warningover the tree before this PR produced sixteen findings across seven files, in six distinct codes:#!/...shebang#!/bin/shor#!/bin/bashper the file's stylecdwithout|| exit|| exitfor ... in $(find ...)while IFS= read -r ... ; do ... done < <(find ...)find ... | xargsfind ... -print0 | xargs -0CDPATH=ambiguousCDPATH=''DIR=$(...)readtoread -rAll fixes are mechanical. None alter runtime behavior. The two stale
npm-audit-fix.shfiles (rootscripts/andpackages/nat/scripts/) date back to 2019 and reference an AgoricBot-era flow with literal???placeholders; they remain in the tree because they are tracked, and the shebangs added here at least make them shellcheck-clean.Skip behavior
A PR that touches no
.shfiles does not trigger the workflow at all (the workflow'spaths:filter excludes everything else). Pushes tomasterrun unconditionally so the gate is authoritative for the branch.Local invocation
Regression evidence
The gate was sanity-tested by removing the shebang from
packages/compartment-mapper/test/neutralize.shand re-runningscripts/shellcheck.sh; it failed with SC2148 on that file. The sabotage was reverted before commit.Notes for reviewers
ubuntu-latestrunners; no apt install or dev-dep is needed.actions/checkout@de0fac2e...) matches the project's existing zizmor-pedantic posture; workflow scope grants onlycontents: read; checkout runs withpersist-credentials: false.concurrencyblock.