[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: non recursive includes proof of concept #2

From: Alexandre Duret-Lutz
Subject: Re: non recursive includes proof of concept #2
Date: Fri, 19 Dec 2003 14:41:29 +0100
User-agent: Gnus/5.1003 (Gnus v5.10.3) Emacs/21.3 (gnu/linux)

>>> "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 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

 Robert> As show by the test cases, this allows a couple of neat things:
 Robert> 1) A stub 
 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 depcomp elisp-comp
 Robert> install-sh 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', 
 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> -                                          '_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

(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/  30 Nov 2003 13:35:29 -0000      1.537
 Robert> +++ tests/  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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]