style(shell): Address various shellcheck issues and formatting

[echoix]

This is the original work that #3547 spun off from.

My end goal with this series is to be able to enable shell script linting with shellcheck. It is also the shell-backing linter in actionlint, which can lint GitHub Action .yml files (using shellcheck for the run step), and Hadolint, that can lint Dockerfiles (using shellcheck for the RUN instructions). It can catch a lot of cases that we aren’t always aware of the problems, adapts the rules according to the interpreter used (sh/bash/dash/ksh), has many great long-form error descriptions (that make it easy to understand the issue), and some of the detections have suggested fixes when they are appropriate.

So here are a couple of changes, starting with the .github folder, binder, docker, and utils. In utils, the dep_tree2sql.sh is in another PR. The fix_typos.sh isn’t yet completed, as it will take more than just trivial fixes, it doesn’t run at all and both files that need to be download don’t exist at these locations anymore. I found the new directory for the QGIS one, but didn’t check for the rest. It will be for another time, as even if the file count is small, it’s not obvious for everyone to review if you don’t remember the subtleties between syntaxes (or have some reference nearby).

The majority of the changes are in regards to double quoting to prevent globing and word splitting. There is also the use of arrays and referring to all of them instead of an unquoted string.

I didn’t go all in for the macOS script changes, as I wasn’t so confident enough on what macOS would support, even as a posix-/bin/sh shell. So instead I simply removed the double quotes inside the quotes of the configure flags, and kept the unquoted use of the variable when calling configure. POSIX sh has more limited features.

Like mentioned in the other PR, I finished these files by uniformizing shell scripts formatting with shfmt -w -s -i 4 -ci -bn -sr. Between files, and sometimes in the same file, tabs vs spaces, number of tabs were different, and spaces before and after different constructs were different. These options are just to have a more complete, readable style rather than the minimal changes. The w flag writes the file in place, s is to simplify (it can also minify, but not our goal here), sets indent as 4 spaces, ci is for indenting the "case" switches, sr for space redirects, and bn for binary operations can start on a new line.
Overall it makes the final touch up of the file easier to read through properly indented (at least for me, as I already find that reading a complete shell script is mentally harder than just code and functions, because of all the different syntax packed together for a same goal).

The explanations for each change are by each commit. Each single type of change, by file, is made separately, with the justification in the commit message. Each of these separate changes aren’t worth a PR by themselves, but together they do.

There is also many more scripts in the tests to go through, and other parts of the repo, but I’m starting with this for now.

[nilason]

Overall it looks good. I left some suggestions, I may return to "module_synopsis.sh" for another overview though.

[nilason]

Here, I'd suggest to refactor, to make things more clear:

# concatenate arguments, from the second up till the last
function get_args_str() {
    local args=("$@");
    for arg in "${args[@]:1}"; do a+="${arg} "; done; echo "${a% }"
}

# Adding -Werror to make's CFLAGS is a workaround for configuring with
# an old version of configure, which issues compiler warnings and
# errors out. This may be removed with upgraded configure.in file.
makecmd="make"
if [[ "$#" -ge 2 ]]; then
    args_str=$(get_args_str "$@")
    makecmd+=" CFLAGS='${CFLAGS} ${args_str}' CXXFLAGS='${CXXFLAGS} ${args_str}'"
fi

[nilason]

See build_ubuntu-22.04.sh

[nilason]

Suggested change

	"${makecmd[@]}"
	eval "$makecmd"

[echoix]

This isn't quite the same. When having the quoted $makecmd, the arguments aren't split. Having unquoted $makecmd is problematic for globs, or all reasons why you'd want to quote variables.

Double quoted array with [@], is equivalent of double quoting each element. That would allow to properly handle a path with spaces as a single argument. (https://www.shellcheck.net/wiki/SC2068)

See https://www.shellcheck.net/wiki/SC2086 for general quoting tips (near the end), and https://www.shellcheck.net/wiki/SC2068 and https://www.shellcheck.net/wiki/SC2048 and
https://www.shellcheck.net/wiki/SC2294

[nilason]

This goes with above suggestion.

[nilason]

Suggested change

	./configure $CONFIGURE_FLAGS
	./configure "$CONFIGURE_FLAGS"

[echoix]

This I wasn't sure that it would work on posix shell on Mac, and didn't know the complete side effects

[nilason]

What shellcheck suggest is the same on Mac.

[nilason]

No need to change anything, revert to original.

[echoix]

If a string doesn't need to expand variables, it can use single quotes.
I wanted to explicitly show that we wanted the $GISBASE as an unevaluated string.

[nilason]

I wanted to explicitly show that we wanted the $GISBASE as an unevaluated string.

That's what the original code does with \.

I really don't understand why we would change a valid and clear code to something that needs to be suppressed.

[wenzeslaus]

We want code where both machine and human will understand what the author meant ideally without a comment (for human or machine). Does the original version generate a warning?

[nilason]

No need to change anything, revert to original.

[echoix]

The reviews on this PRs will be addressed later on, it is way lower on my priorities. I’d really have to carefully check everything out to make sure the suggestions handle the same edge cases and all.