Re: non recursive includes proof of concept #2

[Alexandre Duret-Lutz]

>>> "Robert" == Robert Collins <address@hidden> writes:

[...]

 Robert> It transforms macros and paths in an included file (called
 Robert> Makefile.rules for now) , to make them suitable for a non-recursive
 Robert> build.

I'm skeptical about whether this approach can be made to work
intuitively.  More precisely I don't think it is generally
possible to let users write a subdir Makefile.am fragment as if
it would be run locally in the subdirectory, and translate
*anything* so it actually works from another directory.  Let's
mention user-defined rules referring rewritten variables, or
flag variables including things such as -I.

Anyway you asked for comments on the patch so here are some.
(I'm sorry I had to be brief in the end, because I have a train
to catch in one hour.  I'll be away for one week.)

I don't see what in your changes require the file to be named
Makefile.rules.

 Robert> As show by the test cases, this allows a couple of neat things:
 Robert> 1) A stub Makefile.am 
 Robert> ===
 Robert> include \$(srcdir)/Makefile.rules
 Robert> ===
 Robert> is all that is needed in a given subdirectory to generate a full
 Robert> makefile. (Useful if you want to be able to cd to a given dir and
 Robert> perform builds just in that dir).

This sounds neat.  But AFAICT no test case really cd into that
given dir and perform the build.

[...]

 Robert> +# Canonised variable suffixes
 Robert> +my @canonised_macro_names =
 Robert> +    qw(SOURCES);
 Robert> +# Canonised variable contents (foo->path/foo)
 Robert> +my @canonised_macro_values = 
 Robert> +    qw(SOURCES);
 Robert> +# Canonised macro lists (foo ->path_foo)
 Robert> +my @canonised_macro_lists = 
 Robert> +    qw(PROGRAMS);

Judging from the comments, I understand that 
bin_PROGRAMS = foo does not become path/foo?
Or is it a typo in the example of @canonised_macro_lists?

Since these variables will be used for member ship check, better
use a hash
  my %canonised_macro_names = (SOURCES => 1);
and latter
  if (exists $canonised_macro_names{$_}) ...
instead of those greps.

 Robert> # Match `-d' as a command-line argument in a string.
 Robert> my $DASH_D_PATTERN = "(^|\\s)-d(\\s|\$)";
 Robert> @@ -216,7 +232,7 @@ my @common_files =
 Robert> ansi2knr.1 ansi2knr.c compile config.guess config.rpath config.sub
 Robert> configure configure.ac configure.in depcomp elisp-comp
 Robert> install-sh libversion.in mdate-sh missing mkinstalldirs
 Robert> -      py-compile texinfo.tex ylwrap),
 Robert> +      py-compile texinfo.tex ylwrap Makefile.rules),

