ci: add shellcheck gh action, lint fixes, parallel acts

[rockett-m]

This adds a shellcheck github workflow to check lint our shell files after discussion with @HalosGhost

I fixed a bunch of warnings / errors / info messages from shellcheck across our *.sh scripts.

Edit: added
I assume we will want a scripts/shellcheck.sh script to tidy up the .github/workflows/ci.yml :: shellcheck stage - and to have an easy way for users to run shellcheck / view reports.

act -j shellcheck --container-architecture linux/amd64

[HalosGhost]

There are a lot of style-only changes included here (namely, a dramatic but non-universal adoption of bracing variable expansions); I'm fine with these being included (they are, at worst, benign so long as the braces don't change the expansion, which some of the included ones do), but it's a ton of noise making the review much harder.

In the future, any style-only changes should be in a separate commit so they can be reviewed separately; ideally including a script which could be used to apply the style change so that the reviewer can verify the changes are entirely mechanical style modifications.

[rockett-m]

@HalosGhost I did see the braces recommendation a lot via shellcheck. A warning not an error. I tried to clear as many warnings as I could for this PR

https://www.shellcheck.net/wiki/SC2250

[rockett-m]

Divide and conquer PR new plan
Commit 1: ci.yml and shellcheck.sh and any fatal errors across *.sh files (so the stage can pass)
Commit 2: shellcheck info and warning messages fixed to *.sh files
Commit 3: style fixes etc and shebang update to *.sh scripts
Commit 4: use NUM_CORES and xargs in lint.sh for parallelization to lint.sh

[HalosGhost]

I don't know that this script is really providing a lot of benefit; It's a lot of boilerplate for what is essentially just

git ls-files '*.sh' | xargs -n 1 -P "$CPUS" shellcheck

But it adds the ceremony of dep-checking, writing the output to a file, and some ad-hoc inspection of that file. The only strict benefit is that it offers a shortcut for people not familiar with shellcheck.

Especially since I expect the GH action that uses this script to be much slower than the native action (because the native action doesn't need to spin up a bare ubuntu container, clone the repo, install all the dependencies, and then run this script).

This is the strongest argument I've seen so far for a command-runner (the least interesting use of Make) instead of the ad-hoc script paradigm we've had so far in this repo.

I'd love comments/thoughts from @maurermi as well. At the very least, I think the GH action should be the native form instead of this; and that immediately calls into question the benefit this script offers.

[rockett-m]

I find it very helpful for making shell script changes and being able to run scripts/shellcheck.sh to generate the report right away without having to remember the command above and then how to tee to a file etc.

[HalosGhost]

I'm always a bit hesitant to use color in shell output. If nothing else, I think it has to be used sparingly, and this strikes me as a bit excessive. Is there a specific motivation for having such colorized output?

[rockett-m]

I find its much easier to understand the key insights quickly with the color. A friend who works in UI actually recommended I add it. We do have color output in some of the other scripts such as ./scripts/install-build-tools.sh and ./scripts/setup-dependencies.sh
failing:

passing:

The shellcheck report itself ./shellcheck-report.txt does not have color.

[HalosGhost]

The report can have color, because shellcheck supports configurable colorization.

Other scripts do have color, and we should unify them at some point to behave well for accessibility reasons (color can be helpful, but it can also be terrible; the most accessible option is to support configuration). However, most of those are either run-once, run-super-rarely, or are intended for pipelines and not really for humans (like the end-to-end kubernetes test). If we want to support colorization in this tool, we should at least support a basic way of disabling/enabling it (as shellcheck does).

The gold standard is to offer -c|--color[=WHEN] where WHEN can be one of auto (which disables color when the output is going to a pipe or file, and is a bit tricky to really get right), always, and never (defaulting to auto); and, to support the NO_COLOR environment variable. We should, at least, support the env-var if not something more flexible.

[rockett-m]

ACK - gave this a shot

[HalosGhost]

Can you clarify for me why you prefer having the report saved to a file instead of just sending to stdout? This adds a lot of complexity and requires you to deal with things like cache-invalidation.

[rockett-m]

I allow for both. Send to file always, and --view option to see stdout.
When I am fixing errors I can search very easily in a file, but the stdout could exceed 1000 lines of output and be harder to view the errors of a code. Can grep the stdout but error messages take different amounts of line space so would have to play with grep -B2 -A4 etc.

[HalosGhost]

You barely need to add anything to support outputting to a file (because shells support redirection). Is overriding this default behavior (how almost all other command-line utilities work) of-use? Why not just redirect the output to a file as you need to?

[rockett-m]

It creates two files now so this wouldn't be trivial anymore

[HalosGhost]

It now creates two files by running shellcheck twice every time, once at the requested severity level, and one bypassing the user's choice to make a "full" report. Why do this?

[rockett-m]

The default for shellcheck is 'style' and that seems too strict to make a github action/prevent code from getting in. My script defaults to 'error'.

There are over 100 messages of severity stricter than 'error' (warning, info, style) which I don't want myself or others to be blind to when turning in shell scripts or modifying them.

Very good to have the full report for commit 2 to mow down (warning, info, style) messages. And future reference. Plus being able to see if a certain code is a nuisance more than a help and to deliberate over making it an exclude if so.

[HalosGhost]

“It would be better to give contributors more feedback than is enforced or asked for” does not feel sufficient to me to rationalize the additional complication of bypassing the standard convention of printing output to stdout (which can be captured by file-redirection should the caller desire). Moreover, even though deriving from a positive motivation, taking an argument to specify severity level and then ignoring it intentionally (note that this will duplicate effort entirely if the user specifies the severity to be equal to shellcheck's default) comes close to being user-hostile (and our users in this case, are contributors)!

If you feel strongly that we should prefer a less restrictive severity level by-default, make the argument for why (and we would instead default to that severity level, perhaps disabling specific codes we find less-than-useful).

[rockett-m]

Error is the least restrictive severity level as anything more strict gets ignored in pass/fail of the stage.
What if we changed it to warning or info as default and used whichever one in the gh action? Can discuss in slack/zoom if need be.

[HalosGhost]

My phrasing might have been confusing. I meant less-restrictive in the sense of “print more diagnostics”. As I alluded to, I'm comfortable with “raising” the default from error to something higher, but I would ask that you make an argument for why we should prefer that. I appreciate unifying standards across the repository, and improving the CI/CD, and raising the bar for quality of code committed; but, for a research project predominantly focused on engineering novel solutions and not-at-all currently driven by productionization, these improvements are quality-of-life, not top-priority or of most research-interest. Adding friction for contributors absolutely must be justified; it isn't free.

[rockett-m]

@HalosGhost Incorporated your suggestions. Added more intelligent coloring, waived error codes/severity messages, summary report.

Can be viewed here: https://github.com/mit-dci/opencbdc-tx/actions/runs/10501604065/job/29091863763?pr=298

The waivers are more powerful when looking beyond errors (majority of the info warnings are about variable quotations 'SC2086')

$ ./scripts/shellcheck.sh -C auto -S info -e SC2034 -e SC2086 -e SC1010

Edit: I will add a path to the full report as well in the next push

[HalosGhost]

I'll make a full review after the file-output/severity issue has been appropriately resolved or justified.

[HalosGhost]

This is fine, but auto should be removed from the help output, it's not being handled (which is okay! handling auto-coloring is a bit sticky).