fix some shellcheck warnings

[vnherdeiro]

Contributes to #6498

Following this attempt #6548 at contributing to LightGBM and @jameslamb guidance I am now trying to help with fixing the shellcheck warnings for all the bash scripts inside the package (at least I think so).

Screenshot to show that the warnings are gone in my local environment:

Fixing https://www.shellcheck.net/wiki/SC1091 errors (source activate $SOME_CONDA_ENV) by pointing at at /dev/null is quite unsatisfactory but I did not manage to come up with something better.

Welcoming any feedback!

[vnherdeiro]

@microsoft-github-policy-service agree

[vnherdeiro]

This raises shellcheck info level SC2086 (info): Double quote to prevent globbing and word splitting. but adding the double quotes breaks the command (BUILD_ARGS is empty) I don't understand the root cause so I leave it as it. This is repeated twice in the file.

[vnherdeiro]

I had to revert some of the changes due to breaking commands. Now we have 3 SC2086 (info): Double quote to prevent globbing and word splitting. in build-python.sh. Pipelines seem to be passing (except the mac one, let me know if it's a regression please).

[jameslamb]

Thanks for your help, and for taking the time to contribute to LightGBM!

A few comments:

1. The tool is called shellcheck, not spellcheck

I've changed that in the description and title.

2. Please limit this to just a few files

I do really appreciate your help here, but think this PR is too large to review. As it says in #6498

It is not necessary to fix all of the problems in a single pull request.

There are some changes in this PR that are more controversial and require a bit more discussion, and I don't want all of this work to be blocked on those discussions... let's please reduce this to a smaller set of files and work together on fixing all the shellcheck errors in them. Once this PR is merged, hopefully you'll be able to help with the other shellcheck errors in a follow-up PR.

Pease remove the changes to the following files:

• .ci/test.sh
• .ci/test-r-package.sh
• .ci/setup.sh
• build-python.sh
• build-cran-package.sh

Since you made this PR from a branch also called master on your fork (please do not do that in the future, as I asked in #6548 (comment)), the easiest way to do that is to just go to https://github.com/microsoft/LightGBM, copy the file content, paste it in one your local clone, commit the changes, and push.

I've left suggestions on the changes to the other files.

3. Please use linking here on GitHub to connect the discussions

I've added a reference to #6498 in the description. In the future, when you contribute here and the work is related to other discussions, please do link them.

[jameslamb]

Was this intended to address a specific shellcheck error, or is it just something you're proposing for stylistic similarity? I don't mind this change (I generally support wrapping interpolation in ${} like this), just trying to understand the change.

[vnherdeiro]

Good question. This one was not raised by shellcheck. I was uniformizing the "binding" (interpolation in your words) with the rest of the file. Shall we agree on a way of binding/interpolating?

[jameslamb]

ok just checking. This is fine, let's leave it (I do prefer the style with ${}) but please don't touch other lines that aren't directly related to the purpose of the PR.

[vnherdeiro]

@jameslamb Thanks for the extensive review. I have noted all of your points. Apologies for pushing from master, I did not notice I had forgotten to branch out beforehand. I can close this PR and open it from a feature branch on my clone.

I have reversed the files you have asked me too, let's keep the changes for another PR. It think I have adressed all of your points so far.

[jameslamb]

Thanks very much, these changes look good to me.

It won't matter because we squash-merge PRs here, but just so you know... most of the commits you pushed here were not correctly tied to your GitHub account. You can see #3607 (comment) and the things it links to to learn more about that. If you plan to continue working on projects here on GitHub, I recommend adjusting your .gitconfig to ensure your commit are tied to your GitHub account.

[jameslamb]

ok just checking. This is fine, let's leave it (I do prefer the style with ${}) but please don't touch other lines that aren't directly related to the purpose of the PR.

[vnherdeiro]

Thanks for the approval @jameslamb

[jameslamb]

I re-reviewed the changes you pushed after my last approval and found an issue with them. Please see the suggestion I left here.

[jameslamb]

What was the purpose of these changes that you pushed after I approved? It looks to me like they result in the files we care about all being ignored.

Ignoring file: ./cmake/IntegratedOpenCL.cmake ./cmake/Sanitizer.cmake ./cmake/modules/FindLibR.cmake ./CMakeLists.txt 

(build link)

If this was from that shellcheck warning about assigning the result of find to a variable... let's just use find -exec instead. Please try this:

find \
    . \
    -type f \
    \( -name CMakeLists.txt -o -name '*.cmake' \) \
    -not -path './external_libs/*' \
    -exec cmakelint \
        --linelength=120 \
        --filter=-convention/filename,-package/stdargs,-readability/wonkycase \
        {} \+

I tested this locally (on my mac) and found it worked well. This listed the expected files:

find \
    . \
    -type f \
    \( -name CMakeLists.txt -o -name '*.cmake' \) \
    -not -path './external_libs/*'

./CMakeLists.txt
./cmake/Sanitizer.cmake
./cmake/IntegratedOpenCL.cmake
./cmake/modules/FindLibR.cmake

And the full statement did raise warnings about CMake formatting when I changed some code in those scripts.

[vnherdeiro]

@jameslamb First I did not mean to creep in a change after your approval, I am used to MR/PR settings in which a code change requires a re-approval. I changed the line because the cmakelint execution was failing the CI. I traced it to the file path arguments being passed with '\n' joins instead of single-whitespace. I changed the find + (negative) grep command for a more efficient find with a ignore path logic.

I missed the log line saying that the files were ignored, and I don't understand why it happens. I did think of uxing exec but I guessed the command would get quite long, hence less readable, and some one may want to print the value of $cmake_files for debugging purposes.

Now coming to the snipper you propose. I see you are changing the original -o -path "./cmake/*.cmake" for -o -name '*.cmake' and the file list passed to the cpp linting changes from

./CMakeLists.txt ./cmake/Sanitizer.cmake ./cmake/IntegratedOpenCL.cmake ./cmake/modules/FindLibR.cmake

to

./CMakeLists.txt ./cmake/Sanitizer.cmake ./cmake/IntegratedOpenCL.cmake ./cmake/modules/FindLibR.cmake ./build/CMakeFiles/_lightgbm.dir/DependInfo.cmake ./build/CMakeFiles/_lightgbm.dir/cmake_clean.cmake ./build/CMakeFiles/3.30.2/CMakeCXXCompiler.cmake ./build/CMakeFiles/3.30.2/CMakeCCompiler.cmake ./build/CMakeFiles/3.30.2/CMakeSystem.cmake ./build/CMakeFiles/lightgbm.dir/DependInfo.cmake ./build/CMakeFiles/lightgbm.dir/cmake_clean.cmake ./build/CMakeFiles/lightgbm_capi_objs.dir/DependInfo.cmake ./build/CMakeFiles/lightgbm_capi_objs.dir/cmake_clean.cmake ./build/CMakeFiles/Makefile.cmake ./build/CMakeFiles/lightgbm_objs.dir/DependInfo.cmake ./build/CMakeFiles/lightgbm_objs.dir/cmake_clean.cmake ./build/CMakeFiles/CMakeDirectoryInformation.cmake ./build/cmake_install.cmake

which causes the number of linting errors to jump from 0 to 65 locally. I am pushing your change with exec but using the old -o -path "./cmake/*.cmake" match pattern.

[jameslamb]

ohhhhhhh thank you so much for the thorough explanation!

There shouldn't be a ./build/ directory in CI (because we don't actually run a CMake build in the lint CI task), but it is likely that there could be one lying around when you run this locally if you've also been doing other development in the project.

Keeping ./cmake/*.cmake for now is fine... this is how the linting here worked before (e.g. is not a change made by your PR) and all of the *.cmake files we currently want to lint do happen to be in that directory).

[jameslamb]

thanks!