Re: [Automake-NG] [PATCH 2/2] warns: fix checks against typos in vars (_LDADD and _SOURCES, for example)

[Dave Hart]

On Wed, Jun 27, 2012 at 16:05 UTC, Stefano Lattarini wrote:
> After commit v1.12.1-302-g67d6102 of 2012-06-05, "[ng] warns: typos in
> _SOURCES etc. reported at make runtime", the checks against typos in use
> of few special variables (like 'bar_SOURCES' or 'libfoo_a_LIBADD') are
> done at make runtime rather than at automake runtime.
>
> Such checks have so far been running unconditionally every time make was
> invoked, thus creating a noticeable performance degradation in null or
> almost-null builds of packages that, like GNU coreutils, have a lot of
> programs and/or use several recursive make invocations.
>
> What is worse, such checks have been unable to interact well with the
> Makefile automatic remake rules.  Say that a user encounters a failure
> in those checks; he then fixes his 'Makefile.am' accordingly, and re-run
> "make", expecting the situation to be sorted out by the automatic remake
> rules (as was the case for mainline Automake).  But that doesn't happen,
> because those same checks will run again *before* giving the remake rules
> a chance to kick in -- thus "make" would fail again, exactly as before,
> because the Makefile hasn't been rebuilt, and thus hasn't changed.
> So the user would forced to re-run automake and config.status *by hand* to
> get back a consistent, correct status in its build system!  Such situation
> is utterly unacceptable.
>
> The present change takes care at once of both of the performance and the

s/at once//

"Takes care of" is an idiom that doesn't read well when interspersed
with other words.  Since "at once" isn't really adding any information
here, getting rid of it is the easiest reformulation.

> correctness problems.
>
> On the performance side, after this change, the time for a null build
> on GNU coreutils (bootstrapped with Automake-NG) has gone down from ~8
> seconds to ~2 seconds.  Not bad!
>
> * automake.in (generate_makefile): Pass the '%MAKEFILE%' transform
> when processing 'check-typos.am'.
> * lib/am/check-typos.am: Take advantage of the GNU make automatic restart
> capabilities to re-implement the  existing check in a more "lazy" way:
> they should be re-run only when Makefile has changes, and only after the

I think s/changes/changed/ is clearer.

> automatic remake rules has rebuilt it (if it needs to be rebuilt).

s/has/have/
At your discretion, s/(if it needs to be rebuilt)/(if needed)/ is more
concise without ambiguity.

> * lib/am/header-vars.am (.am/nil): New do-nothing target, for now used
> only by 'check-typos.am'.
> * t/am-dir.sh: Adjust.
> * t/maken.sh: Likewise.
> * t/output6.sh: Likewise.
> * t/spell.sh: Likewise.
> * t/spell2.sh: Likewise.
> * t/vartypos-whitelist.sh: Likewise.
> * t/vartypos.sh: Likewise.
>
> Signed-off-by: Stefano Lattarini <address@hidden>
> ---
>  automake.in             |    3 ++-
>  lib/am/check-typos.am   |   55 
> ++++++++++++++++++++++++++++++++++++++++++-----
>  lib/am/header-vars.am   |    6 ++++++
>  t/am-dir.sh             |    3 ++-
>  t/maken.sh              |    6 +++---
>  t/output6.sh            |    8 +++++--
>  t/spell.sh              |    4 ++--
>  t/spell2.sh             |    4 ++--
>  t/vartypos-whitelist.sh |   20 ++++++++++++-----
>  t/vartypos.sh           |   13 ++++++++---
>  10 files changed, 98 insertions(+), 24 deletions(-)
>
> diff --git a/automake.in b/automake.in
> index 074abf6..a734afc 100644
> --- a/automake.in
> +++ b/automake.in
> @@ -6945,7 +6945,8 @@ sub generate_makefile ($$)
>
>   my $output_checks = '';
>   # See if any _SOURCES (or _LIBADD, or ...) variable were misspelled.
> -  $output_checks .= preprocess_file ("$libdir/am/check-typos.am");
> +  $output_checks .= preprocess_file ("$libdir/am/check-typos.am",
> +                                     MAKEFILE => basename ($makefile));
>   # Report errors (if any) seen at make runtime.
>   $output_checks .= '$(if $(am__seen_error),' .
>                     '$(error Some Automake-NG error occurred))' .
> diff --git a/lib/am/check-typos.am b/lib/am/check-typos.am
> index 71769c9..6e4b804 100644
> --- a/lib/am/check-typos.am
> +++ b/lib/am/check-typos.am
> @@ -18,13 +18,29 @@
>  ##    bin_PROGRAMS = bar
>  ##    baz_SOURCES = main.c  # Should be bar_SOURCES.
>
> -## FIXME: With this, we impose runtime penalty to all make runs.  Maybe we
> -## FIXME: should make all these checks conditional to whether the user sets
> -## FIXME: a AM_SANITY_CHECKS variable or something?
> -
>  ## FIXME: We should document the '.am/' namespace as reserved for automake
>  ## FIXME: internals somewhere.
>
> +## FIXME: we should document the user-settable AM_FORCE_SANITY_CHECKS
> +## FIXME: variable, and its semantics.
> +
> +# Running these checks unconditionally can be quite wasteful, especially
> +# for projects with lots of programs.  So run them only when the values
> +# of the checked variables has possibly changed.  For the moment, we assume

