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. 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
>From d7c8a35e2bd9bcab2718427636d41b3734c351b2 Mon Sep 17 00:00:00 2001
From: Peter Rosin <p...@lysator.liu.se>
Date: Wed, 23 Jan 2013 15:34:03 +0100
Subject: [PATCH] curdir: add support for relative names in included fragments
Suggested by Bob Friesenhahn, and later discussed in bug #13524.
automake.in (read_am_file): Add third argument specifying the
relative directory of this makefile fragment compared to the
main makefile. Replace &{CUR/DIR}& and &{CUR_DIR}& in the
fragment with this relative directory (with slashes etc, or
canonicalized).
(read_main_am_file): Adjust.
t/curdir.sh: New test.
t/list-of-tests.mk: Augment.
---
automake.in | 30 ++++++++++++++++++---
t/curdir.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
t/list-of-tests.mk | 1 +
3 files changed, 99 insertions(+), 5 deletions(-)
create mode 100755 t/curdir.sh
diff --git a/automake.in b/automake.in
index 0e3b882..9c9f41d 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, $CURDIR)
+# ----------------------------------------
# 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, $curdir) = @_;
my $am_file = new Automake::XFile ("< $amfile");
verb "reading $amfile";
@@ -6423,6 +6423,21 @@ sub read_am_file ($$)
my $new_saw_bk = check_trailing_slash ($where, $_);
+ if ($curdir ne '.')
+ {
+ my $cur_dir = &canonicalize ($curdir);
+ $_ =~ s/&\{CUR\/DIR\}&/${curdir}/g;
+ $_ =~ s/&\{CUR_DIR\}&/${cur_dir}/g;
+ }
+ else
+ {
+ # If present, eat the following '_' or '/', converting
+ # "&{CUR/DIR}&/foo" and "&{CUR_DIR}&_foo" into plain "foo"
+ # when $curdir is '.'.
+ $_ =~ s/&\{CUR([_\/])DIR\}&\1//g;
+ $_ =~ s/&\{CUR[_\/]DIR\}&/${curdir}/g;
+ }
+
if (/$IGNORE_PATTERN/o)
{
# Merely delete comments beginning with two hashes.
@@ -6584,8 +6599,13 @@ sub read_am_file ($$)
push_dist_common ("\$\(srcdir\)/$path");
$path = $relative_dir . "/" . $path if $relative_dir ne '.';
}
+ my $new_curdir = $path;
+ if ($new_curdir !~ s,/[^/]*$,,)
+ {
+ $new_curdir = '.';
+ }
$where->push_context ("'$path' included from here");
- &read_am_file ($path, $where);
+ &read_am_file ($path, $where, $new_curdir);
$where->pop_context;
}
else
@@ -6658,7 +6678,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, '.');
}
diff --git a/t/curdir.sh b/t/curdir.sh
new file mode 100755
index 0000000..e2251dc
--- /dev/null
+++ b/t/curdir.sh
@@ -0,0 +1,73 @@
+#! /bin/sh
+# Copyright (C) 2013 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2, or (at your option)
+# any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Test &{CUR/DIR}& and &{CUR_DIR}&.
+
+. test-init.sh
+
+cat >> configure.ac << 'END'
+AC_PROG_CC
+AM_PROG_CC_C_O
+AC_OUTPUT
+END
+
+mkdir foo
+mkdir foo/bar
+mkdir foo/foobar
+
+cat > Makefile.am << 'END'
+AUTOMAKE_OPTIONS = subdir-objects
+bin_PROGRAMS =
+include foo/local.mk
+include $(srcdir)/foo/foobar/local.mk
+include local.mk
+END
+
+cat > local.mk << 'END'
+&{CUR_DIR}&_whoami:
+ @echo "I am &{CUR/DIR}&/local.mk"
+
+bin_PROGRAMS += &{CUR/DIR}&/mumble
+&{CUR_DIR}&_mumble_SOURCES = &{CUR/DIR}&/one.c
+END
+
+cat > one.c << 'END'
+int main(void) { return 0; }
+END
+
+cp local.mk foo
+cp local.mk foo/bar
+cp local.mk foo/foobar
+echo "include &{CUR/DIR}&/bar/local.mk" >> foo/local.mk
+
+cp one.c foo
+cp one.c foo/bar
+cp one.c foo/foobar
+
+$ACLOCAL
+$AUTOCONF
+$AUTOMAKE -a
+./configure
+
+$MAKE whoami
+$MAKE foo_whoami
+$MAKE foo_bar_whoami
+$MAKE foo_foobar_whoami
+$MAKE
+./mumble
+foo/mumble
+foo/bar/mumble
+foo/foobar/mumble
diff --git a/t/list-of-tests.mk b/t/list-of-tests.mk
index 44f598e..1121735 100644
--- a/t/list-of-tests.mk
+++ b/t/list-of-tests.mk
@@ -340,6 +340,7 @@ t/cscope.tap \
t/cscope2.sh \
t/cscope3.sh \
t/c-demo.sh \
+t/curdir.sh \
t/cxx.sh \
t/cxx2.sh \
t/cxxcpp.sh \
--
1.7.9
