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

From: Peter Rosin <[email-masked]>
Date: Wed, 23 Jan 2013 15:34:58 +0100

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 <[email-masked]>
>> 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 <[email-masked]>
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