Re: [PATCH] tests: add AM_PROG_AR to help losing archivers

[Peter Rosin]

Stefano Lattarini skrev 2012-01-31 22:23:
> On 01/31/2012 09:30 PM, Peter Rosin wrote:
>> Stefano Lattarini skrev 2012-01-31 14:31:
>>> On 01/31/2012 01:55 PM, Peter Rosin wrote:
>>> (I know, the present organization of branches sucks in some respects;
>>> we might rethink it after the 1.11.3 release, OK?)
>>
>> I would love to send maint, msvc and branch-1.11 to some dump
>> somewhere far away with a one-way ticket.
>>
> Oh no!  maint and branch-1.11 are were we cut the 1.11.x releases from,
> so until 1.12 is out, they are here to stay.  Sorry.

Of course, that was just my way of saying that I'd like 1.12 to already
be out...

> In retrospective, though, it should have been better to dispense with
> branch-1.11 and simply use maint as base for the 1.11.x releases (that
> would have allowed us to have the 'msvc' branch dropped at this point,
> since it's already merged in the maintenance branch). The only potential
> problem with that approach is: how do we deal with "divergent" changes in
> maint and master then?  (As a concrete example of such a situation, think
> of the 'extra-portability' warnings, that are enabled by '-Wall' in
> master *but* not in the 1.11.x series).  Luckily, after some experience
> with git, I think I have a solution for this problem.
> 
> If you need a divergent change for master ad maint (which is a very rare
> occurrence anyway), proceed as follows:
> 
>   1. merge maint into master;
>   2. create a temporary branch out of maint (say "tmp-branch");
>   3. implement in 'tmp-branch' the behaviour you want for 'maint';
>   4. merge 'tmp-branch' into 'maint';
>   5. change the implementation in 'tmp-branch' to match the behaviour
>      you want for 'master';
>   5. merge 'tmp-branch' into 'master';
>   6. merge 'maint' into 'master'; if we have done things right, this
>      should cause *no* merge conflicts;
>   7. If 'tmp-branch' is not meant to be a permanent topic branch,
>      delete it.
> 
> I'll try this approach after 1.11.3, so that we move 'branch-1.11' to
> be the new 'maint', keep it merged into 'master', and delete the (by
> then obsolete) 'msvc' branch.  WDYT?

Your solution don't really work for permanent topic branches since
it is not possible to merge the topic branch into maint after it has
been updated to suit master.  I.e, the delete part of point 7 is
for all practical purposes required.

This is the problem with the msvc branch, it's code that's intended
for master and not suitable for maint.  So, in order to merge msvc
into maint, you'd have to modify the code to suit maint, but then
you'd have to undo that when you want to merge maint into master
again.  But that will give you "oscillation" if you look at the
history of master, and could potentially confuse bisections etc.
It's a mess.

I suspect that most aren't as hindered by this state of affairs as
I am, as maint is just broken for me.  I have to merge maint into
branch-1.11 and test changes there instead.  Not a big deal if you
have to do it once or twice, but it gets cumbersome.  And it's
confusing.

On the "drop maint" line of discussion, I don't think that's a wise
move.  If you drop maint - and release directly from branch-1.11 -
you'd "leak" e.g. the version change in configure.ac into master the
next time you merge branch-1.11 into master, and you don't want
that.  So, instead you'd create a release branch off of branch-1.11
and do the version change there, but then you'd want to base the
next release off of the old release.  And by then you are back to
square one with the future branch-1.11 being the equivalent of the
current maint.

I think the rule should be that maint should be kept *very* close
to branch-1.11 (only differing by release related commits such as the
above example with the version in configure.ac).  This isn't true
today (e.g. msvc is merged into branch-1.11).

>> They have simply diverged too much.  At least for me, I'm only somewhat up
>> to speed on the ar-lib mess, but you might be in a different position having
>> written most (all?) of the other diverging changes.
>>
>>>> Subject: [PATCH] tests: add AM_PROG_AR to help losing archivers
>>>>
>>>> * tests/extradep.test (configure.in): Add AM_PROG_AR.
>>>> * tests/extradep2.test (configure.in): Likewise.
>>>>
>>> By only reading this, I'm not sure whether the change is needed to pacify
>>> some automake warning, to improve coverage, or to make the tests runnable
>>> with Microsoft lib as the archiver.  Could you add a paragraph after the
>>> summary line that explicitly specifies what is the patch motivation?
>>>
>>> ACK with that addressed.
>>
>> Turns out these AM_PROG_ARs are already on master.  So, the merge from
>> maint into msvc should probably include them with a manual adjustment.
>> I tried doing the merges you describe above, but it's too rich for my
>> stomach.  I get something that works, sort of, by I don't feel good
>> about it and I have this feeling that some changes leaked into branch-1.11
>> that are not supposed to be there.  I simply don't feel qualified and
>> can't assess if my conflict resolutions are good or bad without further
>> digging.  Which I don't have time for, so I'm leaving those merges for
>> someone else<tm>.  Sorry.
>>
> Given the plan outlined above, I say you can just apply your changes
> to branch-1.11.  I will handle the post-1.11.3 branches reorganization
> myself, so you should not worry about "merging pains" :-)
> 
>> Regarding the requested extra paragraph in the commit message, is that
>> really needed?
>>
> Not *really* needed for such a simple change -- but still highly
> preferred.
> 
>> I think it would be quite a bit of extra work to get an
>> accurate description of the various failure cases,
>>
> Oh, but I didn't ask for the accurate description of the failures --
> only for their "kind".  Let me rephrase what I said:
> 
>  - "to improve coverage"
>   This means no real failure; you only made the changes to ensure better
>   coverage when you run the testsuite with MSVC.
> 
>  - "to make the tests runnable with Microsoft lib as the archiver"
>   The tests failed when you run them with Microsoft lib being the
>   archiver; you patch fixes this failure.  So the change is not just
>   an improvement, but a real bug fix (albeit for a testsuite bug).
> 
>  - "the change is needed to pacify some automake warning"
>   This means a failure happening at automake time (maybe only on MSYS),
>   but even *without Microsoft lib involved*.  This too is a real bug
>   fix (albeit again, for a testsuite bug).
> 
>> as I haven't kept the test suite results and would need to rerun the
>> tests with/without patches and with varying compiler settings etc.
>> It's a trivial bug, and as I said, the code is already there on master.
>>
> Then it's acceptable to dispense with the "explanatory paragraph" in
> this case.  Still, if in future you manage keep track of the details
> of a bug before fixing it, and report such details in the commit
> message, you'll make an happy man :-)

Oh f/\(|<, I did a "full check" on branch-1.11.

If I run with AR="/home/peda/automake/lib/ar-lib lib" in the environment
PASS: extradep  (but uses ar as the archiver)
PASS: extradep2 (libtool AC_SUBSTs AR, not needing AM_PROG_AR to do that)

If I also move ar out of the way (why would I need it when AR is pointing
elsewhere?)
FAIL: extradep  (ar is not present)
PASS: extradep2

If I run with AR=lib in the environment
PASS: extradep  (but uses ar as the archiver)
FAIL: extradep2 ("lib cru" doesn't work)

If I also move ar out of the way (why would I need it when AR is pointing
elsewhere?)
FAIL: extradep  (ar is not present)
FAIL: extradep2 ("lib cru" doesn't work)

No AR in the environment and ar moved out of the way
FAIL: extradep  (ar is not present)
FAIL: extradep2 (ar is not present)

With the patch, all 10 cases above PASS and ar is never used even if
present.  So, I'd say we are mainly in your second point making the
tests runnable with MS lib and thus fixing testsuite bugs.

I pushed the patch to branch-1.11 with this updated commit message:

        tests: add AM_PROG_AR to help losing archivers

        Without AM_PROG_AR, using Microsoft lib as the archiver causes
        testsuite failures.

        * tests/extradep.test (configure.in): Add AM_PROG_AR.
        * tests/extradep2.test (configure.in): Likewise.

Cheers,
Peter