bug#10237: AM_SILENT_RULES does not work with NonStop make

[Stefano Lattarini]

Hi Paul.

On Saturday 10 December 2011, Paul Eggert wrote:
> On 12/06/11 11:02, Stefano Lattarini wrote:
> > If you are interested in accomodating this fringe situation, I will
> > then accept a patch on the lines Paul has proposed (with a mandatory
> > testcase, otherwise it would be far too easy to regress in such a
> > almost-never-excercised corner case).
> 
> OK, a proposed patch is below.  It changes the silent-rules test case
> to check the new behavior; hope that's what you're asking for.
>
No, I was asking for a test case that simulates the presence of a
make implementation unable to grasp nested variable expansions, that
checks that the new code correctly kicks in such a situation (this
can be done by grepping configure output and generated Makefile),
and that the build process truly works as expected both with
`--enable-silent-rules' and with `--disable-silent-rules' (see test
`silent.test' for inspiration about how to do so).  Otherwise the
new code would remain basically uncovered in our testsuite for the
very fringe and thorny situation we are trying to fix, which IMO is
a big no-no.

> Comments are welcome.
>
See them inlined below.

> The patch below is just the human-edited parts.  A full patch
> (including the autogenerated parts) is attached, as a compressed file.
>

First nit: since this patch is a basically a bugfix (albeit for a very
corner-case bug), I think it should be applied to the maintenance
line of automake.  So, could you please rebase your patch on maint?
Thanks.

> automake: port silent-rules option to POSIX make
>
s/POSIX make/fringe make implementations/, maybe?

> 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).
>
A brief description of the problem here would be nice.  Would you mind
condensing one from the mailing list discussions?

> 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.
>
Here I'd add something like:

  With this change, make implementations that doesn't grasp nested
  variable expansions will still be able to run Makefile generated
  using the `silent-rules' option; of course, they won't allow the
  user to override the make verbosity at runtime through redefintion
  of $(V) (as in "make V=0"); but this is still an improvement over
  not being able to work at all.

> Similarly for usages like $(am__v_CC_$(AM_DEFAULT_VERBOSITY)).
> * NEWS: Document this.
> * automake.in (define_verbose_var): When defining the variable,
> use @am__V@ rather than $(V),

> and likewise for @am__DEFAULT_VERBOSITY@ and $(AM_DEFAULT_VERBOSITY).
>
Maybe substitute this with a simpler "and @am__DEFAULT_VERBOSITY@
rather than $(AM_DEFAULT_VERBOSITY)" ?

> (handle_options): silent-rules no longer overrides
> portability-recursive.
>
Bad idea, unless you also change the documentation in the manual
about how to write custom "silenceable rules":

  You can add your own variables, so strings of your own choice are shown.
  The following snippet shows how you would define your own equivalent of
  AM_V_GEN: 
               pkg_verbose = $(pkg_verbose_$(V))
               pkg_verbose_ = $(pkg_verbose_$(AM_DEFAULT_VERBOSITY))
               pkg_verbose_0 = @echo GEN $@;
               
               foo: foo.in
                       $(pkg_verbose)cp $(srcdir)/foo.in $@

