Re: even `if FALSE' *_SOURCES should be used for TAGS

[Ralf Wildenhues]

Hi Stefano,

* Stefano Lattarini wrote on Wed, Oct 13, 2010 at 03:54:31PM CEST:
> On Tuesday 12 October 2010, Ralf Wildenhues wrote:
> > * Stefano Lattarini wrote on Tue, Oct 12, 2010 at 05:51:58PM CEST:
> > > > OTOH AM_CONDITIONAL can be used for example if an optional sources part 
> > > > is
> > > > / is not bundled, in such case the current automake behavior is better.
> > 
> > > Ralf, do you agree we should implement the behaviour requested by Jan?
> > 
> > Well, I see the following caveat:
> > 
> > 'make tags' shouldn't cause files to be built (or accessed) that 'make
> > dist' wouldn't build; that probably means conditional nodist_ files
> > shouldn't be tagged.

> I disagree; non-distributed but unconditonally-defined sources should be
> tagged IMHO (even if they are Makefile-generated), as should distributed
> but conditionally-defined sources.

Your statement doesn't conflict with my statement, as far as I can see.
Is there a typo somewhere, or are we misreading each other?
Maybe it helps to clarify my statement by reformulating as follows:

  'make tags' shouldn't cause files to be built (or accessed) that 'make
  dist' wouldn't build (apart from files created by 'make all' already);
  that probably means conditional nodist_ files shouldn't be tagged.

The thing that needs to be avoided is that `make tags' tries to run
the generator in the setting below if the generator is not present:

if HAVE_GENERATOR
nodist_foo_SOURCES += file.c
file.c:
        generator > $@
endif


However, the other question to consider is that with generators, users
might not want the generated files to be tagged, rather than the source:
Good examples are yacc or lex files, where the .y and .l files should be
tagged but the generated .c files shouldn't.  I'm not sure how to solve
that easily except by some new structure information in the Language
struct.

> This is also more consistent with the
> current automake documentation:
>   ``If any C, C++ or Fortran 77 source code or headers are present, then
>     tags and TAGS rules will be generated for the directory.  All files
>     listed using the _SOURCES, _HEADERS, and _LISP primaries will be used
>     to generate tags.  Note that generated source files that are not
>     distributed must be declared in variables like nodist_noinst_HEADERS
>     or nodist_prog_SOURCES or they will be ignored.''

I'm hoping this is also moot if above is cleared up.  :-)

> Only non-distributed conditionally-defined sources should not be tagged
> by default (but should be when listed in some EXTRA_*_SOURCES, or when
> the condition they are defined into happens to be true).

I agree to this with:  s/when listed in some EXTRA_*_SOURCES, or //
Otherwise again the above caveat applies.

FYI, the EXTRA_foo_SOURCES listing is necessary so automake can add a
dependency file for these; it doesn't state whether the source can be
generated or not.

> > OTOH, automake.in:handle_source_transform is the only place where
> > @sources and @dist_sources get possibly different values, and I don't
> > understand why that was done; IOW, why the implementation didn't just
> > have $(SOURCES) = $(DIST_SOURCES)  (except for the possibility of the
> > no-dist option, of course).

> Maybe to take into account the possibility of having non-distributed,
> Makefile-generated and unconditonally-defined sources -- for which tags
> should be enabled, even if they are not distributed.  This is just a
> guess of mine, though, as I've done no history digging.

Good point.

> As an aside, I see that the testsuite coverage on tags functionality
> is *extremely* lacking and inadequate, and I'd really like to extend
> it; so I seize this opportunity to repropose my idea of a new
> "tests-extend" topic branch dedicated to testsuite enhancement
> and extension.

I haven't dismissed the idea of such a topic branch.  I do think that
testsuite additions for 'tags' functionality would better fit on a
branch that deals with 'tags' functionality: topics related to
features seem more fitting than topics related to source code subdirs.