Is this required?  `include' automatically distributes it's
argument, I presume you've preserved this for `subdir_include'.

 Robert> @libtool_files, @libtool_sometimes);
 
 Robert> # Commonly used files we auto-include, but only sometimes.
 Robert> @@ -1697,6 +1713,38 @@ sub handle_single_transform_list ($$$$@)
 Robert> return @result;
 Robert> }
 
 Robert> +# $VALUE
 Robert> +# transform_file_list ($PREPEND, @FILES)
 Robert> +# ----------------------------------------
 Robert> +# insert $PREPEND before every file path that is not absolute
 Robert> +#
 Robert> +sub transform_file_list ($$)
 Robert> +{
 Robert> +      my ($prepend, $tmpfiles) = @_;
 Robert> +      my $result = "";

Please use GNU-like 2-space indentation for new code (see HACKING).

The tail of all Perl files already setups the indentation style
for Emacs.  Maybe you can submit similar hints for your editor.

 Robert> +      my @files = ();
 Robert> +      @files = split(/ /, $tmpfiles); 
 Robert> +      while (scalar @files > 0)
 Robert> +      {
 Robert> +      $_ = shift @files;

can be shortened to

foreach (split(/ /, $tmpfiles))
  {

[...]

 Robert> @@ -2145,7 +2193,7 @@ sub handle_programs
 Robert> # Canonicalize names and check for misspellings.
 Robert> my $xname = &check_canonical_spelling ($one_file, '_LDADD', '_LDFLAGS',
 Robert> '_SOURCES', '_OBJECTS',
 Robert> -                                           '_DEPENDENCIES');
 Robert> +                                           '_DEPENDENCIES', 
'_CFLAGS');
 
 Robert> $where->push_context ("while processing program `$one_file'");
 Robert> $where->set (INTERNAL->get);
 Robert> @@ -2250,7 +2298,7 @@ sub handle_libraries
 Robert> # Canonicalize names and check for misspellings.
 Robert> my $xlib = &check_canonical_spelling ($onelib, '_LIBADD', '_SOURCES',
 Robert> '_OBJECTS', '_DEPENDENCIES',
 Robert> -                                          '_AR');
 Robert> +                                          '_AR', '_CFLAGS');
 
 Robert> if (! var ($xlib . '_AR'))
 Robert> {
 Robert> @@ -2371,7 +2419,20 @@ sub handle_ltlibraries
 Robert> # Canonicalize names and check for misspellings.
 Robert> my $xlib = &check_canonical_spelling ($onelib, '_LIBADD', '_LDFLAGS',
 Robert> '_SOURCES', '_OBJECTS',
 Robert> -                                          '_DEPENDENCIES');
 Robert> +                                          '_DEPENDENCIES', '_CFLAGS');

It's not clear to me how this relates to the rest of this patch.
I agree more checks could be done here, but then they should probably
be extended to all language flags, not only C.

[...]

 Robert> +            error ("can't translate saw-bk, was_rule, $_") if 
$prepend_path ne "";

cryptic diagnostic without location...

[...]

 Robert> +              if ($last_var_name =~ /_([^_]+)$/o)
 Robert> +                {
 Robert> +                  my $var_suffix = $1;
 Robert> +                  if ($prepend_path ne "") 
 Robert> +                    {
 Robert> +                      grep
 Robert> +                        { 
 Robert> +                          if ($_ eq $var_suffix)
 Robert> +                            {
 Robert> +#                             error ("prepending path '$prepend_path' 
to '$last_var_value' ");
 Robert> +                              $last_var_value = 
transform_file_list($prepend_path, $last_var_value);
 Robert> +                            }
 Robert> +                        }
 Robert> +                      @canonised_macro_values;
 Robert> +                    }
 Robert> +                }

I don't think you can do this kind of rewriting here.  The point
is that a variable can refer to other variables.

COMMON_SOURCES = foo.c bar.h
foo_SOURCES = a.c b.c $(COMMON_SOURCES)

(of course COMMON_SOURCES could be conditionally defined)

You should also decide what to do with @SUBSTITIONS@ that might
occur in variable values (probably there is nothing to do but
ignore them).

Maybe you should look how &append_exeext works: it appends
$(EXEEXT) to all items in bin_PROGRAMS, handling conditionals,
nested variables, etc.

[...]

 Robert> --- tests/Makefile.am  30 Nov 2003 13:35:29 -0000      1.537
 Robert> +++ tests/Makefile.am  1 Dec 2003 08:41:27 -0000
 Robert> @@ -431,6 +431,8 @@ subdirbuiltsources.test \
 Robert> subcond.test \
 Robert> subcond2.test \
 Robert> subcond3.test \
 Robert> +subdir_include.test \
 Robert> +subdir_include_distcheck.test \

Ouch, please choose names which are unambiguous on 8+3 file-systems...

[...]

 Robert> #!/bin/sh

 Robert> # Test for subdir_include basic functionality.

 Robert> . defs || exit 1

  . ./defs || exit 1

(or . will obey PATH)

Also, please read tests/README for recommendations about writing
test cases.

[...]

 Robert> # Fail gracefully if no autoconf.
 Robert> $needs_autoconf

$needs_autoconf is not defined.  autoconf's presence is ensure
by Automake's configure anyway...

 Robert> # Likewise for gcc.
 Robert> (gcc -v) > /dev/null 2>&1 || exit 77

required=gcc (see tests/README)
-- 
Alexandre Duret-Lutz