Shellcheck?

[actualben]

The static analysis tool shellcheck is quite useful for finding bugs, potential future problems, and compatibility/portability issues in shell scripts.

In a codebase that deals with (for example) both *nix and darwin it can be very helpful because it finds word-splitting bugs that won't commonly show up on a linux host but will mysteriously appear on a mac because of how often mac file paths have spaces in them.

So, I was wondering:

1. Are you open to a sizeable PR implementing the minimal fixes suggested by shellcheck?

Currently there a 22 files that would be changed (and one separate but related github action update):

$ find . -iname '*.sh' -print0 | xargs -0 shellcheck -f json | jq -r '.[].file' | sort --uniq
./.ci/dockerfiles/deb-sbuild/setup-chroot.sh
./cardcomm/minidriver/build32debug.sh
./cardcomm/minidriver/build32release.sh
./cardcomm/minidriver/build64debug.sh
./cardcomm/minidriver/build64release.sh
./cardcomm/tokend/buildtokend.sh
./cardcomm/tokend/buildtokend2.sh
./installers/eid-mw/mac/create_package.sh
./installers/eid-mw/mac/create_uninst_package.sh
./plugins_tools/xpi/build.sh
./scripts/build-aux/cleanup.sh
./scripts/build-aux/fed-rel.sh
./scripts/build-aux/genver.sh
./scripts/build-aux/suse-rel.sh
./scripts/mac/create-vers.sh
./scripts/mac/export-mac.sh
./scripts/mac/export-viewer.sh
./scripts/mac/make-BEIDToken.sh
./scripts/mac/make-mac.sh
./scripts/mac/make-viewer.sh
./scripts/mac/set_eidmw_version.sh
./scripts/mac/sign-viewer.sh

Here's an example diff from that last item:

$ shellcheck -f diff ./scripts/mac/sign-viewer.sh
--- a/./scripts/mac/sign-viewer.sh
+++ b/./scripts/mac/sign-viewer.sh
@@ -13,5 +13,5 @@
 codesign -s "Developer ID Application" "/Volumes/eID Viewer/eID Viewer.app/Contents/Frameworks/BeidView.framework/Versions/A/Frameworks/libbeidpkcs11.$REL_VERSION.dylib"
 codesign -s "Developer ID Application" "/Volumes/eID Viewer/eID Viewer.app/Contents/Frameworks/BeidView.framework"
 codesign -s "Developer ID Application" "/Volumes/eID Viewer/eID Viewer.app"
-hdiutil detach $DEVNAME
+hdiutil detach "$DEVNAME"
 hdiutil convert tmp-eidviewer.dmg -format UDBZ -o "eID Viewer-$REL_VERSION-signed.dmg"

It just puts quotations around the $DEVNAME argument to prevent word-splitting. In this case the script controls the content DEVNAME (assuming a non-hostile PATH) so adding the quotes maybe just gives a bit more protection (probably a better failure) if apple changes the output formatting of the hdiutil command. I just mention this because there are many times where the changes are just belt-and-suspenders precautions, rather fixing a proper bug.

2. Are you open to a PR which updates your gitlab or github workflows to include a shellcheck test?

There are official github actions from the shellcheck maintainer, but integrating it into an existing ci script is also easy, provided you have the binary (it's an open source haskell codebase).

[yoe]

We already have a number of static analysis tools in the gitlab config, I don't see any reason to reject more :)

Our github CI is not as extensive as the gitlab one at this point, but that's not on purpose; it's just that when I started working on CI, the gitlab actions were not as versatile as they are today. So if you want to add that in addition to something on the gitlab CI, I'm not opposed.

All that said, however, take note of the following:

• Please make sure the code passes the test when you file the PR, otherwise we have to spend time that we might not have.
• I might disagree with some of the decisions of the tool if they seem wrong or not applicable, so if the tool doesn't allow to override something where it makes sense, then that's a big no for me
• Newer versions of static analysis tools often introduce new failing tests. Because of how our build system is set up, this tends to happen at a time when we are under time pressure to release a new version of the software, which relies on CI functioning. For this reason, I prefer the tool to produce a warning rather than failing the pipeline entirely. After all, a new test doesn't suddenly make the code more buggy, it only discovers a new potential issue.

[actualben]

• OK I'll work on the PRs and hold them until they can pass the existing checks.
• Shellcheck supports both inline ignore comments and global check exclusions, so it's flexible
• It's possible to pin the shellcheck version in the ci config to prevent surprises, but I'll make sure the CI PR is for a warning-only check rather than a blocking check

[yoe]

• It's possible to pin the shellcheck version in the ci config to prevent surprises,

I know, but I prefer not to do that, as it creates unnecessary churn.