Add shellcheck support and robustify present shell scripts #5294
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
=== Do not change lines below ===
{
"chain": [
"f724281d8f3cd1dc4f3538186d91fdf0bc8cf997"
],
"cmd": "shellcheck -f diff run | patch -p1",
"exit": 0,
"extra_inputs": [],
"inputs": [],
"outputs": [],
"pwd": "."
}
^^^ Do not change lines above ^^^
It is not clear from the code what was envisioned "exactly". But as given it is
analogous to "${*:2}" if what was really wanted is to get a single string with
all args from 2nd concatenated.
Here are the helpers used here:
❯ cat /tmp/show
#!/bin/bash
args=${@:2}
echo "$args"
/home/yoh/bin/printargs "$args"
qargs="${@:2}" # just quoted to pacify shellcheck
/home/yoh/bin/printargs "$qargs"
qargs="${*:2}" # quoted and * instead of @ to pacify shellcheck
/home/yoh/bin/printargs "$qargs"
corrected_args=( "${@:2}" )
echo "$corrected_args"
/home/yoh/bin/printargs "$corrected_args"
# as array
/home/yoh/bin/printargs "${corrected_args[@]}"
changes on filesystem:
run | 6 ++++++
❯ cat ~/bin/printargs
#!/bin/bash
for a in "$@"; do
echo ">$a<"
done
which would produce following output
❯ bash -x /tmp/show 1 "2 2" 3 4 5
+ args='2 2 3 4 5'
+ echo '2 2 3 4 5'
2 2 3 4 5
+ /home/yoh/bin/printargs '2 2 3 4 5'
>2 2 3 4 5<
+ qargs='2 2 3 4 5'
+ /home/yoh/bin/printargs '2 2 3 4 5'
>2 2 3 4 5<
+ qargs='2 2 3 4 5'
+ /home/yoh/bin/printargs '2 2 3 4 5'
>2 2 3 4 5<
+ corrected_args=("${@:2}")
+ echo '2 2'
2 2
+ /home/yoh/bin/printargs '2 2'
>2 2<
+ /home/yoh/bin/printargs '2 2' 3 4 5
>2 2<
>3<
>4<
>5<
and shellcheck would report following issues on the helper:
❯ shellcheck /tmp/show
In /tmp/show line 3:
args=${@:2}
^----^ SC2124 (warning): Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
In /tmp/show line 7:
qargs="${@:2}" # just quoted to pacify shellcheck
^------^ SC2124 (warning): Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
In /tmp/show line 14:
echo "$corrected_args"
^-------------^ SC2128 (warning): Expanding an array without an index only gives the first element.
In /tmp/show line 15:
/home/yoh/bin/printargs "$corrected_args"
^-------------^ SC2128 (warning): Expanding an array without an index only gives the first element.
For more information:
https://www.shellcheck.net/wiki/SC2124 -- Assigning an array to a string! A...
https://www.shellcheck.net/wiki/SC2128 -- Expanding an array without an ind...
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
yarikoptic
commented
Feb 16, 2024
| $(bold ./$name pnpm build) # Prepare it for production | ||
| $(bold ./"$name" pnpm vitest) # Run watcher for unit tests | ||
| $(bold ./"$name" cypress) # Run integration tests (after starting dev server) | ||
| $(bold ./"$name" pnpm build) # Prepare it for production |
There was a problem hiding this comment.
although looks odd, it is the only way to make it robust if script would ever be renamed to include space in the name. Alternatively that check could be skipped indeed.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5294 +/- ##
============================================
+ Coverage 43.20% 79.15% +35.94%
============================================
Files 23 175 +152
Lines 5115 14514 +9399
Branches 23 866 +843
============================================
+ Hits 2210 11488 +9278
+ Misses 2904 2823 -81
- Partials 1 203 +202
Flags with carried forward coverage won't be shown. Click here to find out more. |
aloisklink
requested changes
Feb 28, 2024
Comment on lines
+13
to
19
| # TODO: fix this one to become spaces tollerant etc | ||
| # If to be used as an array, make it | ||
| # args=( "${@:2}" ) | ||
| # If to concatenated into a single string and thus loosing ability to pass args with spaces | ||
| # args="${*:2}" | ||
| # shellcheck disable=SC2124 | ||
| args=${@:2} |
There was a problem hiding this comment.
todo(blocking): I'm fairly certain quoting "$args" will break this script.
Do you think it makes sense to change it to something like:
diff --git a/run b/run
index f01302c43..ecb392e58 100755
--- a/run
+++ b/run
@@ -10,18 +10,12 @@ red() { ansi 31 "$@"; }
name=$(basename "$0")
command=$1
-# TODO: fix this one to become spaces tollerant etc
-# If to be used as an array, make it
-# args=( "${@:2}" )
-# If to concatenated into a single string and thus loosing ability to pass args with spaces
-# args="${*:2}"
-# shellcheck disable=SC2124
-args=${@:2}
+args=( "${@:2}" )
case $command in
build)
-docker compose build "$args"
+docker compose build "${args[@]}"
;;
sh)
@@ -29,7 +23,7 @@ $RUN mermaid sh
;;
pnpm)
-$RUN mermaid sh -c "pnpm $args"
+$RUN mermaid pnpm "${args[@]}"
;;
dev)
@@ -41,7 +35,7 @@ $RUN --service-ports mermaid sh -c "pnpm run --filter mermaid docs:dev:docker"
;;
cypress)
-$RUN cypress "$args"
+$RUN cypress "${args[@]}"
;;
help|"")I'll be honest, I don't really use this run script, but a quick test of the above changes seemed to work.
Comment on lines
+14
to
+26
| shellcheck: | ||
| name: Check shell scripts | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| - name: Install dependencies | ||
| run: | | ||
| 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.
Love this!
Just to tidy up, do you think it makes sense to move this into the .github/workflows/lint.yml with the other lint job?
You'll have to move the permissions: contents: read into the jobs.shellcheck too.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
## 📑 Summary
https://www.shellcheck.net/ is great to robustify shell scripts, in particular make them work correctly when paths with spaces in them are involved somehow.
TODOs
args=${@:2}since that semantic is unclear. ATM I just ignored that line while fixing the rest. See beed5fd for the description of the gotcha there. One solution is to not do anything and keep that TODO. Another -- do fix it: I think it was intended to be used as an array, hence should becomeargs=( "${@:2}" )but then uses should be adjusted accordingly. Please confirm that it was intended use.Make sure you
MERMAID_RELEASE_VERSIONis used for all new features.developbranch