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

classic Classic list List threaded Threaded
83 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

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

Stefano Lattarini
[+cc bug-automake, so that we won't forget about the issue]
[future replies should drop the automake list]

On 01/22/2013 02:22 AM, Miles Bader wrote:
> Stefano Lattarini <[hidden email]> writes:
>> The best solution is on the user-side IMHO: fix the build system to
>> use less (ideally none) make recursion.  Both the parallel and serial
>> testsuite harness should support that setup OOTB.
>
> It would be nice if automake had some more features for that...
>
Indeed (albeit the present situation is already mostly good enough
for most medium-sized packages).

> E.g., if I have a directory "foo" that has sources etc, and builds
> some specific targets, then I can isolate the automake stuff for foo
> by using an include file "foo/Makefile.am.inc" or something, and then
> putting an appropriate include in the top-level Makefile.am.
>
> But it's a bit annoying, in that AFAICT, all filenames, etc, in foo's
> Makefile fragment must explicitly include the directory name.
>
Yes, and this issue has come up several times already.  Nobody has
been bothered enough to attempt a patch, though, at least so far.

> E.g., if it builds a library, "foo/Makefile.am.inc" might look like:
>
>    libfoo_a_SOURCES = foo/oink.c foo/barf.c foo/barf.h ...
>
> For longish directory names, this can really bloat things up...
>
Someone (probably Eric Blake, but I'm not 100% sure) once noted that this
issue could be mitigated with simple indirections with usual make macros:

  d1 = wow/a/very/very/insanely/long/directory/name

  wow_a_very_very_insanely_long_directory_name_prog_SOURCES = \
   $(d1)/a.c $(d1)/b.c ... $(d1)/z.c

> It would be really cool if there was some way of telling automake
> "hey, for every filename mentioned in this file, try to use a prefix
> of ..."
>
This is probably too automatic; but Bob Friesenhahn suggested Automake
could recognize special substitutions, like %CURDIR% and %XCURDIR%, so
that you could simply use in

    %XCURDIR%_prog_SOURCES = %CURDIR%/a.c %CURDIR%/b.c ... %CURDIR%/z.c

in 'wow/a/very/very/insanely/long/directory/name/local,.mk', include this
'.mk' fragment from the top-level Makefile.am, and have DTRT.  I think
that could be implemented in a simple pre-processing step, before
Automake even parses the Makefile contents (this too was Bob's suggestion,
IIRC); in which case, the change would probably be unobtrusive and mostly
safe.  Patches (even WIP) are welcome.

> I dunno whether that would be associated with the include
> directive, with the makefile fragment, or what, but... anyway.
>
> Does automake have some feature like this that I've missed?
>
Unfortunately no.

> Or has anybody thought about it?
>
They did (see above :-)

Thanks,
  Stefano



Reply | Threaded
Open this post in threaded view
|

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

Peter Rosin
On 2013-01-22 10:18, Stefano Lattarini wrote:

> [+cc bug-automake, so that we won't forget about the issue]
> [future replies should drop the automake list]
>
> On 01/22/2013 02:22 AM, Miles Bader wrote:
>> Stefano Lattarini <[hidden email]> writes:
>>> The best solution is on the user-side IMHO: fix the build system to
>>> use less (ideally none) make recursion.  Both the parallel and serial
>>> testsuite harness should support that setup OOTB.
>>
>> It would be nice if automake had some more features for that...
>>
> Indeed (albeit the present situation is already mostly good enough
> for most medium-sized packages).
>
>> E.g., if I have a directory "foo" that has sources etc, and builds
>> some specific targets, then I can isolate the automake stuff for foo
>> by using an include file "foo/Makefile.am.inc" or something, and then
>> putting an appropriate include in the top-level Makefile.am.
>>
>> But it's a bit annoying, in that AFAICT, all filenames, etc, in foo's
>> Makefile fragment must explicitly include the directory name.
>>
> Yes, and this issue has come up several times already.  Nobody has
> been bothered enough to attempt a patch, though, at least so far.
>
>> E.g., if it builds a library, "foo/Makefile.am.inc" might look like:
>>
>>    libfoo_a_SOURCES = foo/oink.c foo/barf.c foo/barf.h ...
>>
>> For longish directory names, this can really bloat things up...
>>
> Someone (probably Eric Blake, but I'm not 100% sure) once noted that this
> issue could be mitigated with simple indirections with usual make macros:
>
>   d1 = wow/a/very/very/insanely/long/directory/name
>
>   wow_a_very_very_insanely_long_directory_name_prog_SOURCES = \
>    $(d1)/a.c $(d1)/b.c ... $(d1)/z.c
>
>> It would be really cool if there was some way of telling automake
>> "hey, for every filename mentioned in this file, try to use a prefix
>> of ..."
>>
> This is probably too automatic; but Bob Friesenhahn suggested Automake
> could recognize special substitutions, like %CURDIR% and %XCURDIR%, so
> that you could simply use in
>
>     %XCURDIR%_prog_SOURCES = %CURDIR%/a.c %CURDIR%/b.c ... %CURDIR%/z.c
>
> in 'wow/a/very/very/insanely/long/directory/name/local,.mk', include this
> '.mk' fragment from the top-level Makefile.am, and have DTRT.  I think
> that could be implemented in a simple pre-processing step, before
> Automake even parses the Makefile contents (this too was Bob's suggestion,
> IIRC); in which case, the change would probably be unobtrusive and mostly
> safe.  Patches (even WIP) are welcome.
This is proof of concept, and I'm not a perl hacker etc, but it seems to
work ok. Feel free to improve or toss or add documentation or whatever :-)

Patch is based on the maint branch, but being a new feature it perhaps
belongs on master instead. But as I said, do what you want with it...

Cheers,
Peter


0001-reldir-Add-support-for-relative-names-in-included-fr.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Miles Bader-2
In reply to this post by Stefano Lattarini
Stefano Lattarini <[hidden email]> writes:

>> E.g., if I have a directory "foo" that has sources etc, and builds
>> some specific targets, then I can isolate the automake stuff for foo
>> by using an include file "foo/Makefile.am.inc" or something, and then
>> putting an appropriate include in the top-level Makefile.am.
>>
>> But it's a bit annoying, in that AFAICT, all filenames, etc, in foo's
>> Makefile fragment must explicitly include the directory name.
>>
> Yes, and this issue has come up several times already.  Nobody has
> been bothered enough to attempt a patch, though, at least so far.
>
>> E.g., if it builds a library, "foo/Makefile.am.inc" might look like:
>>
>>    libfoo_a_SOURCES = foo/oink.c foo/barf.c foo/barf.h ...
>>
>> For longish directory names, this can really bloat things up...
>>
> Someone (probably Eric Blake, but I'm not 100% sure) once noted that this
> issue could be mitigated with simple indirections with usual make macros:
>
>   d1 = wow/a/very/very/insanely/long/directory/name
>
>   wow_a_very_very_insanely_long_directory_name_prog_SOURCES = \
>    $(d1)/a.c $(d1)/b.c ... $(d1)/z.c

Yes, that's the method I currently use, but it's pretty ugly, and kind
of fiddly -- you need to use a unique macro name for every subdir, and
practicality means probably using some variant of the directory name
for that... meaning you probably have longish variable names in
practice.

Ugliness is better than "broken" but frankly I don't want it; one of
automake's big attractions for me is that it allows one to write
highly readable makefiles that are more easily maintainable because of
their relative simplicity and lack of boilerplate (which hinders
readability by obscuring significant content).  Stuff like this feels
very similar to typical Makefile boilerplate, and I think makes for
more fragile, less maintainable Makefiles.

> This is probably too automatic; but Bob Friesenhahn suggested Automake
> could recognize special substitutions, like %CURDIR% and %XCURDIR%, so
> that you could simply use in
>
>     %XCURDIR%_prog_SOURCES = %CURDIR%/a.c %CURDIR%/b.c ... %CURDIR%/z.c

This is less fragile, but still pretty grotty, that's going to result
in makefile[-fragment]s that feel like they're full of boilerplate.

Really, I'm thinking about something _more_ magic.

I want to write filenames as I think about them:

  ..something...:  a.c b.c ... z.c

If something that does automagic munging of all filenames in a
makefile fragment is too magic, at least maybe this could be done with
some sort of simple rewrite mechanism to at least automake the common
factor.

E.g.:

  libfoo_SOURCES[libfoo/]: a.b b.c ... z.c

This sort of thing would have the advantage of being relatively
"stupid" (a simple explicit string rewrite), while still being able to
eliminate much of the redundancy...

Thanks,

-miles

--
「寒いね」と話しかければ「寒いね」と答える人のいるあったかさ [俵万智]



Reply | Threaded
Open this post in threaded view
|

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

Stefano Lattarini
Hello Miles, thanks for the feedback.

On 01/23/2013 07:54 AM, Miles Bader wrote:

> Stefano Lattarini <[hidden email]> writes:
>>> E.g., if I have a directory "foo" that has sources etc, and builds
>>> some specific targets, then I can isolate the automake stuff for foo
>>> by using an include file "foo/Makefile.am.inc" or something, and then
>>> putting an appropriate include in the top-level Makefile.am.
>>>
>>> But it's a bit annoying, in that AFAICT, all filenames, etc, in foo's
>>> Makefile fragment must explicitly include the directory name.
>>>
>> Yes, and this issue has come up several times already.  Nobody has
>> been bothered enough to attempt a patch, though, at least so far.
>>
>>> E.g., if it builds a library, "foo/Makefile.am.inc" might look like:
>>>
>>>    libfoo_a_SOURCES = foo/oink.c foo/barf.c foo/barf.h ...
>>>
>>> For longish directory names, this can really bloat things up...
>>>
>> Someone (probably Eric Blake, but I'm not 100% sure) once noted that this
>> issue could be mitigated with simple indirections with usual make macros:
>>
>>   d1 = wow/a/very/very/insanely/long/directory/name
>>
>>   wow_a_very_very_insanely_long_directory_name_prog_SOURCES = \
>>    $(d1)/a.c $(d1)/b.c ... $(d1)/z.c
>
> Yes, that's the method I currently use, but it's pretty ugly, and kind
> of fiddly -- you need to use a unique macro name for every subdir,
>
Yes, that is annoying and somewhat fragile.

> and practicality means probably using some variant of the directory
> name for that... meaning you probably have longish variable names in
> practice.
>
> Ugliness is better than "broken" but frankly I don't want it; one of
> automake's big attractions for me is that it allows one to write
> highly readable makefiles that are more easily maintainable because of
> their relative simplicity and lack of boilerplate (which hinders
> readability by obscuring significant content).  Stuff like this feels
> very similar to typical Makefile boilerplate, and I think makes for
> more fragile, less maintainable Makefiles.
>
>> This is probably too automatic; but Bob Friesenhahn suggested Automake
>> could recognize special substitutions, like %CURDIR% and %XCURDIR%, so
>> that you could simply use in
>>
>>     %XCURDIR%_prog_SOURCES = %CURDIR%/a.c %CURDIR%/b.c ... %CURDIR%/z.c
>
> This is less fragile,
>
Which is a step forward at least; and since it appears it might be very
easy to implement and fit in the existing automake processing scheme, I'm
willing to give it a try.  Further improvements and refinement might go
on the top of that.

> but still pretty grotty, that's going to result
> in makefile[-fragment]s that feel like they're full of boilerplate.
>
> Really, I'm thinking about something _more_ magic.
>
It might be nice to do so as a second step, but I'd rather not have
the best be the enemy of the good; we can make the Makefiles less
fragile *today*, and make them less "boilerplatish" tomorrow.

> I want to write filenames as I think about them:
>
>   ..something...:  a.c b.c ... z.c
>
> If something that does automagic munging of all filenames in a
> makefile fragment is too magic, at least maybe this could be done with
> some sort of simple rewrite mechanism to at least automake the common
> factor.
>
> E.g.:
>
>   libfoo_SOURCES[libfoo/]: a.b b.c ... z.c
>
This could be "syntactic sugar" that we might add after the improvement
suggested above; in fact, the two things could eventually compound to
allow usages like:

  libfoo_SOURCES[%CURDIR%] = a.c b.c ... z.c

> This sort of thing would have the advantage of being relatively
> "stupid" (a simple explicit string rewrite), while still being able to
> eliminate much of the redundancy...
>
I'm not opposed to the idea, but I see it as a second step, after the
improvement we are discussing.  A second step for which patches are,
as usual, welcome :-)

> Thanks,
>
> -miles
>

Thanks, and best regards,
  Stefano



Reply | Threaded
Open this post in threaded view
|

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

Stefano Lattarini
In reply to this post by Peter Rosin
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.

On 01/22/2013 11:22 AM, Peter Rosin wrote:
> From 5cc9c775dbe46343b651a7e6ac378f71e6a3b6c1 Mon Sep 17 00:00:00 2001
> From: Peter Rosin <[hidden email]>
> 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).

> 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:

  # 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.

>   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,/[^/]*$,,)

> +      {
> +        $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!

Thanks,
  Stefano



Reply | Threaded
Open this post in threaded view
|

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

Peter Rosin
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 <[hidden email]>
>> 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


0001-curdir-add-support-for-relative-names-in-included-fr.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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

Peter Rosin
On 2013-01-23 16:08, Stefano Lattarini wrote:
> On 01/23/2013 03:34 PM, Peter Rosin wrote:
>> On 2013-01-23 13:45, Stefano Lattarini wrote:
*snip*

>>> 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).
Are you saying that &{CURDIR}& should be replaced with the empty
string when the current relative directory is "." and the current
relative directory followed by a slash otherwise? (And similar, but
with a trailing underscore for &{CANON_CURDIR}&) I don't fancy
that, as I think &{CURDIR}&gazonk.c is that much harder to read than
&{CURDIR}&/gazonk.c.

So, I'd rather have the magic extend beyond the }& even if that is
ugly indeed. Maybe I'm just deluded to want that...

Or, do you mean that &{CURDIR}& should peak ahead and only add a
trailing "/" if it is not followed by a slash (or whitespace?)
already, making "&{CURDIR}&foo.c" and "&{CURDIR}&/foo.c" equivalent
except when &{CURDIR}& is "." (which would come out as "foo.c" and
"./foo.c" respectively)?

*snip*
> Thanks, I'll take a look at it tomorrow.

Another day, another version. I hope you didn't wast too much time
on v2...

I changed to &{CANON_CURDIR}& and &{CURDIR}&, added support for
$(top_srcdir) and improved the test. There's more code due to the
$(top_srcdir) support, and maybe there are some preexisting
functionality that strips common leading directory components that
could have been reused, but I'm ignorant. Besides, what's wrong
with NIH? :-)

Cheers,
Peter


0001-curdir-add-support-for-relative-names-in-included-fr.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Peter Rosin
On 2013-01-24 13:22, Peter Rosin wrote:

> On 2013-01-23 16:08, Stefano Lattarini wrote:
>> On 01/23/2013 03:34 PM, Peter Rosin wrote:
>>> On 2013-01-23 13:45, Stefano Lattarini wrote:
> *snip*
>>>> 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).
>
> Are you saying that &{CURDIR}& should be replaced with the empty
> string when the current relative directory is "." and the current
> relative directory followed by a slash otherwise? (And similar, but
> with a trailing underscore for &{CANON_CURDIR}&) I don't fancy
> that, as I think &{CURDIR}&gazonk.c is that much harder to read than
> &{CURDIR}&/gazonk.c.
>
> So, I'd rather have the magic extend beyond the }& even if that is
> ugly indeed. Maybe I'm just deluded to want that...
>
> Or, do you mean that &{CURDIR}& should peak ahead and only add a
> trailing "/" if it is not followed by a slash (or whitespace?)
> already, making "&{CURDIR}&foo.c" and "&{CURDIR}&/foo.c" equivalent
> except when &{CURDIR}& is "." (which would come out as "foo.c" and
> "./foo.c" respectively)?
>
> *snip*
>> Thanks, I'll take a look at it tomorrow.
>
> Another day, another version. I hope you didn't wast too much time
> on v2...
>
> I changed to &{CANON_CURDIR}& and &{CURDIR}&, added support for
> $(top_srcdir) and improved the test. There's more code due to the
> $(top_srcdir) support, and maybe there are some preexisting
> functionality that strips common leading directory components that
> could have been reused, but I'm ignorant. Besides, what's wrong
> with NIH? :-)
Zapping the NIH part reduced the code size significantly (the patch
is now short, sweet and unintrusive again) so I'm posting a new version.
After all, it's a new day, right?

