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 <p...@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