On 2013-01-23 13:45, Stefano Lattarini wrote:
> 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.

Well, apparently I was in the mood and found some more time :-)

> On 01/22/2013 11:22 AM, Peter Rosin wrote:
>> From 5cc9c775dbe46343b651a7e6ac378f71e6a3b6c1 Mon Sep 17 00:00:00 2001
>> From: Peter Rosin <peda@lysator.liu.se>
>> 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).

And a NEWS entry, and docs.  That part is less fun.

>> 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/\@am_reldir\@\//${reldir}\//g;
>> +	    $_ =~ s/\@am_reldir\@_/${rel_dir}_/g;
>> +	  }
>> +	else
>> +	  {
>> +	    $_ =~ s/\@am_reldir\@\///g;
>> +	    $_ =~ s/\@am_reldir\@_//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:

The gain was that the '.' case needs to peak at, and perhaps eat, the
trailing separator anyway (or it will look bad, I mean, who wants __foo
after writing &{CANON_CURDIR}&_foo and curdir happens to be '.'?)

>   # 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.

After a short brainstorm, I went with &{CUR/DIR}& and &{CUR_DIR}&, but
I'm not totally satisfied with the / version as it doesn't look "natural".
But it is self explanatory, kind of. I'm not attached to the naming in
any way, but &{CANON_CURDIR}& is too long IMHO.

>>  	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,/[^/]*$,,)

Yes, looks better. I haven't bothered to look up the perl doc that
spells out exactly why it is equivalent, but I'll trust you on
that.

>> +	      {
>> +	        $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!

Yes, it was simpler than I anticipated. After posting I noticed that
some project (was it coreutils?) used "include $(srcdir)/foo/local.mk"
and figured my changes wouldn't support that. But that works too, I
don't know why though. (not that I'm complaining)

Updated patch attached, I renamed it to curdir instead of reldir (and
sorry for not dropping the automake list last time around).

Cheers,
Peter