I hope it's ok to use File::Spec->abs2rel () in Automake?

BTW, let me know if I should trim the CC list.

Cheers,
Peter


0001-curdir-add-support-for-relative-names-in-included-fr.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Peter Rosin
On 2013-01-25 17:03, Peter Rosin wrote:

> On 2013-01-24 13:22, Peter Rosin wrote:
>> On 2013-01-23 16:08, Stefano Lattarini wrote:
>>> On 01/23/2013 03:34 PM, Peter Rosin wrote:
>>>> On 2013-01-23 13:45, Stefano Lattarini wrote:
>> *snip*
>>>>> 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).
>>
>> Are you saying that &{CURDIR}& should be replaced with the empty
>> string when the current relative directory is "." and the current
>> relative directory followed by a slash otherwise? (And similar, but
>> with a trailing underscore for &{CANON_CURDIR}&) I don't fancy
>> that, as I think &{CURDIR}&gazonk.c is that much harder to read than
>> &{CURDIR}&/gazonk.c.
>>
>> So, I'd rather have the magic extend beyond the }& even if that is
>> ugly indeed. Maybe I'm just deluded to want that...
>>
>> Or, do you mean that &{CURDIR}& should peak ahead and only add a
>> trailing "/" if it is not followed by a slash (or whitespace?)
>> already, making "&{CURDIR}&foo.c" and "&{CURDIR}&/foo.c" equivalent
>> except when &{CURDIR}& is "." (which would come out as "foo.c" and
>> "./foo.c" respectively)?
>>
>> *snip*
>>> Thanks, I'll take a look at it tomorrow.
>>
>> Another day, another version. I hope you didn't wast too much time
>> on v2...
>>
>> I changed to &{CANON_CURDIR}& and &{CURDIR}&, added support for
>> $(top_srcdir) and improved the test. There's more code due to the
>> $(top_srcdir) support, and maybe there are some preexisting
>> functionality that strips common leading directory components that
>> could have been reused, but I'm ignorant. Besides, what's wrong
>> with NIH? :-)
>
> Zapping the NIH part reduced the code size significantly (the patch
> is now short, sweet and unintrusive again) so I'm posting a new version.
> After all, it's a new day, right?
>
> I hope it's ok to use File::Spec->abs2rel () in Automake?
This time with documentation and a NEWS entry. I also fixed the case
of including something above the current base Makefile.am with a
relative path, e.g.:

        include ../top.mk

