Add shellcheck to pre-commit and fix warnings #1427
Conversation
gforsyth
added
improvement
| RAPIDS_DOCS_DIR="$(mktemp -d)" | ||
| export RAPIDS_DOCS_DIR |
There was a problem hiding this comment.
This looks very weird to me and unnecessarily verbose, why is that change recommended by shellcheck?
There was a problem hiding this comment.
It's a bash footgun.
If it's a single-line, then the return code of the line is the result of export and it swallows the return code of the captured command.
There was a problem hiding this comment.
And when running this locally, the errors will include a link explaining the issue: https://www.shellcheck.net/wiki/SC2155
There was a problem hiding this comment.
Ah makes sense, but it's also only problematic if you're running a command, if you're assigning a static value then exporting in the same line should have no problems. Thanks both!
| @@ -9,10 +9,10 @@ rapids-logger "validate packages with 'pydistcheck'" | |||
|
|
|||
| pydistcheck \ | |||
| --inspect \ | |||
| "$(echo ${wheel_dir_relative_path}/*.whl)" | |||
| "$(echo "${wheel_dir_relative_path}"/*.whl)" | |||
There was a problem hiding this comment.
Maybe I'm mistaken, but wouldn't this mean ${wheel_dir_relative_path} is not within double-quotes since the open double-quote starts before $(echo ...?
There was a problem hiding this comment.
Another bash footgun, quoting resets inside of captures
|
|
||
| rapids-logger "validate packages with 'twine'" | ||
|
|
||
| twine check \ | ||
| --strict \ | ||
| "$(echo ${wheel_dir_relative_path}/*.whl)" | ||
| "$(echo "${wheel_dir_relative_path}"/*.whl)" |
There was a problem hiding this comment.
Similar to the above, doesn't this mean it's actually outside double-quotes?
|
/merge |
Description
shellcheck is a fast, static analysis tool for shell scripts. It's good at
flagging up unused variables, unintentional glob expansions, and other potential
execution and security headaches that arise from the wonders of bash (and other shlangs).
This PR adds a pre-commit hook to run shellcheck on all of the sh-lang files in the ci/ directory, and
the changes requested by shellcheck to make the existing files pass the check.
xref: rapidsai/build-planning#135