bug#13524: Improving user experience for non-recursive builds

[Stefano Lattarini]

Hi Peter, thanks for the patch.

Not sure if you are in the mood (or have the time) to engage in a
discussion about it, but here my review anyway.  Even if you are not
going to work on this patch anymore, a review will still be useful
as a reference to me or other developers in the future.

On 01/22/2013 11:22 AM, Peter Rosin wrote:
> From 5cc9c775dbe46343b651a7e6ac378f71e6a3b6c1 Mon Sep 17 00:00:00 2001
> From: Peter Rosin <address@hidden>
> Date: Tue, 22 Jan 2013 11:17:11 +0100
> Subject: [PATCH] reldir: Add support for relative names in included fragments
> 
A nice explanation of the ratioanle for this change is a must I think.

Also, we should add reference to this discussion (and related bug number),
as well give credit where's its due (this idea was Bob's brainchild).

> automake.in (read_am_file): Add third argument specifying the
> relative directory of this makefile fragment compared to the
> main level makefile. Replace @am_reldir@ in the fragment with
> this relative directory.
>
Using @acsubst@ style substitutions for something that is not substituted
by config.status has proven a bad idea in the patch.  We should use a new
syntax, like '%subst%', ot even '&{subst}&'; personally, I like this latter
best, since it help distinguish between Automake internal %TRASFORMS% from
this new kind of special user-reserved substitution.

> (read_main_am_file): Adjust.
> t/reldir.sh: New test.
> t/list-of-tests.mk: Augment.
> ---
>  automake.in        |   28 +++++++++++++++++----
>  t/list-of-tests.mk |    1 +
>  t/reldir.sh        |   68 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 92 insertions(+), 5 deletions(-)
>  create mode 100755 t/reldir.sh
> 
> diff --git a/automake.in b/automake.in
> index 0e3b882..4e52aca 100644
> --- a/automake.in
> +++ b/automake.in
> @@ -6330,15 +6330,15 @@ sub check_trailing_slash ($\$)
>  }
>  
>  
> -# &read_am_file ($AMFILE, $WHERE)
> -# -------------------------------
> +# &read_am_file ($AMFILE, $WHERE, $RELDIR)
> +# ----------------------------------------
>  # Read Makefile.am and set up %contents.  Simultaneously copy lines
>  # from Makefile.am into $output_trailer, or define variables as
>  # appropriate.  NOTE we put rules in the trailer section.  We want
>  # user rules to come after our generated stuff.
>  sub read_am_file ($$)
>  {
> -    my ($amfile, $where) = @_;
> +    my ($amfile, $where, $reldir) = @_;
>  
>      my $am_file = new Automake::XFile ("< $amfile");
>      verb "reading $amfile";
> @@ -6423,6 +6423,18 @@ sub read_am_file ($$)
>  
>       my $new_saw_bk = check_trailing_slash ($where, $_);
>  
> +     if ($reldir ne '.')
> +       {
> +         my $rel_dir = &canonicalize ($reldir);
> +         $_ =~ s/address@hidden@\//${reldir}\//g;
> +         $_ =~ s/address@hidden@_/${rel_dir}_/g;
> +       }
> +     else
> +       {
> +         $_ =~ s/address@hidden@\///g;
> +         $_ =~ s/address@hidden@_//g;
> +       }
> +
Too much automagic here IMO.  We'd better have two distinct subst, one for
the "real" directory name, and one for the directory name "canonicalized"
for use in Automake primaries.  I.e., from:

  # In sub/sub2/local.mk
  bin_PROGRAMS = sub/sub2/my-prog
  sub_sub2_my_prog_SOURCES = sub/sub2/main.c sub/sub2/compat.c

to:

  # In sub/sub2/local.mk
  bin_PROGRAMS = &{CURDIR}&/my-prog
  &{CANON_CURDIR}&_my_prog_SOURCES = &{CURDIR}&/main.c &{CURDIR}&/compat.c

Aa for what should be the actual names of this substitutions (I realize
the names above kinda suck), suggestions are welcome.

>       if (/$IGNORE_PATTERN/o)
>       {
>           # Merely delete comments beginning with two hashes.
> @@ -6584,8 +6596,14 @@ sub read_am_file ($$)
>               push_dist_common ("\$\(srcdir\)/$path");
>               $path = $relative_dir . "/" . $path if $relative_dir ne '.';
>             }
> +         my $new_reldir = $path;
> +         # $new_reldir =~ s/\$\($reldir\)\///;
> +         if ($new_reldir =~ s/\/[^\/]*$// == 0)
>
This is probably better written as:

    if ($new_reldir !~ s,/[^/]*$,,)

> +           {
> +             $new_reldir = '.';
> +           }
>           $where->push_context ("'$path' included from here");
> -         &read_am_file ($path, $where);
> +         &read_am_file ($path, $where, $new_reldir);
>           $where->pop_context;
>       }
>       else
> @@ -6658,7 +6676,7 @@ sub read_main_am_file ($$)
>      &define_standard_variables;
>  
>      # Read user file, which might override some of our values.
> -    &read_am_file ($amfile, new Automake::Location);
> +    &read_am_file ($amfile, new Automake::Location, '.');
>  }
>  
> [SNIP good testsuite addition]
>

I really like how simple and unobtrusive this patch is!

Thanks,
  Stefano