That change shaved a couple of more lines. Neat.

I also rebased it, so now it is against master.

Cheers,
Peter


0001-curdir-add-support-for-relative-names-in-included-fr.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Stefano Lattarini
Hi Peter.

On 01/27/2013 01:54 AM, Peter Rosin wrote:
>
> [SNIP]
>
>> Zapping the NIH part reduced the code size significantly (the patch
>> is now short, sweet and unintrusive again) so I'm posting a new version.
>> After all, it's a new day, right?
>>
>> I hope it's ok to use File::Spec->abs2rel () in Automake?
>
Yes, File::Spec is a core perl module.

>
> This time with documentation and a NEWS entry. I also fixed the case
> of including something above the current base Makefile.am with a
> relative path, e.g.:
>
> include ../top.mk
>
> That change shaved a couple of more lines. Neat.
>
> I also rebased it, so now it is against master.
>
Thanks, it seems nice and good.  I'm doing some testsuite enhancements,
and then I have some proposals for further tweaks, but all of those
should be for follow-up patches.  Will you allow me some days to prepare
a follow-up patch series for further discussion?

Thanks,
  Stefano



Reply | Threaded
Open this post in threaded view
|

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

Peter Rosin
Hi Stefano,

On 2013-01-27 20:21, Stefano Lattarini wrote:

