| Message ID | 20220720152631.450903-4-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | configure: fix some non-portabilities | expand |
On 20/07/2022 17.26, Peter Maydell wrote: > The variable string-replacement syntax ${var/old/new} is a bashism > (though it is also supported by some other shells), and for instance > does not work with the NetBSD /bin/sh, which complains: > ../src/configure: 687: Syntax error: Bad substitution > > Replace it with a more portable sed-based approach, similar to > what we already do in quote_sh(). > > Note that shellcheck also diagnoses this: > > In ./configure line 687: > e=${e/'\'/'\\'} > ^-----------^ SC2039: In POSIX sh, string replacement is undefined. > ^-- SC1003: Want to escape a single quote? echo 'This is how it'\''s done'. > ^-- SC1003: Want to escape a single quote? echo 'This is how it'\''s done'. > > > In ./configure line 688: > e=${e/\"/'\"'} > ^----------^ SC2039: In POSIX sh, string replacement is undefined. > > Fixes: 8154f5e64b0cf ("meson: Prefix each element of firmware path") > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > configure | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) Thanks, this fixes "make vm-build-netbsd" for me! Tested-by: Thomas Huth <thuth@redhat.com>
On Wed, Jul 20, 2022 at 04:26:29PM +0100, Peter Maydell wrote: > The variable string-replacement syntax ${var/old/new} is a bashism > (though it is also supported by some other shells), and for instance > does not work with the NetBSD /bin/sh, which complains: > ../src/configure: 687: Syntax error: Bad substitution > > Replace it with a more portable sed-based approach, similar to > what we already do in quote_sh(). > > for e in $1; do > - e=${e/'\'/'\\'} > - e=${e/\"/'\"'} > - printf '"""%s""",' "$e" > + printf '"""' > + # backslash escape any '\' and '"' characters > + printf "%s" "$e" | sed -e 's/\([\"]\)/\\\1/g' You've fixed the bashism, but at the expense of a non-POSIX use of sed. POSIX says the input to sed must be a text file (ending in a newline; but $e does not), and as a result it always outputs a newline (but you don't want a newline before the closing """). GNU sed happens to do what you want for input not ending in a newline, but I don't remember off-hand whether BSD sed does, and I know that Solaris sed does not. If this passes on BSD, then I'm okay with it; but if we want to avoid non-POSIX altogether, this should work (using the shell's $() to strip the trailing newline we added to keep sed happy): # backslash escape any '\' and '"' characters printf '"""%s""",' "$(printf "%s\n" "$e" | sed -e '/s/\([\"]\)/\\\1/g')" Your call.
On Wed, 20 Jul 2022 at 17:30, Eric Blake <eblake@redhat.com> wrote: > > On Wed, Jul 20, 2022 at 04:26:29PM +0100, Peter Maydell wrote: > > The variable string-replacement syntax ${var/old/new} is a bashism > > (though it is also supported by some other shells), and for instance > > does not work with the NetBSD /bin/sh, which complains: > > ../src/configure: 687: Syntax error: Bad substitution > > > > Replace it with a more portable sed-based approach, similar to > > what we already do in quote_sh(). > > > > > for e in $1; do > > - e=${e/'\'/'\\'} > > - e=${e/\"/'\"'} > > - printf '"""%s""",' "$e" > > + printf '"""' > > + # backslash escape any '\' and '"' characters > > + printf "%s" "$e" | sed -e 's/\([\"]\)/\\\1/g' > > You've fixed the bashism, but at the expense of a non-POSIX use of > sed. POSIX says the input to sed must be a text file (ending in a > newline; but $e does not), and as a result it always outputs a newline > (but you don't want a newline before the closing """). GNU sed > happens to do what you want for input not ending in a newline, but I > don't remember off-hand whether BSD sed does, and I know that Solaris > sed does not. I just copied the approach we already take in quote_sh: quote_sh() { printf "%s" "$1" | sed "s,','\\\\'',g; s,.*,'&'," } Is that also relying on this non-portable sed use? > If this passes on BSD, then I'm okay with it; but if we want to avoid > non-POSIX altogether, this should work (using the shell's $() to strip > the trailing newline we added to keep sed happy): > > # backslash escape any '\' and '"' characters > printf '"""%s""",' "$(printf "%s\n" "$e" | sed -e '/s/\([\"]\)/\\\1/g')" Mmm. -- PMM
On Wed, Jul 20, 2022 at 06:32:22PM +0100, Peter Maydell wrote: > > > + # backslash escape any '\' and '"' characters > > > + printf "%s" "$e" | sed -e 's/\([\"]\)/\\\1/g' > > > > You've fixed the bashism, but at the expense of a non-POSIX use of > > sed. POSIX says the input to sed must be a text file (ending in a > > newline; but $e does not), and as a result it always outputs a newline > > (but you don't want a newline before the closing """). GNU sed > > happens to do what you want for input not ending in a newline, but I > > don't remember off-hand whether BSD sed does, and I know that Solaris > > sed does not. > > I just copied the approach we already take in quote_sh: > > quote_sh() { > printf "%s" "$1" | sed "s,','\\\\'',g; s,.*,'&'," > } > > Is that also relying on this non-portable sed use? Yep. And since no one has complained, BSD sed probably handles it the way we want (input without a trailing line getting substituted and re-output without a trailing newline). If we cared, this one could also be fixed: quote_sh() { printf "'%s'" "$(printf '%s\n' "$1" | sed "s,','\\\\'',g")" } Also note that we are depending on $1 never containing newlines, because this change would alter the existing: $ quote_sh "a c" 'a' 'c' into: $ quote_sh "a c" 'a c' which is probably more what you wanted. > > > If this passes on BSD, then I'm okay with it; but if we want to avoid > > non-POSIX altogether, this should work (using the shell's $() to strip > > the trailing newline we added to keep sed happy): > > > > # backslash escape any '\' and '"' characters > > printf '"""%s""",' "$(printf "%s\n" "$e" | sed -e '/s/\([\"]\)/\\\1/g')" > > Mmm. Given that no one has complained about quote_sh, insisting on POSIX compliance (with its resulting increase in line noise and length) is not winning any maintainability arguments ;)
diff --git a/configure b/configure index a56c3d921be..c05205b6085 100755 --- a/configure +++ b/configure @@ -684,9 +684,10 @@ meson_option_build_array() { IFS=: fi for e in $1; do - e=${e/'\'/'\\'} - e=${e/\"/'\"'} - printf '"""%s""",' "$e" + printf '"""' + # backslash escape any '\' and '"' characters + printf "%s" "$e" | sed -e 's/\([\"]\)/\\\1/g' + printf '""",' done) printf ']\n' }
The variable string-replacement syntax ${var/old/new} is a bashism (though it is also supported by some other shells), and for instance does not work with the NetBSD /bin/sh, which complains: ../src/configure: 687: Syntax error: Bad substitution Replace it with a more portable sed-based approach, similar to what we already do in quote_sh(). Note that shellcheck also diagnoses this: In ./configure line 687: e=${e/'\'/'\\'} ^-----------^ SC2039: In POSIX sh, string replacement is undefined. ^-- SC1003: Want to escape a single quote? echo 'This is how it'\''s done'. ^-- SC1003: Want to escape a single quote? echo 'This is how it'\''s done'. In ./configure line 688: e=${e/\"/'\"'} ^----------^ SC2039: In POSIX sh, string replacement is undefined. Fixes: 8154f5e64b0cf ("meson: Prefix each element of firmware path") Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- configure | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)