Add shellcheck.py and apply first round of fixes
#7698
Conversation
Eric-Arellano
requested review from
stuhood,
jsirois,
benjyw,
illicitonion,
cosmicexplorer and
blorente
|
|
||
|
|
||
| def run_shellcheck() -> None: | ||
| targets = glob("./build-support/bin/**/*.sh", recursive=True) + [ |
There was a problem hiding this comment.
Could we do something like rg -l "#!/bin/bash" build-support to get an automatic list of these?
There was a problem hiding this comment.
I think this is a great suggestion so looked into it and run into a couple issues:
- Not every environment has ripgrep installed, including CI.
- Using something portable like
find+awkis much harder to read. findalso doesn't skip our.gitignoreby default, which is not good.- We can certainly require they install it—that's fine by me. But it would mean to run
githooks/pre-commitpeople now need bothripgrepandshellcheckinstalled.
- Using something portable like
- Ripgrep doesn't have a way to search only the first line, at least that I know of.
- There was a Python file with this shebang in it that was a false positive.
- Not everything uses the correct shebang
#!/usr/bin/env bash, some use#!/bin/bash.- I'll actually fix this in a dedicated PR!
I think all of these are solvable. Are we okay with #1 to require ripgrep to be installed? I'd love if we could make that assumption, then we could start using ripgrep in other scripts!
There was a problem hiding this comment.
Actually #1 is the one I'm least comfortable with, unless we somehow get a ripgrep binary at the bootstrap step (same way we fetch the rust toolchain, although it's definitely not my area of expertise). Although at this point this definitely becomes a separate PR, along with #2 and maybe #3 if noone has gotten around to it yet.
There was a problem hiding this comment.
So fine with hardcoded paths by now.
| @@ -49,11 +49,13 @@ fi | |||
|
|
|||
There was a problem hiding this comment.
Might be worth adding a comment here about the shellcheck annotations, I was confused a bit when I saw them in this file.
There was a problem hiding this comment.
I don't think I agree, because we use these directives in multiple other files and I wouldn't want to add the comment anywhere we use a directive.
This is very similar to comments we use elsewhere like # isort:skip. They're a bit confusing at first, but also call the tool's name right away and are very search engine friendly.
There was a problem hiding this comment.
Sure, I was only confused for like 15 seconds :)
… issues and for less duplication (#7702) ### Problem The current bash script violates Shellcheck for not properly quoting a variable. However, fixing this breaks the behavior and there is no simple way to fix it by converting the result of `find` to an array. See #7698 (comment). This is not the first time the bash script has been difficult to work with. For example, #6747 took 26 commits to land and several back-and-forth code snippets over Slack. Finally, the bash script has much duplication in it. ### Solution Rewrite to modern Python 3. ### Result Bad imports will be caught the same as before. To de-duplicate logic, the printed user message is a bit different, however. The failure message is also now outputted in red.
Eric-Arellano
dismissed
illicitonion’s stale review
Talked over Slack that concerns were addressed, but didn't have time yet for a re-review.
[Shellcheck](https://www.shellcheck.net) is a phenomenal linter for bash scripts. We have a non-trivial amount of bash code, so any additional help we can get to ensure quality is useful. This PR only takes shellcheck's suggestions when obviously correct. More complex changes are left for followup PRs, after which we can start to require shellcheck in CI.
| if ! out="$("${cargo}" ensure-prefix \ | ||
| --manifest-path="${REPO_ROOT}/src/rust/engine/Cargo.toml" \ | ||
| --prefix-path="${REPO_ROOT}/build-support/rust-target-prefix.txt" \ | ||
| --all --exclude=bazel_protos)"; then |
There was a problem hiding this comment.
I would absolutely make this into a function!
| @@ -11,12 +11,12 @@ mkdir -p logs/jobs | |||
| curl=(curl -s -S -H 'Travis-API-Version: 3') | |||
|
|
|||
| "${curl[@]}" 'https://api.travis-ci.org/repo/pantsbuild%2Fpants/builds?event_type=pull_request&limit=100&sort_by=finished_at:desc' > logs/search | |||
| jobs="$(cat logs/search | jq "[ .builds[] | select(.pull_request_number == ${pull}) ][0] | .jobs[].id")" | |||
| jobs="$(jq "[ .builds[] | select(.pull_request_number == ${pull}) ][0] | .jobs[].id" logs/search)" | |||
| # shellcheck disable=SC2016 | ||
| bad_output="$(find ./* -name '*.sh' -print0 \ | ||
| | xargs -0 grep '^ *\(readonly\|declare\) .*\(\$(\|`\)' \ | ||
| || :)" |
Applies more fixes from the `shellcheck.py` script introduced in #7698. There are still a few more failures, which are left for a final PR that will fix those issues and enforce shellcheck in `githooks/pre-commit`.
Shellcheck is a phenomenal linter for bash scripts. We have a non-trivial amount of bash code, so any additional help we can get to ensure quality is useful.
This PR only takes shellcheck's suggestions when obviously correct. More complex changes are left for followup PRs, after which we can start to require shellcheck in CI.