Package: automake;
Reported by: Miles Bader <miles <at> gnu.org>
Date: Tue, 22 Jan 2013 09:20:02 UTC
Severity: wishlist
Tags: patch
Done: Stefano Lattarini <stefano.lattarini <at> gmail.com>
Bug is archived. No further changes may be made.
Message #27 received at 13524 <at> debbugs.gnu.org (full text, mbox):
From: Stefano Lattarini <stefano.lattarini <at> gmail.com> To: Peter Rosin <peda <at> lysator.liu.se> Cc: karl <at> freefriends.org, 13524 <at> debbugs.gnu.org, miles <at> gnu.org Subject: Re: bug#13524: Improving user experience for non-recursive builds Date: Wed, 23 Jan 2013 16:08:34 +0100
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 <peda <at> 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.
>
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/\@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 '.'?)
>
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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.