> * doc/automake.texi (Invoking Automake): silent-rules no longer
> overrides portability-recursive.
> (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__DEFAULT_VERBOSITY
> accordingly.
> * tests/silent-nowarn.test: Check that silent-rules no longer
> overrides portability-recursive.
> diff --git a/NEWS b/NEWS
> index da9af08..615f420 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,3 +1,14 @@
> +* Changes to automake:
> +
> +  - 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'.  Since the `silent-rules' option no longer
> +    requires nested variables, it no longer disables the
> +    nested-variables warning.
> +
>
This NEWS entry should go in the "Bugs fixed in 1.11a" section,
"Bugs introduced by 1.11" subsection.

> diff --git a/automake.in b/automake.in
> index 0b6d014..d61af86 100644
> --- a/automake.in
> +++ b/automake.in
> @@ -1141,9 +1141,8 @@ sub define_verbose_var ($$)
>      my $silent_var = $pvar . '_0';
>      if (option 'silent-rules')
>        {
> -     # Using `$V' instead of `$(V)' breaks IRIX make.
>
Here, I'd add a pointer to comments in m4/silent.m4, since this code
has become decidedly non-obvious enough to deserve an examplnation.

> -     define_variable ($var, '$(' . $pvar . '_$(V))', INTERNAL);
> -     define_variable ($pvar . '_', '$(' . $pvar . 
> '_$(AM_DEFAULT_VERBOSITY))', INTERNAL);
> +     define_variable ($var, '$(' . $pvar . '_@'.'am__V'.'@)', INTERNAL);
> +     define_variable ($pvar . '_', '$(' . $pvar . 
> '_@'.'am__DEFAULT_VERBOSITY'.'@)', INTERNAL);
>       Automake::Variable::define ($silent_var, VAR_AUTOMAKE, '', TRUE, $val,
>                                   '', INTERNAL, VAR_ASIS)
>         if (! vardef ($silent_var, TRUE));
> @@ -1236,10 +1235,6 @@ sub handle_options
>        return 1 if process_option_list (@options);
>      }
> 
> -  # Override portability-recursive warning.
> -  switch_warning ('no-portability-recursive')
> -    if option 'silent-rules';
> -
>
Don't do this without also updating the documentation.  Thanks.

>    if ($strictness == GNITS)
>      {
>        set_option ('readme-alpha', INTERNAL);
> diff --git a/doc/automake.texi b/doc/automake.texi
> index e937715..8214787 100644
> --- a/doc/automake.texi
> +++ b/doc/automake.texi
> @@ -2702,8 +2702,6 @@ variables.
>  The categories output by default are @samp{syntax} and
>  @samp{unsupported}.  Additionally, @samp{gnu} and @samp{portability}
>  are enabled in @option{--gnu} and @option{--gnits} strictness.
> -On the other hand, the @option{silent-rules} options (@pxref{Options})
> -turns off portability warnings about recursive variable expansions.
>
>  @c Checked by extra-portability.test
>  Turning off @samp{portability} will also turn off @samp{extra-portability},
> @@ -10141,19 +10139,14 @@ Users who prefer to have silent rules enabled by 
> default can edit their
>  default to @samp{yes}.  This should still allow disabling silent rules
>  at @command{configure} time and at @command{make} time.
> 
> address@hidden FIXME: there's really a need to specify this explicitly?
> -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.
> -
Whay are you removing this advice?

> -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.
> 
>  @vindex @code{AM_V_GEN}
>  @vindex @code{AM_V_at}
> diff --git a/m4/silent.m4 b/m4/silent.m4
> index 432dd45..2a4446c 100644
> --- a/m4/silent.m4
> +++ b/m4/silent.m4
> @@ -5,7 +5,7 @@
>  # gives unlimited permission to copy and/or distribute it,
>  # with or without modifications, as long as this notice is preserved.
> 
> -# serial 2
> +# serial 3
> 
>  # AM_SILENT_RULES([DEFAULT])
>  # --------------------------
> @@ -25,6 +25,34 @@ case $enable_silent_rules in @%:@ (((
>     no) AM_DEFAULT_VERBOSITY=1;;
>      *) AM_DEFAULT_VERBOSITY=m4_if([$1], [yes], [0], [1]);;
>  esac
> +am_make=${MAKE-make}
> +AC_CACHE_CHECK([whether $am_make supports nested variables],
>
Here, I'd add a short comment for why this is needed, give a
pointer to the bug reports this code is trying to fix.  For
example:

  # Some fringe make implementations (e.g., NonStop make and
  # NextStep make) don't grasp nested variable expansions.
  # See automake bug#10237 and bug#9928.

> +   [am_cv_make_nested_variables],
>
Hmmm... maybe a name like `am_cv_make_support_nested_variables'
would be more appropriate?  Or would it be just longer?

> +   [am_makefile='
> +TRUE=$(BAR$(V))
> +BAR0=false
> +BAR1=true
> +V=1
> +am__doit:
> +     @$(TRUE)
> +.PHONY: am__doit
> +'
> +if echo "$am_makefile" | $am_make -s -f - >/dev/null 2>&1; then
>
`-s' make option shouldn't be needed here; also, I'd use AS_ECHO
instead of bare echo, even if that is probably not required (call
it extra safety, or cargo-cult programmming if you want :-)

> +  am_cv_make_nested_variables=yes
> +else
> +  am_cv_make_nested_variables=no
> +fi])
> +if test $am_cv_make_nested_variables = yes; then
> +  am__V='$(V)'
>
Please re-add here the comment you've removed from automake.in:

   # Using `$V' instead of `$(V)' would break IRIX make.
   am__V='$(V)'

> +  am__DEFAULT_VERBOSITY='$(AM_DEFAULT_VERBOSITY)'
> +else
> +  am__V=$AM_DEFAULT_VERBOSITY
> +  am__DEFAULT_VERBOSITY=$AM_DEFAULT_VERBOSITY
> +fi
> +AC_SUBST([am__V])dnl
> +AM_SUBST_NOTMAKE([am__V])dnl
> +AC_SUBST([am__DEFAULT_VERBOSITY])dnl
> +AM_SUBST_NOTMAKE([am__DEFAULT_VERBOSITY])dnl
>  AC_SUBST([AM_DEFAULT_VERBOSITY])dnl
>  AM_BACKSLASH='\'
>  AC_SUBST([AM_BACKSLASH])dnl

> diff --git a/tests/silent-nowarn.test b/tests/silent-nowarn.test
> index f0f5e70..9742bc3 100755
> --- a/tests/silent-nowarn.test
> +++ b/tests/silent-nowarn.test
> @@ -14,7 +14,7 @@
>  # 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 that the 'silent-rules' mode suppresses the warnings for recursive
> +# Check that the 'silent-rules' mode does not suppress warnings for recursive
>  # make variable expansions.  This should happen regardless of whether and
>  # where these warnings are requested.
> 
> @@ -39,6 +39,5 @@ END
>  touch AUTHORS ChangeLog COPYING INSTALL NEWS README THANKS
> 
>  $ACLOCAL
> -$AUTOMAKE --gnu -Wall -Wportability-recursive
> -
> -:
> +AUTOMAKE_fails --gnu -Wall -Wportability-recursive
> +grep 'recursive variable expansion' stderr
> 
Please keep the trailing `:' here:

  AUTOMAKE_fails --gnu -Wall -Wportability-recursive
  grep 'recursive variable expansion' stderr
  
  :

Thanks,
  Stefano