>> This time with documentation and a NEWS entry. I also fixed the case
>> of including something above the current base Makefile.am with a
>> relative path, e.g.:
>>
>> include ../top.mk
>>
>> That change shaved a couple of more lines. Neat.
>>
>> I also rebased it, so now it is against master.
>>
> Thanks, it seems nice and good.  I'm doing some testsuite enhancements,
> and then I have some proposals for further tweaks, but all of those
> should be for follow-up patches.  Will you allow me some days to prepare
> a follow-up patch series for further discussion?

Sure, take your time, I feel like I'm done. For about the fifth
time :-) or something like that. Besides, the patch will not be of
all that much use to me until it lands in a released version anyway.

Cheers,
Peter




Reply | Threaded
Open this post in threaded view
|

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

Bert Wesarg-2
Hi all,

On Mon, Jan 28, 2013 at 12:38 AM, Peter Rosin <[hidden email]> wrote:

> Hi Stefano,
>
> On 2013-01-27 20:21, Stefano Lattarini wrote:
>>> This time with documentation and a NEWS entry. I also fixed the case
>>> of including something above the current base Makefile.am with a
>>> relative path, e.g.:
>>>
>>>      include ../top.mk
>>>
>>> That change shaved a couple of more lines. Neat.
>>>
>>> I also rebased it, so now it is against master.
>>>
>> Thanks, it seems nice and good.  I'm doing some testsuite enhancements,
>> and then I have some proposals for further tweaks, but all of those
>> should be for follow-up patches.  Will you allow me some days to prepare
>> a follow-up patch series for further discussion?
>
> Sure, take your time, I feel like I'm done. For about the fifth
> time :-) or something like that. Besides, the patch will not be of
> all that much use to me until it lands in a released version anyway.

