10000 Add `shellcheck` to pre-commit and fix warnings by gforsyth · Pull Request #662 · rapidsai/cuvs · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add shellcheck to pre-commit and fix warnings #662

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

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

gforsyth
Copy link
Contributor
@gforsyth gforsyth commented Feb 5, 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 5, 2025 20:19
@gforsyth gforsyth added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Feb 5, 2025
@github-actions github-actions bot added the ci label Feb 5, 2025
@@ -15,9 +15,6 @@ rapids-print-env

rapids-logger "Begin py build"

package_name="cuvs"
package_dir="python"
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@@ -6,18 +6,17 @@ set -euo pipefail
package_dir=$1
wheel_dir_relative_path=$2

RAPIDS_CUDA_MAJOR="${RAPIDS_CUDA_VERSION%%.*}"
Copy link
Member

Choose a reason for hiding this comment

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

Definitely looks like something I left behind in #594 , thanks for fixing it.

@jameslamb jameslamb removed the request for review from KyleFromNVIDIA February 5, 2025 21:30
@gforsyth
Copy link
Contributor Author
gforsyth commented Feb 6, 2025

/merge

@rapids-bot rapids-bot bot merged commit 4377d48 into rapidsai:branch-25.04 Feb 6, 2025
63 checks passed
@gforsyth gforsyth deleted the shellcheck branch February 6, 2025 14:59
divyegala pushed a commit to divyegala/cuvs that referenced this pull request Feb 12, 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

Authors:
  - Gil Forsyth (https://github.com/gforsyth)

Approvers:
  - James Lamb (https://github.com/jameslamb)

URL: rapidsai#662
rapids-bot bot pushed a commit that referenced this pull request May 2, 2025
Contributes to rapidsai/build-planning#135

Follow-up to #662

While reviewing #805 and #831, I found myself suggesting things manually that I know `shellcheck` would have caught automatically. To prevent that for reviewers in the future, this proposes running `shellcheck` on **all** shell scripts in the repo, not just those in the `ci/` directory.

Other changes:

* updates `rapids-dependency-file-generator` to its latest version (1.18.1)
* consolidates duplicate entries for https://github.com/pre-commit/pre-commit-hooks in `.pre-commit-config.yaml`

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Ben Frederickson (https://github.com/benfred)

URL: #865
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0