> Subject: [PATCH] New tests; add TODO comment to automake.in

> --- a/automake.in
> +++ b/automake.in
> @@ -2208,6 +2208,10 @@ sub handle_source_transform ($$$$%)
>       $used_pfx{$xpfx} = 1
>         unless $prefix =~ /EXTRA_/;
>  
> +     # FIXME: $varname should be added to @sources iff:
> +        #   1. it is unconditionally defined, or
> +        #   2. it is in EXTRA_DIST, or
> +        #   3. it is distributed

(2) is redundant with (3), no?

Ah, interesting tangent here: Ben reported a while ago that conditional
EXTRA_DIST distributes conditionally.  I had very much grown accustomed
to this behavior, but it is at odds with the way conditionals work
otherwise.  OTOH, user setups will break if we change the current
functionality.

Anyway, rather than committing the FIXME patch let's fix it.  ;-)

>       push @sources, "\$($varname)";
>       push @dist_sources, shadow_unconditionally ($varname, $where)
>         unless (option ('no-dist') || $prefix =~ /^nodist_/);

> --- /dev/null
> +++ b/tests/tagscond.test
> @@ -0,0 +1,117 @@

> +# Check that tags for conditionally defined (but distributed)
> +# sources work.

This test looks sensible to me (with nits below).

> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >> configure.in <<'END'
> +AC_PROG_CC
> +AM_PATH_LISPDIR
> +AM_CONDITIONAL([COND], [test x"$cond" = x"yes"])
> +AC_OUTPUT
> +END
> +
> +cat > Makefile.am <<'END'
> +bin_PROGRAMS = foo bar
> +foo_SOURCES = main.c
> +if COND
> +  foo_SOURCES += foo1.c
> +  bar_SOURCES = main.c bar.h
> +  include_HEADERS = quux.h
> +  lisp_LISP = joe.el
> +else
> +  foo_SOURCES += foo2.c
> +  bar_SOURCES = main.c baz.h
> +  include_HEADERS = zardoz.h
> +  lisp_LISP = moe.el

I always cringe if the text in conditionals is indented: makefile
parsing is fragile, automake isn't perfect, and it misreads
space-indented rules.  For that reason I tend to avoid it fully
(we might want to fix it but that's a different issue entirely and
also one that breaks real-world setups and should be kept orthogonal to
this).

(automake also misparses non-TAB-indented continuations of rule recipes;
this is another limitation over Posix, but I think it might be shared by
some older makes also.)

> +endif
> +
> +quux.h zardoz.h:
> +     : > $@

Unfortunately, automake also doesn't correctly analyze

target1 target2:

which is something we should probably fix but which some users might
also have come to rely on.

> +CLEANFILES = quux.h zardoz.h
> +
> +exp.list:
> +     (case $(srcdir) in .) d=;; *) d=$(srcdir)/;; esac \
> +       && echo $${d}bar.h \
> +       && echo $${d}baz.h \
> +          && echo $${d}foo1.c \
> +          && echo $${d}foo2.c \
> +          && echo $${d}main.c \
> +       && echo $${d}quux.h \
> +       && echo $${d}zardoz.h \
> +       && echo $${d}joe.el \
> +       && echo $${d}moe.el \
> +        ) > $@

Let's use { ...; } > $@ for the outer redirection so the unbalanced
parentheses don't hurt the eye (and editor syntax highlighting) so much.

> +MOSTLYCLEANFILES = exp.list
> +
> +.PHONY: test
> +test: exp.list ID TAGS CTAGS
> +     diff exp.list TAGS
> +     diff exp.list tags
> +     diff exp.list ID
> +
> +check-local: test
> +END
> +
> +echo 'int main(void) { return 0; }' > main.c
> +echo 'extern int i;' > foo1.c
> +echo 'extern int j;' > foo2.c
> +: > bar.h
> +: > baz.h
> +
> +: > elisp-comp
> +
> +mkdir bin
> +cat > bin/etags <<'END'
> +#!/bin/sh
> +for i in ${1+"$@"}; do echo "$i"; done | sort > TAGS