while I didn't try the current patch out. I would like to describe in
short the situation where one of our projects build system is in:
Sure, we use a non-recursive build. But we use the same sources in one
package by two sub-configures to build different targets.

Our makefiles which we include in the top-level Makefile.am look like
this (we name them 'Makefile.inc.am):

src/Makefile.inc.am:

bin_PROGRAM = foo
foo_SOURCES = ../src/foo.c

and in the top-level Makefile.am:

build-1/Makefile.am:

include ../src/Makefile.inc.am

So, using this patch, we would change the src/Makefile.inc.am to:

bin_PROGRAM = foo
foo_SOURCES = &{D}&/foo.c

Right?

Thanks.
Bert

>
> Cheers,
> Peter
>



Reply | Threaded
Open this post in threaded view
|

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

Stefano Lattarini
On 02/01/2013 04:18 PM, Bert Wesarg wrote:

> Hi all,
>
> On Mon, Jan 28, 2013 at 12:38 AM, Peter Rosin <[hidden email]> wrote:
>> Hi Stefano,
>>
>> On 2013-01-27 20:21, Stefano Lattarini wrote:
>>>> This time with documentation and a NEWS entry. I also fixed the case
>>>> of including something above the current base Makefile.am with a
>>>> relative path, e.g.:
>>>>
>>>>      include ../top.mk
>>>>
>>>> That change shaved a couple of more lines. Neat.
>>>>
>>>> I also rebased it, so now it is against master.
>>>>
>>> Thanks, it seems nice and good.  I'm doing some testsuite enhancements,
>>> and then I have some proposals for further tweaks, but all of those
>>> should be for follow-up patches.  Will you allow me some days to prepare
>>> a follow-up patch series for further discussion?
>>
>> Sure, take your time, I feel like I'm done. For about the fifth
>> time :-) or something like that. Besides, the patch will not be of
>> all that much use to me until it lands in a released version anyway.
>
> while I didn't try the current patch out. I would like to describe in
> short the situation where one of our projects build system is in:
> Sure, we use a non-recursive build. But we use the same sources in one
> package by two sub-configures to build different targets.
>
> Our makefiles which we include in the top-level Makefile.am look like
> this (we name them 'Makefile.inc.am):
>
> src/Makefile.inc.am:
>
> bin_PROGRAM = foo
> foo_SOURCES = ../src/foo.c
>
> and in the top-level Makefile.am:
>
> build-1/Makefile.am:
>
> include ../src/Makefile.inc.am
>
> So, using this patch, we would change the src/Makefile.inc.am to:
>
> bin_PROGRAM = foo
> foo_SOURCES = &{D}&/foo.c
>
> Right?
>
Yes, that should work.

Best regards,
  Stefano



Reply | Threaded
Open this post in threaded view
|

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

Stefano Lattarini
In reply to this post by Peter Rosin
tags 13524 + patch
thanks

[+cc automake-patches]

Reference:
<http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13524>

On 01/28/2013 12:38 AM, Peter Rosin wrote:

> Hi Stefano,
>
> On 2013-01-27 20:21, Stefano Lattarini wrote:
>>> This time with documentation and a NEWS entry. I also fixed the case
>>> of including something above the current base Makefile.am with a
>>> relative path, e.g.:
>>>
>>> include ../top.mk
>>>
>>> That change shaved a couple of more lines. Neat.
>>>
>>> I also rebased it, so now it is against master.
>>>
>> Thanks, it seems nice and good.  I'm doing some testsuite enhancements,
>> and then I have some proposals for further tweaks, but all of those
>> should be for follow-up patches.  Will you allow me some days to prepare
>> a follow-up patch series for further discussion?
>
> Sure, take your time, I feel like I'm done. For about the fifth
> time :-) or something like that. Besides, the patch will not be of
> all that much use to me until it lands in a released version anyway.
>
I've pushed the promised patches to the rewindable branch
'experimental/preproc' (based off of maint).  I'll also soon
send them to the list to simplify review (I will drop the
bug tracker from CC:, to avoid cluttering up the report).

