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

[Greg Chicares]

On 2021-08-17 13:17, Vadim Zeitlin wrote:
> 
>  Debian Sid has updated shellcheck to 0.7.2 version

And 'bullseye' has become the new 'stable' as of 2021-Aug-14.
Before I upgrade my host OS, let me ask whether you have
upgraded to 'bullseye' yourself and found the migration
to go smoothly. (Probably you did, and probably it did, but
I figure it can't hurt to ask.)

I've upgraded my 'testing' chroot and now have shellcheck-0.7.1
there--not 0.7.2, so I can't conveniently see the diagnostics
you mention. But the log you presented should be all I need.

And I tested all my committed but unpushed changes here:
  https://www.shellcheck.net/
so that probably means they're right.

>  First of all, let me say that if you don't care about the details, these
> errors can be fixed by merging my shellcheck-0.7.2-fixes branch
> corresponding to https://github.com/let-me-illustrate/lmi/pull/189

Thanks, but I want to consider them in detail.

>  I can reproduce these errors here, even though they didn't (and still
> don't) appear with shellcheck 0.7.1

Confirmed.

> so it's really due to the changes in
> the new shellcheck version. There seem to be at least 2 of them:
> 
> 1. It now complains about the use of non-POSIX features such as process
>    substitution, brace expansion and arrays in POSIX sh scripts. We don't
>    actually use any of those in the sh scripts, but we pretend that zsh
>    scripts are sh scripts as shellcheck doesn't know about zsh at all (and
>    probably won't any time soon, seeing how the discussion in
>    https://github.com/koalaman/shellcheck/issues/809 went absolutely
>    nowhere since 5+ years). The simplest solution I can think of is to just
>    pretend that zsh is ksh instead, as it's fairly compatible with it and
>    this is enough to avoid the warnings.

The solution I've been using is to pretend that zsh is sh, and
disable each warning specifically. Now that dash is directly
supported, I should pretend that zsh is dash instead.

> 2. It also complains about the use of non-POSIX "local" in set_toolchain.sh.
>    This one is indeed a /bin/sh script, but an existing comment there
>    explains that using "local" is actually fine and the corresponding
>    shellcheck warning was already disabled. However the new version changes
>    the warning given for "local", so the fix here is to just disable the
>    new warning too.

That sounds good. Maybe it won't even be necessary if we treat
zsh as dash.