Re: [lmi] [PATCH] Fix shellcheck 0.7.2 errors in the CI builds

[Vadim Zeitlin]

On Thu, 19 Aug 2021 11:13:09 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 2021-08-19 02:01, Greg Chicares wrote:
GC> > On 2021-08-17 13:17, Vadim Zeitlin wrote:
GC> > 
GC> > [...] I tested all my committed but unpushed changes here:
GC> >   https://www.shellcheck.net/
GC> > so that probably means they're right.
GC> 
GC> Pushed now.

 Thanks, I can confirm that this fixes the problem and all jobs at
https://github.com/let-me-illustrate/lmi/actions/runs/1146809069 are now
green again.

GC> >> 1. It now complains about the use of non-POSIX features such as process
GC> >>    substitution, brace expansion and arrays in POSIX sh scripts.
GC> 
GC> Formerly, SC2039 inhibited all such warnings. In shellcheck-0.7.2,
GC> SC2039 has been "retired" and no longer inhibits them, so we need
GC> to disable each one. But we still need to inhibit SC2039 for
GC> shellcheck-0.7.0, which doesn't recognize the new SC3XXX codes.
GC> 
GC> > The solution I've been using is to pretend that zsh is sh, and
GC> > disable each warning specifically. Now that dash is directly
GC> > supported, I should pretend that zsh is dash instead.
GC> 
GC> Belay that. Pretending that zsh is dash just gives one omnibus
GC> warning like SC2039 (with the up-to-the-minute shellcheck.net).
GC> It is better to have the specific warnings that shellcheck gives
GC> when we pretend that zsh is sh.

 I'm not sure if I really agree with the logic here, why is it better to
pretend that zsh doesn't support (many) features that it does support and
disable the warnings about them, rather than avoid the warnings in the
first place? zsh should be 99% upwards-compatible with ksh (and probably
with dash too, although I'm less sure about this), so IMO it makes much
more sense to handle zsh scripts as {da,k}sh than just plain sh. Of course,
this is not important enough to spend much time on this, so if you feel
strongly that the current approach is better, I'm not going to waste your
time by arguing.

 But, while we're on the topic of shellcheck, I'm more interested in
another question: why do we use the sed hack instead of using shellcheck
--shell=sh option? The commit adding it[*] was included in v0.3.1 of
shellcheck and even (the now old) Debian stable has v0.5.0, so there
shouldn't be any problem with using it. And it would make check_script.sh
slightly simpler and, more importantly, result in more useful "In foo.sh
..." error output rather than "In - ... in file foo.sh" that we have now
and which is much more difficult to parse (for humans). Should I make the
trivial patch changing check_script.sh to use this option or is there some
reason for not using it that I'm missing?

[*]: 
https://github.com/koalaman/shellcheck/commit/4968e7d9ffa83a5389e92c3f1d4786ca88d687c8

 Thanks in advance,
VZ

pgponXCwXhGdk.pgp
Description: PGP signature