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

[Stefano Lattarini]

On 01/23/2013 03:34 PM, Peter Rosin wrote:
> 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 <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).
> 
> And a NEWS entry, and docs.
>
Yes, but I've found out those can often be written in follow-up patches
without too much churn or "history confusion"; OTOH, writing a clear and
complete commit message is essential, especially for a new feature.

> That part is less fun.
>
As for myself, I've actually reached a point where I find writing commit
messages quite interesting.  And I'm mostly neutral on NEWS entries.
But of course, writing documentation sucks ;-)

>>> 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:
> 
> 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 '.'?)
> 
Good point.  We should allow the user to write something like
"&{CANON_CURDIR}&foo_SOURCES" instead, so that it can work for the
current directory too; not much important for human-written makefiles,
but might be for autogenerated ones (I'm thinking ahead about a Gnulib
integration here; the current support for non-recursive projects
there is quite hacky, and could benefit from this new feature).

>>   # 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.
>
I agree we shouldn't overthink this; the naming is something that could be
changed easily in a follow-up patch, before this feature branch is integrated
back in master.

I'm also thinking we could introduce shorthands for often-used &{subst}&;
e.g., &{C}& for &{CANON_CURDIR}& and &{D}& for &{CURDIR}&.  Again, this is
material for a follow-up.

>>>     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"
>
That usage is advised by our own manual, so we should definitely
support it ...  As well as "include $(top_srcdir)/foo/local.mk"
(this latter might be trickier though).

> and figured my changes wouldn't support that. But that works too, I
> don't know why though. (not that I'm complaining)
> 
We should definitely add a test for that too; yes, I'm volunteering to
do so -- you know I like writing tests :-)

> Updated patch attached, I renamed it to curdir instead of reldir (and
> sorry for not dropping the automake list last time around).
> 
Thanks, I'll take a look at it tomorrow.

Regards,
  Stefano