-
Notifications
You must be signed in to change notification settings - Fork 148
Add shellcheck support and some fixes to make shell scripts "cleaner" #1074
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
base: release
Are you sure you want to change the base?
Conversation
=== Do not change lines below === { "chain": [], "cmd": "shellcheckit autopatch", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^
- Add shellcheck disable for SC1091 in gather-system-info.sh (sourcing /etc/os-release) - Fix SC2046 in shell.sh: Quote dbus-launch command substitution - Fix SC2178/SC2128 in shell.sh: Use array syntax consistently for args variable All shellcheck warnings resolved.
Xephyr $NEW_DISPLAY & | ||
DISPLAY=$NEW_DISPLAY | ||
args=--x11 | ||
args=(--x11) |
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.
We're not really using the array, so I think we might as well make this x11_arg="--x11"
here and gnome-shell "$x11_arg"
later
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.
well, then better be named also x11_arg
to be factually correct? since if args
then would assume multiple and thus treated accordingly ;-)
sudo apt update && sudo apt install -y shellcheck | ||
- name: shellcheck | ||
run: | | ||
git grep -l '^#\( *shellcheck \|!\(/bin/\|/usr/bin/env \)\(sh\|bash\|dash\|ksh\)\)' | xargs shellcheck |
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.
What does *shellcheck
do?
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.
https://www.shellcheck.net/ says in the title "finds bugs in your shell scripts." ;)
Quite often it is so, and most often could be observed if you run any script under path with a space in some folder name . (that is why in our datalad tool testing we even come up with some most obscure path filesystem can tollerate to ensure we always correctly quote etc)
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.
Ah, I should have clarified. I know shellcheck (it's great, I use it for all my shell scripts). I was asking about the string "*shellcheck
" in the git grep
regex.
More info is at https://www.shellcheck.net about shellcheck
Note that the last commit was done mostly by
claude code
but upon my review should be kosher.In view of potential desire for moving some logic from Makefile (see #1073 ) into external scripts, might especially be nice to ensure that shell scripts code is more robust