s/has possibly changed/may have changed/
or "might have changed" or "could have changed" if you prefer.

> +# this can only happen if the Makefile has been updated since the later

s/later/last/
or "most recent" if you prefer.

> +# check.  And to give user more control, we also allow him to require these
> +# checks run unconditionally, by setting AM_FORCE_SANITY_CHECKS to "yes".
> +
> +# The idiom we employ to implement our "lazy checking" relies on recursive
> +# make invocations and creation of an auxiliary makefile fragments, and
> +# such an approach do not interact very well with "make -n"; in such a case,

s/do not/does not/
s/in such a case/in which case/ is more concrete -- we're not dealing
with a class of -n -like options, just one.

> +# it's simpler and safer to go for "greedy checking".
> +ifeq ($(am__make_dryrun),true)
> +AM_FORCE_SANITY_CHECKS ?= yes
> +endif
> +
> +ifeq ($(AM_FORCE_SANITY_CHECKS),yes)
> +
>  # Variables with these suffixes are candidates for typo checking.
>  .am/vartypos/suffixes := _SOURCES _LIBADD _LDADD _LDFLAGS _DEPENDENCIES
>
> @@ -49,7 +65,7 @@ ifeq "$(am__using_parallel_tests)" "yes"
>  endif
>
>  # Allow the user to add his own whitelist.
> -# FIXME: this is still undocumtend!
> +# FIXME: this is still undocumented!
>  .am/vartypos/whitelisted-vars += $(AM_VARTYPOS_WHITELIST)
>
>  # Canonicalized names of programs and libraries (vanilla or libtool) that
> @@ -86,3 +102,32 @@ endef
>  # separator" error from GNU make.
>  $(eval $(foreach v,$(.am/vartypos/candidate-vars), \
>                    $(call .am/vartypos/check,$v)))
> +
> +else # $(AM_FORCE_SANITY_CHECKS) != yes
> +
> +# We use "-include" rather than "include" to avoid getting, on the first
> +# make run in a clean tree, the following annoying warning:
> +#    Makefile: .am/check-typos-stamp.mk: No such file or directory
> +# Although such a warning would *not* be an error in our setup, it still
> +# is ugly and annoying enough to justify ...
> +-include $(am__dir)/check-typos-stamp.mk
> +
> +# ... this workaround; which is required by the fact that, if a recipe
> +# meant to rebuild a file included with "-include" file fails, the make
> +# run itself is not considered failed (this is quite consistent with
> +# the "-include" semantics).
> +ifdef .am/sanity-checks-failed
> +$(shell rm -f $(am__dir)/check-typos-stamp.mk)
> +$(error Some Automake-NG sanity checks failed)
> +else
> +$(am__dir)/check-typos-stamp.mk: %MAKEFILE% | $(am__dir)
> +       @if \
> +         $(MAKE) --no-print-directory AM_FORCE_SANITY_CHECKS=yes .am/nil; \
> +       then \
> +         echo "# stamp" > $@; \
> +       else \
> +         echo ".am/sanity-checks-failed = yes" > $@; \
> +       fi
> +endif
> +
> +endif # $(AM_FORCE_SANITY_CHECKS) != yes
> diff --git a/lib/am/header-vars.am b/lib/am/header-vars.am
> index 75e5a3a..d2486b6 100644
> --- a/lib/am/header-vars.am
> +++ b/lib/am/header-vars.am
> @@ -23,6 +23,12 @@ VPATH = @srcdir@
>  ## this can greatly speed up null or almost-null builds, up to even 50%!
>  MAKEFLAGS += --no-builtin-rules
>
> +# For when we need a do-nothing target.  Add an actual rule to it to avoid
> +# the annoying message "make[1]: Nothing to be done for .am/nil" from make.
> +.PHONY: .am/nil
> +.am/nil:
> +       @:
> +
>  ## Declare an error, without immediately terminating the execution (proper
>  ## code will take care later of that).  This will allow us to diagnose more
>  ## issues at once, rather than stopping at the first one.
> diff --git a/t/am-dir.sh b/t/am-dir.sh
> index f302659..92c474b 100755
> --- a/t/am-dir.sh
> +++ b/t/am-dir.sh
> @@ -70,7 +70,8 @@ do_check ()
>   srcdir=$1
>   $srcdir/configure
>   $MAKE
> -  find $d xsrc/$d | sort > got
> +  # The grep is to ignore files used internally by Automake-NG.
> +  find $d xsrc/$d | grep -v '\.mk$' | sort > got
>   cat $srcdir/exp
>   cat got
>   diff $srcdir/exp got
> diff --git a/t/maken.sh b/t/maken.sh
> index ffd5ef0..2ec872a 100755
> --- a/t/maken.sh
> +++ b/t/maken.sh
> @@ -47,14 +47,14 @@ $AUTOCONF
>  $AUTOMAKE
>  ./configure
>
> -echo stamp > stampfile
> -$sleep
>  for target in dist distcheck; do
> +  echo stamp > stampfile
> +  $sleep
>   $MAKE -n $target
>   $MAKE -n $target | grep stamp-sub-dist-hook
> -  $MAKE test-no-distdir
>   # No file has been actually touched or created.
>   is_newest stampfile $(find .)
> +  $MAKE test-no-distdir
>  done
>
>  :
> diff --git a/t/output6.sh b/t/output6.sh
> index 80ae4ff..0b94853 100755
> --- a/t/output6.sh
> +++ b/t/output6.sh
> @@ -51,12 +51,16 @@ EOF
>
>  echo 'd = D' > d.in
>
> +cat > GNUmakefile << 'END'
> +include foo
> +END
>
>  $ACLOCAL
>  $AUTOCONF
>  $AUTOMAKE
> +
>  ./configure
> -$MAKE -f foo test1
> +$MAKE test1
>
>  $sleep
>  cat >b.in <<'EOF'
> @@ -66,6 +70,6 @@ c = F
>  d = F
>  EOF
>
> -$MAKE -f foo test2
> +$MAKE test2
>
>  :
> diff --git a/t/spell.sh b/t/spell.sh
> index 790739d..4d14a55 100755
> --- a/t/spell.sh
> +++ b/t/spell.sh
> @@ -41,12 +41,12 @@ $AUTOMAKE
>  $MAKE 2>stderr && { cat stderr >&2; Exit 1; }
>  cat stderr >&2
>
> -LC_ALL=C sed 's/^Makefile:[0-9][0-9]*: //' stderr > got
> +LC_ALL=C sed -e 's/^Makefile:[0-9][0-9]*: //' \
> +             -e '/^\*\*\*.*Automake-NG/d' stderr > got
>
>  cat > exp << 'END'
>  variable 'boo_SOURCES' is defined but no program
>   or library has 'boo' as canonical name
> -*** Some Automake-NG error occurred.  Stop.
>  END
>
>  diff exp got
> diff --git a/t/spell2.sh b/t/spell2.sh
> index dead9c1..39377cb 100755
> --- a/t/spell2.sh
> +++ b/t/spell2.sh
> @@ -41,12 +41,12 @@ $AUTOMAKE
>  $MAKE 2>stderr && { cat stderr >&2; Exit 1; }
>  cat stderr >&2
>
> -LC_ALL=C sed 's/^Makefile:[0-9][0-9]*: //' stderr > got
> +LC_ALL=C sed -e 's/^Makefile:[0-9][0-9]*: //' \
> +             -e '/^\*\*\*.*Automake-NG/d' stderr > got
>
>  cat > exp << 'END'
>  variable 'qardoz_LDADD' is defined but no program
>   or library has 'qardoz' as canonical name
> -*** Some Automake-NG error occurred.  Stop.
>  END
>
>  diff exp got
> diff --git a/t/vartypos-whitelist.sh b/t/vartypos-whitelist.sh
> index 7c718b2..e418c66 100755
> --- a/t/vartypos-whitelist.sh
> +++ b/t/vartypos-whitelist.sh
> @@ -21,6 +21,12 @@
>  required=cc
>  . ./defs || Exit 1
>
> +edit ()
> +{
> +  file=$1; shift
> +  "$@" <$file >t && mv -f t $file || fatal_ "editing $file"
> +}
> +
>  cat >> configure.ac << 'END'
>  AC_PROG_CC
>  AM_PROG_AR
> @@ -93,17 +99,21 @@ $AUTOMAKE -a
>  ./configure
>  $MAKE
>
> +# Sanity check the distribution.
> +$MAKE distcheck
> +
>  # If we remove the whitelisting, failure ensues.
> -$MAKE AM_VARTYPOS_WHITELIST= 2>stderr && { cat stderr; Exit 1; }
> +sed '/^AM_VARTYPOS_WHITELIST *=/d' <Makefile.am >t && mv -f t Makefile.am \
> +  || fatal_ "editing Makefile.am"
> +$MAKE 2>stderr && { cat stderr; Exit 1; }
>  cat stderr >&2
>  grep "'copy_LDADD' is defined but no program" stderr
>  grep "'remove_LDADD' is defined but no program" stderr
> -$MAKE AM_VARTYPOS_WHITELIST=remove_LDADD 2>stderr && { cat stderr; Exit 1; }
> +
> +$MAKE AM_VARTYPOS_WHITELIST=remove_LDADD AM_FORCE_SANITY_CHECK=yes \
> +  2>stderr && { cat stderr; Exit 1; }
>  cat stderr >&2
>  grep "'copy_LDADD' is defined but no program" stderr
>  grep "remove_LDADD" stderr && Exit 1
>
> -# Sanity check the distribution.
> -$MAKE distcheck
> -
>  :
> diff --git a/t/vartypos.sh b/t/vartypos.sh
> index cf9b31e..ce0e236 100755
> --- a/t/vartypos.sh
> +++ b/t/vartypos.sh
> @@ -101,9 +101,16 @@ lib_LIBRARIES = libfoo.a
>  lib_LTLIBRARIES = libbar.la
>  END
>
> -# FIXME!  We have to remake the Makefile by hand!  This is unacceptable.
> -$AUTOMAKE Makefile
> -./config.status Makefile
>  $MAKE nihil
>
> +# If further errors are introduced, they are reported.
> +$sleep
> +echo none_SOURCES = >> Makefile.am
> +
> +$MAKE nihil 2>stderr && { cat stderr >&2; Exit 1; }
> +cat stderr >&2
> +
> +grep "variable 'none_SOURCES'" stderr
> +grep "'none' as canonical name" stderr
> +
>  :
> --
> 1.7.9.5
>
>

Cheers,
Dave Hart