As usual, reviews are welcome.

Thanks,
  Stefano





Reply | Threaded
Open this post in threaded view
|

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

Peter Rosin
On 2013-02-03 21:42, Stefano Lattarini wrote:
> I've pushed the promised patches to the rewindable branch
> 'experimental/preproc' (based off of maint).  I'll also soon
> send them to the list to simplify review (I will drop the
> bug tracker from CC:, to avoid cluttering up the report).
>
> As usual, reviews are welcome.

I like the end result of this series, I especially like that I don't have
to type &{this}& anymore. But I have some doubts whether the longish
way to get there is really all that interesting? The argument that you
should keep the history to show how you get from A to B is just not worth
it when it's just about bike-shedding the naming... Why not merge 1 with
the nobrainer renames of 3-6? The only "real changes" in the later patches
are the $$ -> $$$ thing and the NEWS entry as far as I can tell, and if
something should be in its own patch, that $$$ change is it. Oh right,
there's the re-computing change as well. But I don't think it's worth it,
just collapse it. The only sane way to look at this series is to diff
from 6/6 to the branch-point, which should tell you something. But I
don't feel strongly about it, it's just what I would have done. If 2/6
is a separate patch is not important to me either, so I would be ok with
you collapsing the whole series into a single co-authored patch. But then
again, I don't really care, and if you get tired from just thinking about
rewriting the commit message I can fully understand that position.

Another thing is that your new NEWS item is a bit awkward, and its
single sentence is simply too long and winding IMHO. The * heading
also needs an update.

From your 5/6:

* Current directory in makefile fragments:

  - The special Automake-time substitutions '{RELDIR}' and '{CANON_RELDIR}'
    (and their abbreviated versions, '{D}' and '{C}' respectively) can now
    be used in an included Makefile fragment to indicate respectively the
    relative directory of that fragment and its canonicalized version,
    relative to the including Makefile:

My suggestion:

* Relative directory in Makefile fragments:

  - The special Automake-time substitutions '{RELDIR}' and '{CANON_RELDIR}'
    (and their abbreviated versions, '{D}' and '{C}' respectively) can now
    be used in an included Makefile fragment.  They are substituted with
    the relative directory of the included fragment, or its canonicalized
    version, compared to the top level including Makefile:

Cheers,
Peter

PS. You introduced the curdir naming, I had reldir in my original patch. :-)




Reply | Threaded
Open this post in threaded view
|

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

Peter Rosin
On 2013-02-04 00:10, Peter Rosin wrote:

> On 2013-02-03 21:42, Stefano Lattarini wrote:
>> I've pushed the promised patches to the rewindable branch
>> 'experimental/preproc' (based off of maint).  I'll also soon
>> send them to the list to simplify review (I will drop the
>> bug tracker from CC:, to avoid cluttering up the report).
>>
>> As usual, reviews are welcome.
>
> I like the end result of this series, I especially like that I don't have
> to type &{this}& anymore.

*snip*

Oops, a couple more things. The new naming will clobber
anyone using a Makefile variable named RELDIR with brace
syntax. E.g., this previously working code is broken by
the series.

RELDIR = src
foobar_CPPFLAGS = $(AM_CPPFLAGS) -DRELDIR=${RELDIR}

It will be preprocessed into

RELDIR = src
foobar_CPPFLAGS = $(AM_CPPFLAGS) -DRELDIR=$src

and quasi-expanded into (assuming $s is empty)

foobar_CPPFLAGS = $(AM_CPPFLAGS) -DRELDIR=rc

Not sure what to do about it, or if it matters...



