Add shellcheck to pre-commit and fix warnings #4954

gforsyth · 2025-03-03T19:03:28Z

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

jameslamb

Thanks for continuing this. Left some comments for your consideration.

jameslamb · 2025-03-03T22:27:21Z

ci/build_docs.sh

  DGL_CHANNEL="dglteam/label/th23_cu118"
 else
-  CONDA_CUDA_VERSION="12.1"


I love when these shellcheck PRs find unused variables 😁

jameslamb · 2025-03-03T22:29:37Z

ci/test_wheel.sh

@@ -28,5 +28,5 @@ else
       --import-mode=append \
       --benchmark-disable \
       -k "not test_property_graph_mg and not test_bulk_sampler_io" \
-       ./python/${package_name}/${python_package_name}/tests
+       ./python/"${package_name}"/"${python_package_name}"/tests


Suggested change

./python/"${package_name}"/"${python_package_name}"/tests

"./python/${package_name}/${python_package_name}/tests"

Since there isn't anything else here like wildcards that need to be evaluated, I have a mild preference for just wrapping the entire thing in double quotes, instead of having 2 inner sets of double quotes. Think that's a little easier to read and to modify.

jameslamb · 2025-03-03T22:30:44Z

ci/utils/nbtest.sh

+    jupyter nbconvert --to script "${NBFILENAME}" --output "${NBTMPDIR}"/"${NBNAME}"-test
+    echo "${MAGIC_OVERRIDE_CODE}" > "${NBTMPDIR}"/tmpfile
+    cat "${NBTESTSCRIPT}" >> "${NBTMPDIR}"/tmpfile
+    mv "${NBTMPDIR}"/tmpfile "${NBTESTSCRIPT}"


Suggested change

jupyter nbconvert --to script "${NBFILENAME}" --output "${NBTMPDIR}"/"${NBNAME}"-test

echo "${MAGIC_OVERRIDE_CODE}" > "${NBTMPDIR}"/tmpfile

cat "${NBTESTSCRIPT}" >> "${NBTMPDIR}"/tmpfile

mv "${NBTMPDIR}"/tmpfile "${NBTESTSCRIPT}"

jupyter nbconvert --to script "${NBFILENAME}" --output "${NBTMPDIR}/${NBNAME}-test"

echo "${MAGIC_OVERRIDE_CODE}" > "${NBTMPDIR}/tmpfile"

cat "${NBTESTSCRIPT}" >> "${NBTMPDIR}/tmpfile"

mv "${NBTMPDIR}/tmpfile" "${NBTESTSCRIPT}"

Similar to my comments above.

jameslamb · 2025-03-03T22:50:03Z

.pre-commit-config.yaml

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


Could you expand this a bit? I'd really like to see it cover stuff like https://github.com/rapidsai/cugraph/blob/branch-25.04/datasets/get_test_data.sh and https://github.com/rapidsai/cugraph/blob/branch-25.04/scripts/dask/run-dask-process.sh too

If you want to just remove this exclusion and have it cover all shell scripts in this round, I don't mind that making the PR larger.

That'd also be one less repo to have to modify in the second round of this that covers build.sh scripts.

bdice · 2025-04-11T15:28:35Z

@gforsyth Want to revive this? I retargeted to 25.06 but there are merge conflicts.

If we quote a string variable like `INSTALL_TARGET=""` then that expands with `${INSTALL_TARGET}` into `''` inline which can break argument parsing for `cmake` and other programs. If we make them arrays, empty arrays splat out to nothing, but arguments are correctly spaced.

ci/release/update-version.sh

acostadon

It looks good from Docs standpoint.
I have a question but nothing to slow down approval.

rlratzel · 2025-04-15T20:19:16Z

build.sh

+CMAKE_VERBOSE_OPTION=()
 BUILD_TYPE=Release
-INSTALL_TARGET="--target install"
+INSTALL_TARGET=("--target" "install")
 BUILD_CPP_TESTS=ON
 BUILD_CPP_MG_TESTS=OFF
 BUILD_CPP_MTMG_TESTS=OFF
 BUILD_ALL_GPU_ARCH=0
