tests-hardening: addressing shellcheck errors in our tests

[smoe]

These changes shall prepare LinuxCNC's test routines to accept blanks in file names.

[BsAtHome]

Darn! I have many of the same changes in my tree.

However, there are many more and a few fixes. See master...BsAtHome:linuxcnc:fix_shellcheck for a diff. I'm still missing a few.

Notably scripts/runtests.in has added left-over shared memory check and tests/halcompile/serial-out-of-tree/checkresult fixes testing the correct target.

[BsAtHome]

And, BTW, these changes are not yet enough to run with spaces in the path.

There are a could of hard ones that, for example, expand compiler flags that can contain paths which are not escaped. Expanding it cannot use quotes because there are individual arguments and that will split an embedded path too. The right way is to use arrays, but that needs to be done consistently throughout the code. That will be very hard.

[BsAtHome]

in, for example, cppcheck.sh:

# Do this from the source directory
cd "$(dirname "$0")/../src" || (echo "E: Could not return to source directory" ; exit 1 )

You cannot use ( ) because it runs in a sub-shell. You need to use { } to exit the current shell, like in:

# Do this from the source directory
cd "$(dirname "$0")/../src" || { echo "E: Could not return to source directory"; exit 1; }

[BsAtHome]

in scripts/cppcheck.sh:

find emc/ -type d -not -name "*__pycache__" | while read -r d
...

This will fail because read does word-splitting. If there are any spaces in the path then it fails. See https://github.com/BsAtHome/linuxcnc/blob/fix_shellcheck/scripts/cppcheck.sh how it is fixed there.

[BsAtHome]

In scritps/swich.sh

SRCDIR=$(cd "$MYDIR/../src" && pwd || exit 1)

This is just plain ugly and the exit is run in the sub-shell instead of the current shell. May I suggest:

SRCDIR=$(realpath "$MYDIR/../src")
[ -d "$SRCDIR" ] || exit 1

And, $? and $# do not need double quotes.

[BsAtHome]

Disabling quote warning:
# shellcheck disable=SC2086

This is a seriously bad idea in $CPPFLAGS (f.ex. in tests/build/header-sanity/test.sh) to disable the warning. The CPPFLAGS variable may actually include paths that contain spaces. It needs to be fixed at a project-wide scale and requires arrays.

[BsAtHome]

Generally: For each added shellcheck disable I suggest you add a comment why it is done.

In tests/overrun/test.sh:

trap "rm -rf '$TMPDIR'" 0 1 2 3 15
# not trapping 9 since it cannot be trapped

Using single quotes inside will not expand TMPDIR when the trap executes. The outside must be single quoted then the inside will evaluate when executed. Pedantic note: the comment is not really useful.

In tests/tooledit/test.sh

if [ "$(wc -l tclerror.log)" -ne 0 ]; then

It should read $(wc -l < tclerror.log) because wc on a file outputs multiple fields. You only want it to return a number for which you need read from stdin, hence the redirect.

[smoe]

Well done. Thank you very much for your exceptional review. Shall I proceed by fixing my changes accordingly and then rebase everything on your branch followed by a PR against it?

[BsAtHome]

Well done. Thank you very much for your exceptional review. Shall I proceed by fixing my changes accordingly and then rebase everything on your branch followed by a PR against it?

You are welcome. I was doing the same thing, so I had most stuff in active memory.

Fixes are one thing. There are some more issues spanning our trees. It is either you merge my changes into your tree or I merge your changes into my tree. Whatever makes you happy...

[smoe]

Disabling quote warning: # shellcheck disable=SC2086

This is a seriously bad idea in $CPPFLAGS (f.ex. in tests/build/header-sanity/test.sh) to disable the warning. The CPPFLAGS variable may actually include paths that contain spaces. It needs to be fixed at a project-wide scale and requires arrays.

Do you have a good pointer to how it is done properly? I have undone the disabling of SC2086 and am with you the need for arrays. But with "make" that is a bit difficult, right?

[BsAtHome]

Disabling quote warning: # shellcheck disable=SC2086
This is a seriously bad idea in $CPPFLAGS...

Do you have a good pointer to how it is done properly? I have undone the disabling of SC2086 and am with you the need for arrays. But with "make" that is a bit difficult, right?

Yes and no. In principle it is easy where you can use mapfile or read (I used it several places in my patches).

# using IFS word-splitting:
read -r -a ARRAYVAR < <(echo "word word word")

# using full input lines (-t removes trailing newline):
mapfile -t ARRAYVAR < <(find "$SOMEPATH"  -name "*.c")

The real challenge is in the input values from third party sources. For example, path building in the configure step may already include spaces in paths. If these end up in a statement like CFLAGS=$CFLAGS "-IPath With Space", then you are in for a real pain later on when expanding CFLAGS. This goes for any constructed option variable.

I do not "see" the right thing to do, so I kept the shellcheck warnings to remind us that it needs to be addressed. We even may need to revert the "$HEADERS" expansion because it states HEADERS (plural) and that may require it to be unquoted. But I have not yet checked the definition(s) of HEADERS to see how it is constructed.

[smoe]

BsAtHome#1

[andypugh]

What's the status of this? It is showing as still WIP.

[BsAtHome]

This has been fixed in the other PRs.