Further, the use of /dev/full in the demo test is not
portable. If I create /dev on Windows (i.e. C:\dev, or have
it previously, I didn't, but not unlikely), and am allowed
to write there (likely, on Windows), and /dev/full isn't
supported (where is it supported, except Linux?), then the
test falls apart. The MinGW program happily writes "dummy"
to /dev/full (i.e. C:\dev\full from its view of the file
system) but the MSYS script does not think /dev/full is a
character device and it is not be allowed to write to
/dev/full either, because typically /dev doesn't exist as
a real directory. Typically /dev is virtual in MSYS (if it
existed, it would be C:\MinGW\msys\1.0\dev from the MSYS
view of the file system) so it assumes the MinGW program
skipped, but it didn't.

Why use special features like /dev/full in the testsuite of
Automake? Surely there are better things one can do to check if
a Makefile works. Mixing in stuff like that is just a recipe
for strange bug reports and hardship for those on weird
platforms. The gain for Automake is zero.

Also, creating the file /dev/full with content "dummy" is
bad manors, even if someone runs the testsuite as root (on a
system not supporting /dev/full) and arguably deserves it.

Cheers,
Peter




Reply | Threaded
Open this post in threaded view
|

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

Stefano Lattarini
In reply to this post by Peter Rosin
On 02/04/2013 12:10 AM, Peter Rosin wrote:

> On 2013-02-03 21:42, Stefano Lattarini wrote:
>> I've pushed the promised patches to the rewindable branch
>> 'experimental/preproc' (based off of maint).  I'll also soon
>> send them to the list to simplify review (I will drop the
>> bug tracker from CC:, to avoid cluttering up the report).
>>
>> As usual, reviews are welcome.
>
> I like the end result of this series, I especially like that I don't have
> to type &{this}& anymore. But I have some doubts whether the longish
> way to get there is really all that interesting?
>
No, not really; the different approaches and names we tried can just be
reported in the commit message, rather than littering up the Git history.

So I'll squash all the patches, excluding only the second one, which I
still wish to keep separate.

> [SNIP]  rest of rationle

> Another thing is that your new NEWS item is a bit awkward, and its
> single sentence is simply too long and winding IMHO. The * heading
> also needs an update.
>
> From your 5/6:
>
> * Current directory in makefile fragments:
>
Oops, I had indeed forgotten to update this!

>   - The special Automake-time substitutions '{RELDIR}' and '{CANON_RELDIR}'
>     (and their abbreviated versions, '{D}' and '{C}' respectively) can now
>     be used in an included Makefile fragment to indicate respectively the
>     relative directory of that fragment and its canonicalized version,
>     relative to the including Makefile:
>
> My suggestion:
>
> * Relative directory in Makefile fragments:
>
>   - The special Automake-time substitutions '{RELDIR}' and '{CANON_RELDIR}'
>     (and their abbreviated versions, '{D}' and '{C}' respectively) can now
>     be used in an included Makefile fragment.  They are substituted with
>     the relative directory of the included fragment, or its canonicalized
>     version, compared to the top level including Makefile:
>
Yes, better.  But I find the following even better (marginally):

    The special Automake-time substitutions '{RELDIR}' and '{CANON_RELDIR}'
    (and their abbreviated versions, '{D}' and '{C}' respectively) can now
    be used in an included Makefile fragment.  They are substituted,
    respectively, with the relative directory of the included fragment and
    its canonicalized version, compared to the top level including Makefile:

OK?

> PS. You introduced the curdir naming, I had reldir in my original patch. :-)
>
Yikes, sorry.

Regards,
  Stefano



Reply | Threaded
Open this post in threaded view
|

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

Stefano Lattarini
In reply to this post by Peter Rosin
On 02/04/2013 10:35 AM, Peter Rosin wrote:

> On 2013-02-04 00:10, Peter Rosin wrote:
>> On 2013-02-03 21:42, Stefano Lattarini wrote:
>>> I've pushed the promised patches to the rewindable branch
>>> 'experimental/preproc' (based off of maint).  I'll also soon
>>> send them to the list to simplify review (I will drop the
>>> bug tracker from CC:, to avoid cluttering up the report).
>>>
>>> As usual, reviews are welcome.
>>
>> I like the end result of this series, I especially like that I don't have
>> to type &{this}& anymore.
>
> *snip*
>
> Oops, a couple more things. The new naming will clobber
> anyone using a Makefile variable named RELDIR with brace
> syntax.
Yikes, I didn't think of that.  *blush*

> E.g., this previously working code is broken by
> the series.
>
> RELDIR = src
> foobar_CPPFLAGS = $(AM_CPPFLAGS) -DRELDIR=${RELDIR}
>
> It will be preprocessed into
>
> RELDIR = src
> foobar_CPPFLAGS = $(AM_CPPFLAGS) -DRELDIR=$src
>
Ouch.

> and quasi-expanded into (assuming $s is empty)
>
> foobar_CPPFLAGS = $(AM_CPPFLAGS) -DRELDIR=rc
>
> Not sure what to do about it, or if it matters...
>
It does IMHO, since the failure you pointed out, albeit easy to
work around, wouldn't be very obvious to diagnose, from the point
of view of a non-particularly-expert user.

