Re: [PATCH v2 1/4] Replace 'test "${var+set}" = set' with '[ ${var+y} ]'

[Eric Blake]

On 04/09/2015 07:54 AM, Bernhard Reutner-Fischer wrote:

>>> +++ b/lib/autoconf/general.m4
>>> @@ -1467,7 +1467,7 @@ _AC_ENABLE_IF_ACTION([$1], m4_translit([$2], [-+.], 
>>> [___]), [$3], [$4])
>>>  m4_define([_AC_ENABLE_IF_ACTION],
>>>  [m4_append_uniq([_AC_USER_OPTS], [$1_$2], [
>>>  ])dnl
>>> -AS_IF([test "${$1_$2+set}" = set], [$1val=$$1_$2; $3], [$4])dnl
>>> +AS_IF([[[ ${$1_$2+y} ]]], [$1val=$$1_$2; $3], [$4])dnl
>>
>> This code snippet definitely appears multiple times in typical configure
>> files, so it's nice to shave in size.
>>
>> However, this is one of those cases where you may have broken things.
> 
> If i did then we should add a testcase for this case for sure. I don't
> immediately see
> potential problems with that though, IIRC m4 should expand $digit just
> fine even after
> having seen an escaped '[', no?

No. M4 expands $digit early, as part of macro expansion, and THEN does
another pass over the expansion for any further macro names.  That is,
if I call _AC_ENABLE_IF_ACTION([foo], [bar]), the old code would expand
to AS_IF([test "${foo_bar+set}" = ..., which would then expand AS_IF()
into something like: if test "${foo_bar+set}" = ..., then check if 'if'
or 'foo_bar' or 'set' is a macro name.  The new code would expand to
AS_IF([[[ ${foo_bar+y} ]]], ..., which would then expand AS_IF() into
something like: if [[ ${foo_bar+y} ]] ..., then check if 'if' is a
macro, and strip a layer of quotes from the string [[ ${foo_bar+y} ]]
without checking whether foo_bar or y is a macro.  Of course, I
seriously doubt anyone would name an m4 macro 'if' or 'foo_bar', but I
can't guarantee it, and therefore it is a subtle semantic change
(foo_bar is now quoted, where before it was not) that requires an audit;
whereas sticking to 'test' instead of [] doesn't change quoting and
therefore cannot cause any regressions for an unexpected corner case.

And maybe we can argue that the extra quoting is a bug fix (that is, a
user is unlikely to EVER want foo_bar expanded as a potential m4 macro
name in this particular context), but if so, we should do that as a
separate commit, and it should look more like AS_IF([[test
"${$1_$2+set}" = set]], ...) (that is, change the quoting independently
from changing the text that gets output in the normal case of no
colliding macros).

> 
>> If $1 or $2 is an m4 macro name, the old code would recursively expand
>> that macro, the new code will not.  (I don't know if someone ever passed
>> $1 or $2 as a macro name, but my point is that avoiding the need to
>> audit whether subtle semantic changes matter is the better approach).
>> That, and '[[[' looks awkward.
> 
> m4 is great ;)

Yes, it's a neat language.

> 
>>
>>>  ])
>>>
>>>  # AC_ARG_ENABLE(FEATURE, HELP-STRING, [ACTION-IF-TRUE], [ACTION-IF-FALSE])
>>> @@ -2044,7 +2044,7 @@ _AC_CACHE_DUMP() |
>>>       /^ac_cv_env_/b end
>>>       t clear
>>>       :clear
>>> -     s/^\([^=]*\)=\(.*[{}].*\)$/test "${\1+set}" = set || &/
>>> +     s/^\([^=]*\)=\(.*[{}].*\)$/[ ${\1+y} ] || &/
>>
>> Given the [] earlier in the line, it looks like the added use of [] here
>> is at the right level of nesting, so this one is probably safe. But this
>> macro is not frequently output into configure scripts, so it is not
>> saving as much file size.
> 
> the nesting is ok, it's emitted properly in the configure files i diffed.
> Fine to drop this hunk if you prefer.

I don't mind using shorter invocations; so I won't completely drop this
hunk.  I'm just arguing that the difference of one byte between [ ] and
test is not worth the break in style, and so we might as well use test
like always.


>> This pattern for setting a variable to a default value also occurs in
>> the manual under the section on ${var=value}; we should probably update
>> that recommendation to the shorter form, since we should follow our own
>> documentation.  Or we should determine if we can portably use the even
>> shorter ${CONFIG_COMMANDS=$config_commands} these days, after properly
>> rejecting ancient broken shells.
> 
> ${x=$default} is even better, agree.
> Given that this was even in SUSv3 it should be reasonable safe to use
> this by now.

Not all the world is SUSv3.  We have a section in the manual for a reason.
https://www.gnu.org/software/autoconf/manual/autoconf.html#Shell-Substitutions

| The portable way out consists in using a double assignment, to switch
the 8th bit twice on Ultrix:
|
|           list=${list="$default"}
|
| ...but beware of the ‘}’ bug from Solaris (see above). For safety, use:
|
|           test "${var+set}" = set || var={value}

Okay, so maybe we can argue that no one uses autoconf to generate
configure scripts for Ultrix any more, and that the Solaris bug is
something we easily avoid (because we require a shell with functions),
so maybe we CAN now get away with the shorter default assignment form;
but it should be its own commit and testsuite enhancements to make sure
we aren't overlooking some other broken shell that is used in the wild.

> [I would just bundle a sane fallback shell with autoconf and nuke all
> the legacy support right away, but well]

It's not autoconf that needs the bundling, it is the configure script
for EVERY PACKAGE.  Yes, autoconf could bundle the dash tarball
alongside every configure script it generates, and then make the running
of './configure' result in first building up dash then running dash on
any other scripting - but I don't think end users would appreciate the
penalty in tarball size or in lengthened configure times.  And if you
are arguing that we should pack a sane shell alongside every configure
script, then go one step further: write configure operations in a sane
language like C, and then './configure' consists of first using a
brain-dead known-to-work-everywhere recipe for compiling the C driver,
then run the C code that probes for everything else related to
portability for the real package that we currently now do in shell.  It
would probably result in smaller tarballs (how many times have you seen
configure larger than a megabyte?).  But not the sort of undertaking I
want to do.  Or write configure operations in a different meta-language,
like GNU Make (I think GNU Quagmire is a project that attempted this),
so that ./configure is a no-op, and 'make' then requires GNU Make to be
present, but is able to determine all the configuration options that the
autotools did via configure.  And unless your new tool behaves
identically to current autoconf at parsing configure.ac into your cool
smarter-language configuration driver, good luck on overcoming inertia
to get projects to use your ideas instead of sticking with what already
works, even if what already works is clunky.

> 
>>> --- a/tests/m4sh.at
>>> +++ b/tests/m4sh.at
>>> @@ -297,7 +297,7 @@ test $as_lineno = 9999 || AS_ERROR([bad as_lineno at 
>>> depth 2])
>>>  AS_LINENO_POP
>>>  test $as_lineno = 9999 || AS_ERROR([bad as_lineno at depth 1])
>>>  AS_LINENO_POP
>>> -test x${as_lineno+set} = xset && AS_ERROR([as_lineno set at depth 0])
>>> +test "${as_lineno+y}" && AS_ERROR([as_lineno set at depth 0])
>>
>> Ahh, you kept 'test' here.
> 
> yea, that was on purpose for consistency with the 'test' above :)

And yet, this is a case where the "" are superfluous - as 'test' and
'test ""' behave identically, and since the use of ${as_lineno+y}
guarantees an expansion that does not need protection with "" to avoid
syntax issues with test.

I'll probably apply something along the lines of what you have been
attempting, or at least post a revised version for your sign-off before
applying anything, because I do think there is merit in what you are
trying to do; it's just that we aren't quite yet at the fully polished
version.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature