bug#9928: bug#10237: AM_SILENT_RULES does not work with NonStop make

[Stefano Lattarini]

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 <address@hidden>
> 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 = $(address@hidden@)".  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  <address@hidden>
> +
>
[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
> address@hidden option if your package needs to build with
> address@hidden implementations that do not support it.  The
> address@hidden 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}
> address@hidden @code{AM_V}
> address@hidden @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 = $(address@hidden@)
  pkg_verbose_ = $(address@hidden@)

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