What about doubling the curly braces?  As in '{{RELDIR}}'.
Would that be tolerable?  Other possibilities (none particularly
pleasant either, IMHO):

  {+RELDIR+}
  {:RELDIR:}
  {.RELDIR.}
  {-RELDIR-}

Other proposals?

>
> Further, the use of /dev/full in the demo test is not
> portable. If I create /dev on Windows (i.e. C:\dev, or have
> it previously, I didn't, but not unlikely), and am allowed
> to write there (likely, on Windows), and /dev/full isn't
> supported (where is it supported, except Linux?), then the
> test falls apart. The MinGW program happily writes "dummy"
> to /dev/full (i.e. C:\dev\full from its view of the file
> system) but the MSYS script does not think /dev/full is a
> character device and it is not be allowed to write to
> /dev/full either, because typically /dev doesn't exist as
> a real directory. Typically /dev is virtual in MSYS (if it
> existed, it would be C:\MinGW\msys\1.0\dev from the MSYS
> view of the file system) so it assumes the MinGW program
> skipped, but it didn't.
>
> Why use special features like /dev/full in the testsuite of
> Automake? Surely there are better things one can do to check if
> a Makefile works. Mixing in stuff like that is just a recipe
> for strange bug reports and hardship for those on weird
> platforms. The gain for Automake is zero.
>
> Also, creating the file /dev/full with content "dummy" is
> bad manors, even if someone runs the testsuite as root (on a
> system not supporting /dev/full) and arguably deserves it.
>
Mostly agreed; I tried to create the test as a reasonable
demo mimicking real-world packages, but I've probably gotten
carried too far, for no real advantage (but with potential
lurking issues down the road).  I will simplify the test.

Thanks,
  Stefano



Reply | Threaded
Open this post in threaded view
|

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

Peter Rosin
In reply to this post by Stefano Lattarini
On 2013-02-04 12:23, Stefano Lattarini wrote:

> On 02/04/2013 12:10 AM, Peter Rosin wrote:
>> On 2013-02-03 21:42, Stefano Lattarini wrote:
>>> I've pushed the promised patches to the rewindable branch
>>> 'experimental/preproc' (based off of maint).  I'll also soon
>>> send them to the list to simplify review (I will drop the
>>> bug tracker from CC:, to avoid cluttering up the report).
>>>
>>> As usual, reviews are welcome.
>>
>> I like the end result of this series, I especially like that I don't have
>> to type &{this}& anymore. But I have some doubts whether the longish
>> way to get there is really all that interesting?
>>
> No, not really; the different approaches and names we tried can just be
> reported in the commit message, rather than littering up the Git history.
>
> So I'll squash all the patches, excluding only the second one, which I
> still wish to keep separate.

Good.

>> [SNIP]  rest of rationle
>
>> Another thing is that your new NEWS item is a bit awkward, and its
>> single sentence is simply too long and winding IMHO. The * heading
>> also needs an update.
>>
>> From your 5/6:
>>
>> * Current directory in makefile fragments:
>>
> Oops, I had indeed forgotten to update this!
>
>>   - The special Automake-time substitutions '{RELDIR}' and '{CANON_RELDIR}'
>>     (and their abbreviated versions, '{D}' and '{C}' respectively) can now
>>     be used in an included Makefile fragment to indicate respectively the
>>     relative directory of that fragment and its canonicalized version,
>>     relative to the including Makefile:
>>
>> My suggestion:
>>
>> * Relative directory in Makefile fragments:
>>
>>   - The special Automake-time substitutions '{RELDIR}' and '{CANON_RELDIR}'
>>     (and their abbreviated versions, '{D}' and '{C}' respectively) can now
>>     be used in an included Makefile fragment.  They are substituted with
>>     the relative directory of the included fragment, or its canonicalized
>>     version, compared to the top level including Makefile:
>>
> Yes, better.  But I find the following even better (marginally):
>
>     The special Automake-time substitutions '{RELDIR}' and '{CANON_RELDIR}'
>     (and their abbreviated versions, '{D}' and '{C}' respectively) can now
>     be used in an included Makefile fragment.  They are substituted,
>     respectively, with the relative directory of the included fragment and
>     its canonicalized version, compared to the top level including Makefile:
>
> OK?

I still like my version better, I think that the slight ambiguity you aim
to kill with the extra "respectively" is not going to fool anyone, and I
find that the twist it adds just makes it harder to read the dang thing.
But I'm not a native speaker, so what do I know?

New (and final, from me) attempt:

* Relative directory in Makefile fragments:

  - The special Automake-time substitutions '{RELDIR}' and '{CANON_RELDIR}'
    (and their abbreviated versions, '{D}' and '{C}' respectively) can now
    be used in an included Makefile fragment.  The former is substituted
    with the relative directory of the included fragment (compared to the
    top level including Makefile), and the latter with the canonicalized
    version of the same relative directory:

Cheers,
Peter




12345