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

Apply more shellcheck fixes to build-support scripts #7719

Merged
merged 6 commits into from May 15, 2019

Conversation

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

commented May 14, 2019

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.

@illicitonion
Copy link
Contributor

left a comment

Thanks!

@@ -1,7 +1,7 @@
#!/bin/bash -eu

if [[ "$(uname)" == "Darwin" ]]; then
if (( $(ls -1 /cores | wc -l) > 0 )); then
if (( $(find /cores -maxdepth 1 ! -name "$(basename /cores)" | wc -l) > 0 )); then

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 14, 2019

Contributor

Teach me! This seems worse than what was there before... What's wrong with the old way?

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 14, 2019

Author Contributor

Agreed that it's more verbose. This change is because you should never use ls in scripts. ls is only designed for humans and not machines, e.g. its output is not stable. See https://mywiki.wooledge.org/ParsingLs.

The specific issue this fixes is https://github.com/koalaman/shellcheck/wiki/SC2012.

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 14, 2019

Contributor

According to https://mywiki.wooledge.org/ParsingLs the thing you're guarding against is that filenames may contain newlines, and these two approaches have the same problem there... I won't block the PR, but I don't like this particular change...

Show resolved Hide resolved build-support/bin/publish_docs.sh Outdated
Show resolved Hide resolved build-support/bin/shellcheck.py Outdated
Show resolved Hide resolved build-support/githooks/prepare-commit-msg Outdated

@Eric-Arellano Eric-Arellano merged commit 58972ae into pantsbuild:master May 15, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:shellcheck-round-2 branch May 15, 2019

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.