Skip to content
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

Add `shellcheck.py` and apply first round of fixes #7698

Merged
merged 19 commits into from May 14, 2019

Conversation

Projects
None yet
4 participants
@Eric-Arellano
Copy link
Contributor

commented May 10, 2019

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.

@blorente
Copy link
Contributor

left a comment

Yay more safe bash!



def run_shellcheck() -> None:
targets = glob("./build-support/bin/**/*.sh", recursive=True) + [

This comment has been minimized.

Copy link
@blorente

blorente May 10, 2019

Contributor

Could we do something like rg -l "#!/bin/bash" build-support to get an automatic list of these?

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 10, 2019

Author Contributor

I think this is a great suggestion so looked into it and run into a couple issues:

  1. Not every environment has ripgrep installed, including CI.
    • Using something portable like find + awk is much harder to read.
    • find also doesn't skip our .gitignore by 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-commit people now need both ripgrep and shellcheck installed.
  2. 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.
  3. 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!

This comment has been minimized.

Copy link
@blorente

blorente May 10, 2019

Contributor

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.

This comment has been minimized.

Copy link
@blorente

blorente May 10, 2019

Contributor

So fine with hardcoded paths by now.

@@ -49,11 +49,13 @@ fi

This comment has been minimized.

Copy link
@blorente

blorente May 10, 2019

Contributor

Might be worth adding a comment here about the shellcheck annotations, I was confused a bit when I saw them in this file.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 10, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@blorente

blorente May 10, 2019

Contributor

Sure, I was only confused for like 15 seconds :)

@illicitonion
Copy link
Contributor

left a comment

Generally looks great!

One concerning area, but should be easy to fix :)

Show resolved Hide resolved build-support/bin/check_banned_imports.sh Outdated
Show resolved Hide resolved build-support/bin/release.sh

Eric-Arellano added a commit that referenced this pull request May 10, 2019

Convert `check_banned_imports.sh` to Python to workaround Bash array…
… 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.

Talked over Slack that concerns were addressed, but didn't have time yet for a re-review.

@Eric-Arellano Eric-Arellano merged commit 39f8080 into pantsbuild:master May 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:shellcheck branch May 14, 2019

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request May 14, 2019

Add `shellcheck.py` and apply first round of fixes (pantsbuild#7698)
[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

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 14, 2019

Contributor

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)"

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 14, 2019

Contributor

function

# shellcheck disable=SC2016
bad_output="$(find ./* -name '*.sh' -print0 \
| xargs -0 grep '^ *\(readonly\|declare\) .*\(\$(\|`\)' \
|| :)"

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 14, 2019

Contributor

function

Eric-Arellano added a commit that referenced this pull request May 15, 2019

Apply more shellcheck fixes to build-support scripts (#7719)
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`.

Eric-Arellano added a commit that referenced this pull request Jun 3, 2019

Apply final set of Shellcheck fixes and turn on in CI (#7832)
Builds off of the work from #7698 and #7719.

Several of the problems are ignored because they are very difficult to correctly fix, and the behavior is not broken when ran locally and in CI so the cost of fixing is not deemed worth it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.