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 #2575

Merged
merged 2 commits into from
Feb 10, 2025

Conversation

gforsyth
Copy link
Contributor

@gforsyth gforsyth commented Feb 6, 2025

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 February 6, 2025 21:14
@gforsyth gforsyth added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 6, 2025
@gforsyth gforsyth requested a review from AyodeAwe February 6, 2025 21:14
@github-actions github-actions bot added the ci label Feb 6, 2025

cd "${package_dir}"

rapids-logger "validate packages with 'pydistcheck'"

pydistcheck \
--inspect \
"$(echo ${wheel_dir_relative_path}/*.whl)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously package_name was unused in this script

Copy link
Member

Choose a reason for hiding this comment

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

This change is why https://github.com/rapidsai/raft/actions/runs/13188022880/job/36815130064?pr=2575 is failing like this:

Usage: pydistcheck [OPTIONS] [FILEPATHS]...
Try 'pydistcheck --help' for help.

Error: Invalid value for '[FILEPATHS]...': Path 'final_dist/raft-dask*.whl' does not exist.

package_name here is raft-dask (with a -), but the wheel filename begins with raft_dask (with a _). That's intentional behavior.

I think a better fix here would be to just remove package_name from the script (and the corresponding calls to it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, stripped out package_name. thanks for tracking that down, @jameslamb !

Copy link
Member

Choose a reason for hiding this comment

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

No prob! It's my fault that package_name was left behind there unused in anyway 😅

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.

Left on requested change on validate_wheel.sh.


cd "${package_dir}"

rapids-logger "validate packages with 'pydistcheck'"

pydistcheck \
--inspect \
"$(echo ${wheel_dir_relative_path}/*.whl)"
Copy link
Member

Choose a reason for hiding this comment

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

This change is why https://github.com/rapidsai/raft/actions/runs/13188022880/job/36815130064?pr=2575 is failing like this:

Usage: pydistcheck [OPTIONS] [FILEPATHS]...
Try 'pydistcheck --help' for help.

Error: Invalid value for '[FILEPATHS]...': Path 'final_dist/raft-dask*.whl' does not exist.

package_name here is raft-dask (with a -), but the wheel filename begins with raft_dask (with a _). That's intentional behavior.

I think a better fix here would be to just remove package_name from the script (and the corresponding calls to it).

@jameslamb jameslamb removed the request for review from AyodeAwe February 7, 2025 17:50
@gforsyth
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 1a8e38b into rapidsai:branch-25.04 Feb 10, 2025
76 checks passed
@gforsyth gforsyth deleted the shellcheck branch February 10, 2025 14:38
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
Development

Successfully merging this pull request may close these issues.

2 participants