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 to pre-commit and fix warnings #6246

Open
wants to merge 3 commits into
base: branch-25.02
Choose a base branch
from

Conversation

gforsyth
Copy link

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

@gforsyth gforsyth requested a review from a team as a code owner January 22, 2025 18:57
@gforsyth gforsyth requested a review from jameslamb January 22, 2025 18:57
@github-actions github-actions bot added the ci label Jan 22, 2025
@gforsyth gforsyth added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 22, 2025
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, nice improvement!

Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I support this! But left one question and one suggestion.

hooks:
- id: shellcheck
args: ["--severity=warning"]
files: ^ci/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not run this on all shell scripts in the repo?

I especially would love to have this coverage on build.sh (at the repo root), as that's a public interface into building the components of this project. It's be nice to have shellcheck catch issues in changes there.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, build.sh coverage is definitely on my list -- I wanted to start with ci scripts to get shellcheck in place, then we can start to expand coverage to other shell scripts in the repository.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok just wanted to be sure I understood the reasoning. As long as we'll come back again and do the rest, that's fine.


# Support invoking run_cuml_singlegpu_pytests.sh outside the script directory
cd "$(dirname "$(realpath "${BASH_SOURCE[0]}")")"/../python/cuml/cuml/tests
cd "$(dirname "$(realpath "${BASH_SOURCE[0]}")")"/../python/cuml/cuml/tests || exit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cd "$(dirname "$(realpath "${BASH_SOURCE[0]}")")"/../python/cuml/cuml/tests || exit
cd "$(dirname "$(realpath "${BASH_SOURCE[0]}")")"/../python/cuml/cuml/tests || exit 1

The || half of this will only be reached if the path doesn't exist. In that case, I think we want a non-0 exit code, so this fails loudly. Otherwise, we risk silently not running any tests in CI until someone notices.

A bare exit returns exit code 0.

cat > ./out.sh <<EOF
exit
EOF

bash ./out.sh
echo $?
# 0

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's a very good point and suggestion

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put that suggestion on just this line to show the point, but there are other || exit in the diff that I think should be || exit 1, can you update those as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants