Re: [PATCH] {maint} remake: behave better with non-GNU make in subdirectories

[Peter Rosin]

Den 2011-05-30 22:22 skrev Stefano Lattarini:
> On Sunday 29 May 2011, Peter Rosin wrote:
>> Den 2011-05-29 19:32 skrev Stefano Lattarini:
>>> Hi Peter, thanks for the review.
>>
>> I wouldn't call it review, it was more of a knee-jerk reaction
>>
> Well, I still appreciate it anyway.
> 
>> (and I haven't even read the actual patch).
>>
> There's still time to remedy that, right?  (Just kidding ;-)

There is just no way I'm going to keep up with your seemingly endless
string of patches. :-)

>>> On Sunday 29 May 2011, Peter Rosin wrote:
>>>> Den 2011-05-29 16:26 skrev Stefano Lattarini:
>>>>> Currently, every decent non-GNU make program makes it possible
>>>>> to remake out-of-date autotools-generated files with a simple
>>>>> "make Makefile" issued from the top-level directory.  And while
>>>>> having to run "make Makefile" by hand isn't as convenient as
>>>>> the GNU make feature of "automatic remake *and reloading* of
>>>>> out-of-date makefiles", it is usually good enough and definitely
>>>>> worth having.
>>>>>
>>>>> Unfortunately, a silly limitation in the current implementation
>>>>> of remake rules prevents this idiom from working when it's used
>>>>> outside the top-level directory.  Luckily, this limitation is
>>>>> easy to   remove, and that's what this patch does.
>>>>
>>>> Nitpick: The tense is wrong for a ChangeLog entry, and there's a tab
>>>> in there as well.  Suggestion:
>>>>
>>>>    Remove silly limitation that prevents this idiom from
>>>>    working outside the top-level directory.
>>>>
>>>> I'm not sure if the essay style of the first paragraph should stay
>>>> or go though?
>>>>
>>> I mostly agree with your objections.  What do you think about this
>>> ChangeLog entry instead?
>>
>> Hmmm, it just feels like you have identified some problem, and then
>> you proceed to writing a patch.  Then you write some words to
>> tell the world about the problem and the solution and put it all in
>> the ChangeLog.
>>
> Actually, I did the other way around (mostly: identify problem, write
> ChangeLog entry, fix problem, tweak ChangeLog entry if needed).  While
> this might sometimes lead to overly longish ChangeLog entries, IMVHO it
> also usually leads to better patches, as it forces me to express clearly
> and unambiguously what I'm trying to do, and more importantly, why.
> 
>> It feels backwards to have the *original* problem description in
>> the ChangeLog, but I'm no longer sure if it's really all that bad
>> or if it just feels backwards _to_me_.
>>
> Do you have explicit, specifical objections to the ChangeLog change,
> or suggestions about how to improve it?  I'm prepared to listen to
> them, and tweak the ChangeLog entry accordingly.
> 
>> And regarding the tense, now that I think of it that's probably only
>> the rule for the actual changes in the asterisk lines, not the "header",
>> so that was probably off base too.  Not sure though.
>>
> In effect, using the present tense to describe a just fixed, and thus
> past, problem, is suboptimal and might be confusing.  So what about
> this updated ChangeLog entry?
> 
>     remake: behave better with non-GNU make in subdirectories
>     Currently, with every decent make program, it is possible to
>     rebuild out-of-date autotools-generated files with a simple
>     "make Makefile".  But for this to work reliably with non-GNU
>     make implementations, that command had be issued from the
>     top-level directory.  This patch has removed such limitation.
>     * lib/am/configure.am (am--refresh): Depend on `%MAKEFILE%'.
>     * tests/defs.in (using_gmake): New function, backported from the
>     `master' branch (and simplified).
>     * tests/remake-subdir.test: New test.
>     * tests/remake-subdir2.test: Likewise.
>     * tests/remake-subdir-gnu.test: Likewise.
>     * tests/remake-subdir-from-subdir.test: Likewise.
>     * tests/Makefile.am (TESTS): Update.
> 
>> Me feels bad, sorry for the interruption.  I lost the confidence
>> to voice my opinion...
>>
> Why?  I think that the modified ChangeLog entries prompted by your
> answer are already an improvement over my original one; so I wouldn't
> qualify your intervention as useless, or as an "interruption".

Ok, new attempt at criticism. Sigh. Here we go.

I think it is a bit awkward to talk about "this patch" in the patch
description, as it is obvious that the text refers to this patch. I also
think "currently" is not obviously the state *without* the patch, you
can also spot that there are problems with that when you continue
to refer to how it was before the patch in past tense ("that command
_had_ been issued") even though you started out with describing the state
before the patch as the current state. One would think that the current
state deserves present tense, but present tense is reserved for the
changes themselves.

It quickly becomes a maze when you try to talk about both the state
before the change, the change itself and the state after the change.
So you have to be careful.

My attempt follows:

        remake: behave better with non-GNU make in subdirectories.
        With a decent make program, it is possible to rebuild
        out-of-date autotools-generated files with a simple "make
        Makefile".  Remove the limitation that "make Makefile" has
        to be issued from the top-level directory with non-GNU
        make implementations.
        * lib/am/configure.am (am--refresh): Depend on `%MAKEFILE%'.
        * tests/defs.in (using_gmake): New function, backported from the
        `master' branch (and simplified).
        * tests/remake-subdir.test: New test.
        * tests/remake-subdir2.test: Likewise.
        * tests/remake-subdir-gnu.test: Likewise.
        * tests/remake-subdir-from-subdir.test: Likewise.
        * tests/Makefile.am (TESTS): Update.

Now, I haven't looked into the patch so I'm not sure *which* Makefile
"make Makefile" regenerates if run from a subdir. The top-level one (my
blind guess) or the one in the current directory (which might be more
natural)?

Cheers,
Peter