-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce shellcheck to lint shell scripts #15169
Introduce shellcheck to lint shell scripts #15169
Conversation
.github/workflows/format.yml
Outdated
@@ -0,0 +1,29 @@ | |||
on: | |||
push: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth defining paths
here. Supposedly this check could then run very rarely, only when needed
https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#onpushpull_requestpull_request_targetpathspaths-ignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was wondering about that as well. A bit of an obstacle is that there's no simple glob to catch all shell script. Some like bin/crystal
have no extension. The GitHub action actually checks all files for a matching shebang.
Limiting paths would need to hard code all paths that don't identify as shell script by their extension. That means the check could miss out on new ones being added.
I suppose this is an acceptable limitation, though. I wouldn't expect too much movement in that regard.
On the other hand, this job finishes reallly quickly - especially compared to our other workflows - in just 7s so it's not too much overhead to run it on every commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's a fair point. Not a good tradeoff then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not so sure. Either way seems quite fine. So maybe we can hear some other's comments on this.
It's also relevant to note that the discovery feature depends on the GitHub action and another comment is suggesting we might do without the action. Droping automatic path discovery would make that easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I inspected the repository and found the following scripts which cannot be identified by typical shell extensions in the file name:
$ find . -type f ! -path './.git/*' -exec file {} + | awk -F: '/shell script/{print $1}' | grep -v \.sh
./src/llvm/ext/find-llvm-config
./scripts/git/pre-commit
./bin/crystal
./bin/ci
./bin/check-compiler-flag
That shouldn't be difficult to hardcode. And maybe we could even rename some to include an extension (find-llvm-config.sh
for example; it's only an internal tool anyway).
In case we add a new script without an extension, the risk of missing to add it here it's acceptable.
It will be nice to have this 🙂 |
Co-authored-by: Oleh Prypin <[email protected]>
Issues fixed: * https://www.shellcheck.net/wiki/SC2145 * https://www.shellcheck.net/wiki/SC2027 * https://www.shellcheck.net/wiki/SC2034 * https://www.shellcheck.net/wiki/SC2046 * https://www.shellcheck.net/wiki/SC2124 * https://www.shellcheck.net/wiki/SC2164 * https://www.shellcheck.net/wiki/SC2294 * https://www.shellcheck.net/wiki/SC2059 * https://www.shellcheck.net/wiki/SC2086 * https://www.shellcheck.net/wiki/SC2295 * https://www.shellcheck.net/wiki/SC2002 * https://www.shellcheck.net/wiki/SC2005 * https://www.shellcheck.net/wiki/SC2006 Issues ignored: * https://www.shellcheck.net/wiki/SC2013 Co-authored-by: Oleh Prypin <[email protected]>
This PR fixes a number of issues in several shell scripts that shellcheck complains about. It also introduces a new CI job to ensure shellcheck compliance in the future.
Issues fixed:
Issues ignored:
Follow-up to #15139