Package: automake;
Reported by: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Tue, 6 Dec 2011 17:53:02 UTC
Severity: minor
Tags: patch
Done: Stefano Lattarini <stefano.lattarini <at> gmail.com>
Bug is archived. No further changes may be made.
Message #78 received at 10237 <at> debbugs.gnu.org (full text, mbox):
From: Stefano Lattarini <stefano.lattarini <at> gmail.com> To: Paul Eggert <eggert <at> cs.ucla.edu> Cc: 9928 <at> debbugs.gnu.org, Eric Blake <eblake <at> redhat.com>, 10237 <at> debbugs.gnu.org, skunk <at> iskunk.org Subject: Re: bug#9928: bug#10237: AM_SILENT_RULES does not work with NonStop make Date: Wed, 21 Dec 2011 13:21:26 +0100
Hi Paul, thanks for the respin. My comments and objections are inlined.
On 12/21/2011 02:30 AM, Paul Eggert wrote:
> On 12/11/11 01:42, Stefano Lattarini wrote:
>
>> I was asking for a test case that simulates the presence of a
>> make implementation unable to grasp nested variable expansions
>
> Ah, OK, revised patch enclosed below. This patch should address
> your other comments too. Thanks for the careful review.
>
> From 07edd4aa81af2a8fb982427705a2009c2b7eb0bf Mon Sep 17 00:00:00 2001
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Date: Tue, 20 Dec 2011 17:29:06 -0800
> Subject: [PATCH] automake: silent-rules no longer requires nested vars
>
This sounds a little misleading to me; i.e., it sounds like we have
reimplemented the silent-rules feature to work completely and at its
best even with make implementations not supporting nested variables,
which is not true. What about this instead?
silent-rules: provide fallback for makes without nested variables support
> This fixes two problems reported for Automake (Bug#9928, Bug#10237)
> and is in response to a bug report for building coreutils on HP
> NonStop OS (Bug#10234). The problem is that HP NonStop 'make'
> treats a line like "AM_V_CC = $(am__v_CC_$(V))" as one that
> expands a macro with the funny name am__v_CC_$(V instead of the
> desired name am__v_CC_1 or am__v_CC_0, and since the funny macro
> is not defined the line is equivalent to "AM_V_CC = )"; this
> inserts a stray ")" when $(AM_V_CC) is used, which eventually
> causes 'make' to fail.
>
Very good explanation.
> The basic idea is that instead of generating Makefile.in lines like
> "AM_V_CC = $(am__v_CC_$(V))", we generate
> "AM_V_CC = $(am__v_CC_ <at> AM_V@)". We then AC_SUBST $(V) for @AM_V@
> in the usual case where `make' supports nested variables,
> and substitute 1 (or 0) otherwise. Similarly for usages like
> $(am__v_CC_$(AM_DEFAULT_VERBOSITY)).
>
> With this change, make implementations that doesn't grasp nested
> variable expansions will still be able to run Makefiles generated
> using the silent-rules option. They won't allow the user to
> override the make verbosity at runtime through redefinition of
> $(V) (as in "make V=0"); but this is still an improvement over not
> being able to work at all.
>
And all this is good too. Thanks.
> * NEWS: Document this.
> * automake.in (define_verbose_var): When defining the variable,
> use @AM_V@ rather than $(V), and use
> @AM_AM_DEFAULT_VERBOSITY@ rather than $(AM_DEFAULT_VERBOSITY).
> * doc/automake.texi (Automake silent-rules Option): Explain new system.
> * m4/silent.m4 (AM_SILENT_RULES): Check whether `make' supports
> nested variables, and substitute AM_V and AM_AM_DEFAULT_VERBOSITY
>
s/AM_AM_DEFAULT_VERBOSITY/AM_V_DEFAULT_VERBOSITY/ ?
> accordingly.
> * tests/silent-nested-vars.test: New file.
>
Micro-nit: I'd prefer s/file/test/ here (but this is not a requirement
for an ACK!).
> * tests/Makefile.am (TESTS): Add it.
> * tests/Makefile.in: Regenerate.
>
Small nit: we don't mention changes to autogenerated-but-committed files
in our ChangeLog entries and commit messages.
> ---
> ChangeLog | 40 +++++++++++++++
> NEWS | 7 +++
> automake.in | 9 ++-
> doc/automake.texi | 22 +++++----
> m4/silent.m4 | 33 +++++++++++-
> tests/Makefile.am | 1 +
> tests/Makefile.in | 1 +
> tests/silent-nested-vars.test | 111 +++++++++++++++++++++++++++++++++++++++++
> 8 files changed, 209 insertions(+), 15 deletions(-)
> create mode 100755 tests/silent-nested-vars.test
>
> diff --git a/ChangeLog b/ChangeLog
> index 49b6e8b..b2d6e11 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,43 @@
> +2011-12-11 Paul Eggert <eggert <at> cs.ucla.edu>
> +
>
[SNIP ChangeLog entry]
[The same comments given above for the git commit message applies, obviously].
> diff --git a/NEWS b/NEWS
> index 46803a7..58c90c9 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -74,6 +74,13 @@ Bugs fixed in 1.11.0a:
> not used, `make' output no longer contains spurious backslash-only
> lines, thus once again matching what Automake did before 1.11.
>
> + - The `silent-rules' option now generates working makefiles even for
> + the uncommon `make' implementations that do not support the
> + nested-variables extension to POSIX 2008. For such `make'
> + implementations, whether a build is silent is determined at
> + configure time, and cannot be overridden at make time with `make
> + V=0' or `make V=1'.
> +
Very clear, thanks.
> - The AM_COND_IF macro also works if the shell expression for the conditional
> is no longer valid for the condition.
>
> diff --git a/automake.in b/automake.in
> index db7f3c6..ae2011c 100755
> --- a/automake.in
> +++ b/automake.in
> @@ -1161,9 +1161,12 @@ sub define_verbose_var ($$)
> my $silent_var = $pvar . '_0';
> if (option 'silent-rules')
> {
> - # Using `$V' instead of `$(V)' breaks IRIX make.
> - define_variable ($var, '$(' . $pvar . '_$(V))', INTERNAL);
> - define_variable ($pvar . '_', '$(' . $pvar . '_$(AM_DEFAULT_VERBOSITY))', INTERNAL);
> + # For typical `make's, `configure' replaces AM_V (inside @@) with $(V)
> + # and AM_AM_DEFAULT_VERBOSITY (inside @@) with $(AM_DEFAULT_VERBOSITY).
>
AM_AM_DEFAULT_VERBOSITY ? More instances below, in this and other files.
> + # For strict POSIX 2008 `make's, it replaces them with 0 or 1 instead.
> + # See AM_SILENT_RULES in m4/silent.m4.
> + define_variable ($var, '$(' . $pvar . '_@'.'AM_V'.'@)', INTERNAL);
> + define_variable ($pvar . '_', '$(' . $pvar . '_@'.'AM_AM_DEFAULT_VERBOSITY'.'@)', INTERNAL);
> Automake::Variable::define ($silent_var, VAR_AUTOMAKE, '', TRUE, $val,
> '', INTERNAL, VAR_ASIS)
> if (! vardef ($silent_var, TRUE));
> diff --git a/doc/automake.texi b/doc/automake.texi
> index a5f857d..1a689b6 100644
> --- a/doc/automake.texi
> +++ b/doc/automake.texi
> @@ -10130,18 +10130,20 @@ For portability to different @command{make} implementations, package authors
> are advised to not set the variable @code{V} inside the @file{Makefile.am}
> file, to allow the user to override the value for subdirectories as well.
> A
> -The current implementation of this feature relies on a non-POSIX, but in
> -practice rather widely supported @file{Makefile} construct of nested
> -variable expansion @samp{$(@var{var1}$(V))}. Do not use the
> -@option{silent-rules} option if your package needs to build with
> -@command{make} implementations that do not support it. The
> -@option{silent-rules} option turns off warnings about recursive variable
> -expansion, which are in turn enabled by @option{-Wportability}
> -(@pxref{Invoking Automake}).
> +The current implementation of this feature normally uses nested
> +variable expansion @samp{$(@var{var1}$(V))}, a @file{Makefile} feature
> +that is not required by POSIX 2008 but is widely supported in
> +practice. On the rare @command{make} implementations that do not
> +support nested variable expansion, whether rules are silent is always
> +determined at configure time, and cannot be overridden at make time.
> +Future versions of POSIX are likely to require nested variable
> +expansion, so this minor limitation should go away with time.
>
Good and clear explanation.
> @vindex @code{AM_V_GEN}
> @vindex @code{AM_V_at}
> @vindex @code{AM_DEFAULT_VERBOSITY}
> +@vindex @code{AM_V}
> +@vindex @code{AM_AM_DEFAULT_VERBOSITY}
>
> To extend the silent mode to your own rules, you have two choices:
>
> @itemize @bullet
> @@ -10157,8 +10159,8 @@ The following snippet shows how you would define your own equivalent of
> @code{AM_V_GEN}:
>
> @example
> -pkg_verbose = $(pkg_verbose_$(V))
> -pkg_verbose_ = $(pkg_verbose_$(AM_DEFAULT_VERBOSITY))
> +pkg_verbose = $(pkg_verbose_@@AM_V@@)
> +pkg_verbose_ = $(pkg_verbose_@@AM_AM_DEFAULT_VERBOSITY@@)
>
Do we have a test verifying that the previously documented usage:
pkg_verbose = $(pkg_verbose_$(V))
pkg_verbose_ = $(pkg_verbose_$(AM_DEFAULT_VERBOSITY))
still works with make implementations that support nested variable expansion,
as it did before this change? More importantly, do we have a test verifying
that the new documented usage:
pkg_verbose = $(pkg_verbose_ <at> AM_V@)
pkg_verbose_ = $(pkg_verbose_ <at> AM_AM_DEFAULT_VERBOSITY@)
actually works as advised?
> pkg_verbose_0 = @@echo PKG-GEN $@@;
>
> foo: foo.in
> diff --git a/m4/silent.m4 b/m4/silent.m4
> index 6d2a1a2..1928feb 100644
> --- a/m4/silent.m4
> +++ b/m4/silent.m4
> @@ -1,11 +1,11 @@
> ## -*- Autoconf -*-
> -# Copyright (C) 2009 Free Software Foundation, Inc.
> +# Copyright (C) 2009, 2011 Free Software Foundation, Inc.
> #
> # This file is free software; the Free Software Foundation
> # gives unlimited permission to copy and/or distribute it,
> # with or without modifications, as long as this notice is preserved.
>
> -# serial 1
> +# serial 2
>
> # AM_SILENT_RULES([DEFAULT])
> # --------------------------
> @@ -20,6 +20,35 @@ yes) AM_DEFAULT_VERBOSITY=0;;
> no) AM_DEFAULT_VERBOSITY=1;;
> *) AM_DEFAULT_VERBOSITY=m4_if([$1], [yes], [0], [1]);;
> esac
> +dnl
> +dnl A few `make' implementations (e.g., NonStop OS and NextStep)
> +dnl do not support nested variable expansions.
> +dnl See automake bug#9928 and bug#10237.
> +am_make=${MAKE-make}
> +AC_CACHE_CHECK([whether $am_make supports nested variables],
> + [am_cv_make_support_nested_variables],
> + [if AS_ECHO([['TRUE=$(BAR$(V))
> +BAR0=false
> +BAR1=true
> +V=1
> +am__doit:
> + @$(TRUE)
> +.PHONY: am__doit']]) | $am_make -f - >/dev/null 2>&1; then
> + am_cv_make_support_nested_variables=yes
> +else
> + am_cv_make_support_nested_variables=no
> +fi])
> +if test $am_cv_make_support_nested_variables = yes; then
> + AM_V='$(V)'dnl Using `$V' instead of `$(V)' breaks IRIX make.
>
Keep this comment on a separate line maybe? That would seem clearer to me:
# Using `$V' instead of `$(V)' breaks IRIX make.
AM_V='$(V)'
> + AM_AM_DEFAULT_VERBOSITY='$(AM_DEFAULT_VERBOSITY)'
> +else
> + AM_V=$AM_DEFAULT_VERBOSITY
> + AM_AM_DEFAULT_VERBOSITY=$AM_DEFAULT_VERBOSITY
> +fi
> +AC_SUBST([AM_V])dnl
> +AM_SUBST_NOTMAKE([AM_V])dnl
> +AC_SUBST([AM_AM_DEFAULT_VERBOSITY])dnl
> +AM_SUBST_NOTMAKE([AM_AM_DEFAULT_VERBOSITY])dnl
> AC_SUBST([AM_DEFAULT_VERBOSITY])dnl
> AM_BACKSLASH='\'
> AC_SUBST([AM_BACKSLASH])dnl
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 831906b..920d994 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -751,6 +751,7 @@ silent-many-gcc.test \
> silent-many-generic.test \
> silent-lex-gcc.test \
> silent-lex-generic.test \
> +silent-nested-vars.test \
> silent-yacc-gcc.test \
> silent-yacc-generic.test \
> silent-configsite.test \
> diff --git a/tests/Makefile.in b/tests/Makefile.in
> index 3ad0146..470ed6c 100644
> --- a/tests/Makefile.in
> +++ b/tests/Makefile.in
> @@ -1035,6 +1035,7 @@ silent-many-gcc.test \
> silent-many-generic.test \
> silent-lex-gcc.test \
> silent-lex-generic.test \
> +silent-nested-vars.test \
> silent-yacc-gcc.test \
> silent-yacc-generic.test \
> silent-configsite.test \
> diff --git a/tests/silent-nested-vars.test b/tests/silent-nested-vars.test
> new file mode 100755
> index 0000000..6716930
> --- /dev/null
> +++ b/tests/silent-nested-vars.test
> @@ -0,0 +1,111 @@
> +#!/bin/sh
> +# Copyright (C) 2011 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2, or (at your option)
> +# any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +# Check silent-rules mode, on 'make' implementations that do not
> +# support nested variables.
>
Could you please add a reference to the relevant bug numbers here as well?
Thanks.
> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +mkdir sub
> +
> +cat >>configure.in <<'EOF'
> +AM_SILENT_RULES
> +AC_CONFIG_FILES([sub/Makefile])
> +AC_PROG_CC
> +AM_PROG_CC_C_O
> +AC_OUTPUT
> +EOF
> +
> +cat > Makefile.am <<'EOF'
> +# Need generic and non-generic rules.
> +bin_PROGRAMS = foo bar
> +bar_CFLAGS = $(AM_CFLAGS)
> +SUBDIRS = sub
> +EOF
> +
> +cat > sub/Makefile.am <<'EOF'
> +AUTOMAKE_OPTIONS = subdir-objects
> +# Need generic and non-generic rules.
> +bin_PROGRAMS = baz bla
> +bla_CFLAGS = $(AM_CFLAGS)
> +EOF
> +
Maybe trying out also subdir-objects is an overkill in the context of this
patch ... Oh well, than can be fixed by a later patch if the need arise,
so let's not worry about it now.
> +cat > foo.c <<'EOF'
> +int main ()
> +{
> + return 0;
> +}
> +EOF
> +cp foo.c bar.c
> +cp foo.c sub/baz.c
> +cp foo.c sub/bla.c
> +
> +cat >mymake <<'EOF'
> +#! /bin/sh
> +makerules=
> +
> +case $1 in
> + -f)
> + makefile=$2
> + case $2 in
> + -) makerules=`cat` || exit ;;
> + esac ;;
> + *)
> + for makefile in makefile Makefile; do
> + test -f $makefile && break
> + done ;;
> +esac
> +
> +if printf '%s\n' "$makerules" | LC_ALL=C grep '\$([a-zA-Z0-9_]*\$' $makefile
>
This seems wrong, as grep will not consider its standard input when given a
non-empty argument. Or am I missing something?
> +then
> + echo >&2 "mymake: $makefile contains nested variables"
> + exit 1
> +fi
> +exec $mymake_MAKE "$@"
>
What if we had "-f -" among the arguments? The stdin meant for make will result
to be already consumed by the previous `cat`...
> +EOF
> +chmod a+x mymake
> +mymake_MAKE=${MAKE-make}
> +export mymake_MAKE
> +
I'd add a sanity check here to verify that the `mymake' script really
rejects Makefiles using nested variables:
cat > Makefile <<'END'
a = $(b$(c))
all:
touch bar
END
./mymake && Exit 99
mv -f Makefile foo.mk
./mymake -f foo.mkr && Exit 99
cat foo.mk | ./mymake -f - && Exit 99
test -f bar && Exit 99
sed '/a =/d' foo.mk > Makefile
./mymake && test -f bar || Exit 99
rm -f foo.mk bar Makefile
Ugly, I know, but similar internal checks have saved me from testsuite bugs and
false negatives a few times already.
> +$ACLOCAL
> +$AUTOMAKE --add-missing
> +$AUTOCONF
> +
> +./configure --enable-silent-rules MAKE=./mymake
>
You should also grep configure output for:
checking whether ./mymake supports nested variables... no
and maybe the generated Makefile for the definition:
AM_V = 1
> +./mymake >stdout || { cat stdout; Exit 1; }
> +cat stdout
> +$EGREP ' (-c|-o)' stdout && Exit 1
> +grep 'mv ' stdout && Exit 1
> +grep 'CC .*foo\.' stdout
> +grep 'CC .*bar\.' stdout
> +grep 'CC .*baz\.' stdout
> +grep 'CC .*bla\.' stdout
> +grep 'CCLD .*foo' stdout
> +grep 'CCLD .*bar' stdout
> +grep 'CCLD .*baz' stdout
> +grep 'CCLD .*bla' stdout
> +
> +./mymake clean
> +./configure --disable-silent-rules MAKE=./mymake
>
You should also grep configure output for:
checking whether ./mymake supports nested variables... no
and maybe the generated Makefile for the definition:
AM_V = 0
> +./mymake >stdout || { cat stdout; Exit 1; }
> +cat stdout
> +grep ' -c' stdout
> +grep ' -o foo' stdout
> +$EGREP '(CC|LD) ' stdout && Exit 1
> +
> +:
Thanks,
Stefano
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.