Bug 579062 – sys-apps/gentoo-functions: fails with `set -u` due to unset CONSOLETYPE

• Product: Gentoo Linux
• Status: RESOLVED FIXED

[Description Stuart Shelton 2016-04-05 01:13:55 UTC]

functions.sh from sys-apps/gentoo-functions-0.10 causes scripts which source it to fail under 'set -u' (fail on use of unbound variables) due to CONSOLETYPE (line 423) being used undefined.

However, I also notice that functions.sh starts with:

#
# All functions in this file should be written in POSIX sh. Please do
# not use bashisms.
#

... but then proceeds to use 'local' (sometimes with multiple arguments) and 'test -nt', both of which are valid in bash but are undefined in POSIX sh.

(Source: https://wiki.ubuntu.com/DashAsBinSh, http://mywiki.wooledge.org/Bashism)

Instead targeting dash or bb ash may give the desired flexibility without needing nasty workarounds to attain strict POSIX compliance...

[Comment 1 Mike Gilbert 2016-04-05 13:14:57 UTC]

In the future, please file separate bugs for separate issues.

[Comment 2 SpanKY 2016-04-12 08:21:33 UTC]

Gentoo has long accepted `local` as usable even in "POSIX" shell code as all relevant shells have long supported it.  this isn't specific to gentoo-functions.

[Comment 3 Stuart Shelton 2016-04-12 10:06:23 UTC]

Sure, that part may well be a documentation issue... perhaps a link to a wiki page or similar documenting the features beyond what is supported by pure POSIX sh which are considered to be standard/supported for Gentoo?

(I guess we're going for anything supported by ash/dash, to provide the option for a reduced shell for use by init/openrc?  I've read reports of Debian booting noticeably faster when they moved from bash to dash as /bin/sh for system scripts.)

[Comment 4 SpanKY 2016-04-12 16:22:08 UTC]

(In reply to Stuart Shelton from comment #3)

"whatever ash/dash supports" is not our bar.  it's "whatever is in POSIX and `local`".  when we weighed the benefits of `local` vs finding a relevant shell that *didn't* support it (which we couldn't), it was pretty easy to just say "`local` is allowed".  it's been discussed on the gentoo-dev list but i don't know it's been spelled out in the wiki.

[Comment 5 Stuart Shelton 2016-04-12 23:14:45 UTC]

Created attachment 430268 [details, diff]
functions.sh patch to add shellcheck compliance


Okay, cool - how about then:

"
#
# All functions in this file should be written in POSIX sh, with the
# addition of "local" to set variable scope. Please do not use other
# bashisms.
#
"

(plus the change of "${CONSOLETYPE}" to "${CONSOLETYPE:-}" on line 423)?

In case anyone's interested I've attached a patch which passes shellcheck validation, FWIW.

[Comment 6 Sam James 2022-11-12 07:15:01 UTC]

[15:40:52]  <+floppym> sam_: I think it was "fixed" indirectly by https://gitweb.gentoo.org/proj/gentoo-functions.git/commit/?id=1c801ce130726a81c59b96cdf4e2bef27893e0b7
[15:41:12]  <+floppym> (the reference to CONSOLETYPE was removed)

We should still apply the patch though.

[Comment 7 kfm 2022-11-12 08:54:27 UTC]

The patch looks good overall. However, unless the variables containing the ECMA-48 sequences are defined in a different manner, the use of %b in the format strings doesn't make any sense. Let's consider why.

$ WARN=$(printf '\033[33;01m')

It can be proven that this assigns a valid ECMA-48 byte sequence to WARN.

$ printf '%s' | od -An -a
 esc   [   3   3   ;   0   1   m

The use of %s is quite deliberate there. To use %b would imply that backslash-escapes should be interpreted, which is not presently the case. Instead, the value of the variable is already a valid ECMA-48 byte sequence and, by definition, should not be subject to modification in any way, even theoretically.

The same criticism can be levelled at the current code because it injects the value of the variable into the format string, effectively acting as %b would.

So, for %b to make any sense, the variable assignments should instead follow this convention.

$ WARN='\033[33;01m'

There, we don't yet have a valid ECMA-48 sequence, byte-wise.

$ printf '%s' "$WARN" | od -An -a
\033[33;01m

However, using %b would then interpet the string in the appropriate fashion.

$ printf '%b' "$WARN" | od -An -a
 esc   [   3   3   ;   0   1   m

In summary, rendering the ECMA-48 sequences by way of operands following the format string would be sensible, but the applicable variables should then be defined in a fashion that is commensurate with the approach. Should the code continue to use a comsub/printf combination to assign to the variables, %s would be appropriate. Otherwise, it could be %b.

The latter approach would allow for the script to forgo at least six subshells, which seems like an appealing prospect to me.

[Comment 8 kfm 2022-11-12 08:58:20 UTC]

There was a rather minor copy/paste mishap in the previous post. To clarify:

$ WARN='\033[33;01m'
$ printf '%s' "$WARN" | od -An -a
   \   0   3   3   [   3   3   ;   0   1   m
$ printf '%b' "$WARN" | od -An -a
 esc   [   3   3   ;   0   1   m

[Comment 9 kfm 2022-11-12 09:01:30 UTC]

Logically, it follows that, for as long as tput is allowed to be used to generate the ECMA-48 sequences, using %b is a non-starter.

[Comment 10 kfm 2023-02-15 08:56:07 UTC]

(In reply to Stuart Shelton from comment #5)
> Okay, cool - how about then:
> 
> "
> #
> # All functions in this file should be written in POSIX sh, with the
> # addition of "local" to set variable scope. Please do not use other
> # bashisms.

Commit 279ff36 added a global shellcheck hint and commit f100672 removed the comment advising against the use of "bashisms". The hint is as follows.

# shellcheck shell=sh disable=3043

I believe this to be sufficient because it indicates that sh is the target, with SC3034 being an exempted warning class (that concerns the use of local). I don't very much like local either, by the way. For what it is worth, the use of local has been somewhat curtailed.

As of commit 9301549, functions.sh raises no shellcheck warnings, though there are a few necessary exemptions here and there.

As was noted by comment 6, the reference to CONSOLETYPE was removed, so I suppose that this bug can now be closed. Thank you for taking the time to prepare and submit the patch, even though it was not directly applied.