Using the ${1+"$@"} idiom requires sanitization for zsh.  OTOH, if you
can rely on the set of positional parameters to be nonempty you can just
use "$@" which requires no sanitization.  With a for loop, things are
easier even: you can just use
  for i
  do
    ...
  done

(the newline before 'do' is intentional; see autoconf.info).
Also below in ctags.

You need to sanitize the locale to C when sorting, if only to stabilize
sorting in case-ignoring collation.  Several instances.

> +END
> +cat > bin/ctags <<'END'
> +#!/bin/sh
> +for i in ${1+"$@"}; do echo "$i"; done | sort > tags
> +END
> +cat > bin/mkid <<'END'
> +#!/bin/sh
> +case $1 in
> +  -f*) out=`expr "x$1" : 'x-f\(.*\)'`; shift;;
> +    *) out=ID;;
> +esac
> +for i in ${1+"$@"}; do echo "$i"; done | sort > "$out"
> +END
> +chmod a+x bin/etags bin/ctags bin/mkid
> +PATH=`pwd`/bin$PATH_SEPARATOR$PATH
> +
> +$ACLOCAL
> +$AUTOCONF
> +$AUTOMAKE
> +
> +for cond in yes no; do
> +  case $cond in
> +    yes) anticond=no;;
> +     no) anticond=yes;;
> +  esac
> +  ./configure cond="$cond" EMACS=no
> +  $MAKE test
> +  $MAKE distcheck DISTCHECK_CONFIGURE_FLAGS="cond='$cond' EMACS=no"
> +  $MAKE distcheck DISTCHECK_CONFIGURE_FLAGS="cond='$anticond' EMACS=no"
> +  $MAKE distclean
> +done
> +
> +:

> --- /dev/null
> +++ b/tests/tagsextra.test
> @@ -0,0 +1,70 @@

> +# Check interaction of EXTRA_SOURCES with tags.

Please reevaluate this test given the reasoning above.
Also, can we name the thingy EXTRA_*_SOURCES so it is clear that it is
not literal (several instances)?  Thanks.

> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >> configure.in <<'END'
> +AM_CONDITIONAL([COND], [false])
> +AC_PROG_CC
> +AC_OUTPUT
> +END
> +
> +cat > Makefile.am <<'END'
> +bin_PROGRAMS = foo
> +foo_SOURCES = main.c
> +if COND
> +nodist_foo_SOURCES = bad.c
> +endif
> +EXTRA_foo_SOURCES = bad.c
> +bad.c:
> +## this file should never be compiled, so make its content invalid C
> +     echo '/* %{~GREP~ME~}% */ choke me' > $@
> +END
> +
> +echo 'int main(void) { return 0; }' > main.c
> +
> +mkdir bin
> +for p in etags ctags mkid; do
> +  (echo '#!/bin/sh' && echo 'exit 0') > bin/$p
> +  chmod a+x bin/$p
> +done
> +PATH=`pwd`/bin$PATH_SEPARATOR$PATH
> +
> +$ACLOCAL
> +$AUTOCONF
> +$AUTOMAKE
> +
> +./configure
> +
> +for target in ID ctags CTAGS tags TAGS; do
> +  rm -f bad.c
> +  $MAKE $target
> +  $FGREP '%{~GREP~ME~}%' bad.c
> +done
> +
> +$MAKE
> +./foo
> +
> +$MAKE distdir
> +test ! -r $me-1.0/bar.c
> +
> +$MAKE distcheck
> +
> +:

> --- /dev/null
> +++ b/tests/tagsnodist.test

> +# Check that "make TAGS" and similar do not try to create
> +# non-distributed files used only in "false" conditionals.

This test looks ok to me, with nits below.

> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >> configure.in <<'END'
> +AM_CONDITIONAL([COND_OK], [:])
> +AM_CONDITIONAL([COND_KO], [false])
> +AC_PROG_CC
> +AC_OUTPUT
> +END
> +
> +cat > Makefile.am <<'END'
> +bin_PROGRAMS = foo
> +foo_SOURCES = main.c
> +
> +if COND_OK
> +## conditionally defined, but distributed: it will be considered
> +## for tags, even if it's not listed in EXTRA_SOURCES.
> +foo_SOURCES += bar.c
> +## non-distributed and conditionally defined: but it will be considered
> +## for tags, since the conditional COND_OK heppens to be true.

happens

> +nodist_foo_SOURCES = baz.c
> +else
> +## non-distributed and conditionally defined: it won't be considered
> +## for tags, since it's not listed in EXTRA_SOURCES and it's in the
> +## `else' branch of the conditional COND_OK, which happens to be true.

Please reevaluate this comment and the next one as per above.

> +nodist_foo_SOURCES = quux.c
> +endif
> +
> +if COND_KO
> +## non-distributed and conditionally defined: won't be considered
> +## for tags, since it's not listed in EXTRA_SOURCES and the conditional
> +## COND_KO happensto be false.

happens to

> +nodist_foo_SOURCES += zardoz.c
> +endif
> +
> +bar.c:
> +     echo 'int bar(void) { return 0; }' > $@
> +baz.c:
> +     echo 'int baz(void) { return 1; }' > $@
> +CLEANFILES = bar.c baz.c
> +
> +quux.c zardoz.c:

See above.

> +     @echo 'ERROR: $@ should never be crated' >&2; exit 1
> +END
> +
> +cat > main.c <<'END'
> +int main(void)
> +{
> +  int bar(void);
> +  int baz(void);
> +  return bar() * baz();
> +}
> +END
> +
> +mkdir bin
> +for p in etags ctags mkid; do
> +  (echo '#!/bin/sh' && echo 'exit 0') > bin/$p
> +  chmod a+x bin/$p
> +done
> +PATH=`pwd`/bin$PATH_SEPARATOR$PATH
> +
> +$ACLOCAL
> +$AUTOCONF
> +$AUTOMAKE
> +
> +./configure
> +
> +for target in ID ctags CTAGS tags TAGS; do
> +  $MAKE $target
> +  test -f bar.c
> +  test -f baz.c
> +  test ! -r quux.c
> +  test ! -r zardoz.c
> +  rm -f bar.c baz.c
> +done
> +
> +$MAKE
> +./foo

I prefer to avoid executing compiled programs if executability is not
the primary purpose of the test.  Rationale is that it breaks cross
compile setups (we don't really support those setups in the testsuite
yet but I have pending patches to fix that), and it breaks for example
when you have wine installed on GNU/Linux and the binfmt_misc module in
place, and need to specify the .exe extension so the file is found.

Most actual issues can be solved by causing link failures in the
unwanted cases.  If not, then it's still easier to use TESTS and 'make
check', or run ./foo$(EXEEXT) from within the Makefile.

Another instance below.

> +$MAKE distcheck
> +$MAKE clean
> +rm -f ID *tags* *TAGS* # just to be sure
> +
> +# Try again, with no-dist option.
> +
> +echo 'AUTOMAKE_OPTIONS = no-dist' >> Makefile.am
> +$AUTOMAKE Makefile
> +
> +./config.status Makefile
> +
> +test ! -r bar.c # sanity check
> +test ! -r baz.c # likewise
> +for target in ID ctags CTAGS tags TAGS; do
> +  $MAKE $target
> +  test -f bar.c
> +  test -f baz.c
> +  test ! -r quux.c
> +  test ! -r zardoz.c
> +  rm -f bar.c baz.c
> +done
> +
> +$MAKE
> +./foo
> +
> +$MAKE distdir && Exit 1 # sanity check
> +
> +:

Thanks,
Ralf