-CMAKE_GENERATOR_OPTION="-G Ninja"
-PYTHON_ARGS_FOR_INSTALL="-m pip install --no-build-isolation --no-deps --config-settings rapidsai.disable-cuda=true"
+CMAKE_GENERATOR_OPTION=("-G" "Ninja")
+PYTHON_ARGS_FOR_INSTALL=("-m" "pip" "install" "--no-build-isolation" "--no-deps" "--config-settings" "rapidsai.disable-cuda=true")


TIL that shellcheck turns strings with spaces into arrays. The potential issues it's trying to avoid don't seem to outweigh the clunkiness of using an array as a string with spaces IMO, but maybe I'll get used to it.

That was probably a macro gone overboard by me. I think I can strip out the quotes

gforsyth · 2025-04-15T20:51:27Z

/merge

`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) - Bradley Dice (https://github.com/bdice) - Don Acosta (https://github.com/acostadon) - Chuck Hastings (https://github.com/ChuckHastings) - Rick Ratzel (https://github.com/rlratzel) URL: rapidsai#4954

Add shellcheck to pre-commit and fix warnings

afb0bf3

gforsyth requested review from a team as code owners March 3, 2025 19:03

gforsyth requested a review from jameslamb March 3, 2025 19:03

github-actions bot added the ci label Mar 3, 2025

gforsyth added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 3, 2025

jameslamb approved these changes Mar 3, 2025

View reviewed changes

Fix all shell scripts

8ca6c9c

gforsyth requested review from a team as code owners March 4, 2025 16:41

github-actions bot added cuGraph python benchmarks conda datasets labels Mar 4, 2025

Merge branch 'branch-25.04' into shellcheck

b28a75e

gforsyth mentioned this pull request Mar 11, 2025

Add shellcheck across RAPIDS rapidsai/build-planning#135

Open

bdice changed the base branch from branch-25.04 to branch-25.06 April 11, 2025 15:28

Merge branch 'branch-25.06' into shellcheck

c5faa3b

github-actions bot removed the conda label Apr 11, 2025

gforsyth added 3 commits April 11, 2025 12:00

chore: missing a $

8becb2a

refactor: make python args an array

c4f3f8f

bdice approved these changes Apr 11, 2025

View reviewed changes

acostadon reviewed Apr 14, 2025

View reviewed changes

ci/release/update-version.sh Show resolved Hide resolved

acostadon approved these changes Apr 14, 2025

View reviewed changes

ChuckHastings approved these changes Apr 14, 2025

View reviewed changes

Merge branch 'branch-25.06' into shellcheck

2ce9487

rlratzel approved these changes Apr 15, 2025

View reviewed changes

gforsyth and others added 2 commits April 15, 2025 16:33

chore: remove quotes from argument arrays

21c6e28

Merge branch 'branch-25.06' into shellcheck

614eb0f

rapids-bot bot merged commit 9e1211a into rapidsai:branch-25.06 Apr 15, 2025
81 checks passed

gforsyth deleted the shellcheck branch April 16, 2025 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shellcheck to pre-commit and fix warnings #4954

Add shellcheck to pre-commit and fix warnings #4954

gforsyth commented Mar 3, 2025

jameslamb left a comment

jameslamb Mar 3, 2025

jameslamb Mar 3, 2025

jameslamb Mar 3, 2025

jameslamb Mar 3, 2025

bdice commented Apr 11, 2025

acostadon left a comment

rlratzel Apr 15, 2025

gforsyth Apr 15, 2025

gforsyth commented Apr 15, 2025

	./python/"${package_name}"/"${python_package_name}"/tests
	"./python/${package_name}/${python_package_name}/tests"

Add shellcheck to pre-commit and fix warnings #4954

Add shellcheck to pre-commit and fix warnings #4954

Conversation

gforsyth commented Mar 3, 2025

jameslamb left a comment

Choose a reason for hiding this comment

jameslamb Mar 3, 2025

Choose a reason for hiding this comment

jameslamb Mar 3, 2025

Choose a reason for hiding this comment

jameslamb Mar 3, 2025

Choose a reason for hiding this comment

jameslamb Mar 3, 2025

Choose a reason for hiding this comment

bdice commented Apr 11, 2025

acostadon left a comment

Choose a reason for hiding this comment

rlratzel Apr 15, 2025

Choose a reason for hiding this comment

gforsyth Apr 15, 2025

Choose a reason for hiding this comment

gforsyth commented Apr 15, 2025