[ci] fix some shellcheck warnings in package-building scripts

[vnherdeiro]

Contributes to #6498 continues from #6621

Now I am tackling ./build-python.sh and ./build-cran-package.sh

All feedback is welcome!

Please forgive the polluted commit history.

[jameslamb]

Thanks for working on this!

Left a first round of comments for your consideration.

[jameslamb]

Please forgive the polluted commit history.

No worries! We'll squash all of the history when this is merged anyway.

[vnherdeiro]

That is a prompt review @jameslamb Thanks, will amend soonish!

[vnherdeiro]

@jameslamb thanks for the review and code suggestions about the more extent quotemarks! Do you mind if I start a new PR soon with changes for the files you had asked my to postpone from #6621 :

• .ci/test.sh
• .ci/test-r-package.sh
• .ci/setup.sh
  ?

[jameslamb]

Thanks for working on this.

I'm confused by several of these changes. As I've asked in previous reviews... please explain the reasoning for your proposes changes here. Several of these are not obvious at all, and it's difficult to review without knowing the underlying shellcheck warnings you're trying to address.

[jameslamb]

Please, explain this. What specifically is shellcheck complaining about that led to this change?

[vnherdeiro]

https://www.shellcheck.net/wiki/SC2086 The problem comes from BUILD_ARGS being empty and interpolated, it passes an argument (which is the empty string, and the command fail.s We need a ternary check for this case.

[jameslamb]

Thanks, but I just pushed commits reverting this for the reasons mentioned in #6641 (comment).

The claim you've made here that this being empty would result in the command failing is simply not true. You can try this yourself:

sh build-python.sh bdist_wheel

That will reach this line with BUILD_ARGS empty, and nothing will fail. We even intentionally set BUILD_ARGS to the empty string when --precompile is passed.

LightGBM/build-python.sh

Lines 295 to 299 in 668bf5d

	if test "${INSTALL}" = true; then
	if test "${PRECOMPILE}" = true; then
	BUILD_SDIST=true
	BUILD_WHEEL=false
	BUILD_ARGS=""

And I use sh build-python.sh install --precompile every time I develop on LightGBM's Python package.

[jameslamb]

What is the purpose of this change?

[vnherdeiro]

I had explained it here-above. I cannot get a link, so pasting:

This one used to raise https://www.shellcheck.net/wiki/SC2086

Here using "$BUILD_ARGS" breaks down if it's empty. It seems to suggest using an array to group all arguments possibly appended to BUILD_ARGS. So we have this ternary expression checking for string emptiness if yes, the return is null. That's the way I understand it. Per the shellcheck docs:

This is better than an unquoted value because the alternative value can be properly quoted, e.g. wget ${output:+ -o "$output"}.

[jameslamb]

I had explained it here-above

I see. I had not seen #6641 (comment) when I wrote this, maybe it's another one that wasn't published originally because it was part of an unpublished review (as you mentioned in #6641 (comment)).

I've pushed commits reverting this, for the reasons explained in #6641 (comment)

[jameslamb]

Do you mind if I start a new PR soon with changes for the files you had asked my to postpone from #6621

Looks like you decided to do that in #6646. That's fine, will review there.

[jameslamb]

@vnherdeiro will you have time in the next week to come back and work with us on this? If not, I'll close this and fix the remaining shellcheck issues myself.

[vnherdeiro]

@jameslamb I am back and willing to continue working on this.

[jameslamb]

Alright thanks. You can start here: #6641 (comment)

[vnherdeiro]

@jameslamb I am still figuring out how github review works, I had input comments and responses to your own but missed the submit review action, hence they were not visitble to you (or anyone but me). Completed that now.

I have commits on this PR now.

[jameslamb]

I've pushed commits to finish this up.

Now that I have commits on here, my approval shouldn't count toward a merge. Hopefully another reviewer will be able to look soon and we can merge this.

After that, I'll put up a PR to start running shellcheck in LightGBM's CI. Thanks for your help with this project, @vnherdeiro !

[StrikerRUS]

LGTM!