Re: [PATCH] [cygwin|mingw] fix dlpreopen with --disable-static (take 8?)

[Charles Wilson]

On 6/28/2010 2:10 AM, Ralf Wildenhues wrote:
> * Charles Wilson wrote on Mon, Jun 28, 2010 at 12:05:40AM CEST:
>> So...we APPEAR to have a bunch of dead code.
> 
> I wasn't aware of that.  Sorry about the sloppy review.
> 
>>  It obviously isn't
>> SUPPOSED to be dead -- or it wouldn't be there.
> 
> Well, I wouldn't put my money on that reasoning.

<g>.  Except that I do recall that back-in-the-day, this code WAS
actually used.  That's why I had to change it as I did, before your
suggestion in Jan 2009 to use the save-.la-name-in-a-custom-variable
approach.

>> Which?
> 
> I'd say the part that is easier for you, so I guess that would be
> committing all the code, including presumably-dead.  And maybe in a
> future patch adding a testsuite test that exercises the code.

I'll provide a test in the future that exercises building a dlpreopen
app against an 'installed' library for which there is no .la file.  It
won't actually exercise the dead code -- until it is made alive again.
So...the test will probably XFAIL.

But we'll approach that separately.

As for THIS patch, I'll revise version 7 to address your other comments,
post that as version "9" and then wait 72 hours, as:

http://lists.gnu.org/archive/html/libtool-patches/2010-06/msg00174.html
> OK.  Here's my take on this: if you fix all nits I noted inline below,
> post the updated and tested patch (you decide what testing is needed),
> then you are OK to commit after 72 hours of waiting.

>> I feel (more) discouraged now (than usual), having wasted so much time
>> addressing a criticism of a patch that wasn't meant to be taken seriously.
> 
> I would like to apologize for this comment making you do this extra
> work.  Again, that review of mine was more sloppy than it should have
> been.

Accepted (Although you didn't actually 'make' me do the extra work.
Your review did not actually REQUIRE it -- but your increasing
unhappiness with $host-specific code to ltmain.m4sh made it appear to me
that tackling it now -- before the cross-compile patch comes up for
review again -- was a good idea.  It wasn't.)

I apologize also for letting my frustration overtake my good sense. I
shouldn't have complained as...vociferously. You're just doing your job:
reviewing code to make sure libtool is as good as it can be.

>> In fact, I have often wondered if the reason many of my patches -- and
>> Peter's -- tend to languish so long is because of these aesthetic
>> objections
> 
> Of course code maintenance aspects and long-term slowdown of the code
> are a part of code quality.

As they should be.

I guess the issue is, the shared library model of PE/COFF is just so
different than ELF that the differences, to me, just don't seem to be
the kind of thing that can be handled by m4 code -- at least, given the
current architecture of the libtool script.  Now, if the ENTIRE body of
'libtool' were generated from libtool.m4, rather than the bulk of it
being presented in ltmain.m4sh...then maybe the "skeleton" could be more
platform-agnostic.

But, two things: (1) this means moving a LOT of what we probably
consider "generic" code into libtool.m4 (imagine what that m4 would have
to look like, to eliminate ALL case $host statements), and (2) you'd
basically end up with, effectively, two DIFFERENT scripts that each CALL
themselves "libtool".  The "ELF-ish" one would not look anything like
the "PE/COFF-ish" one.

Maybe that's the right thing to do...long term.

But that's a long-term project...I was just trying to fix a single
regression (that turned into a rabbit hole).

>> Anyway, if we're going to try and nail down these aspects of the API, I
>> think that's a good thing to do for libltdl2 (whether Gary's or some
>> other brainstorm).
> 
> Yep, I guess.

I guess that's what I'm getting at: I think some of this ugliness is
unavoidable given the major architectural differences between PE/COFF
and ELF -- and the EXISTING division of labor between libtool.m4 and
ltmain.m4sh.  "Fixing" it is going to require...*major* changes.

Given that...unless we plan to DO those major rewrites now...harping on
them with regards to w32 is counter-productive. Peter and I will
certainly try to put code into libtool.m4, but...it's not clear exactly
how successful it is possible for us to be, without beginning that major
rewrite process.

> Yes, the literal you tried is already split into words, so word
> splitting of the shell would not make any difference.

Ah.

> And yes, it may happen that I spend a large amount of time trying to
> review patches well.  I didn't spend so much time this time, i.e., I
> did glance at the 6 takes and discussion threads of this patch
> beforehand, but not in large detail.  You can't have both, good review
> and timely review. 

<g>

> I'm sorry if review is painful to accept, and I
> don't on purpose try to review in a way making you do double work.
> That that has happened now is bad, sorry about that.

No, I think you misunderstand. *Review* is good. Critical review is even
better.

But...the reason you found it so hard to review this patch -- I mean,
really, having to review six different discussions spread out over two
years? -- that's ridiculous! Who would expect THAT to happen quickly?
But how did we GET to that point: it was because in each of those
previous six attempts, the review process got stalled.

And that's the issue -- putting a hold on a patch or patch series with a
"I need to think about this"...and then not actually following up. I'm
not blameless: after a week or two of silence, I'd usually moved on to
something else, and it might be a while before I come back to "libtool".
 If *I* had kept up the "pressure" maybe we all would have been able to
keep the details of the various discussions in our primary memory bank,
AND resolved the issue(s) in just a month or two, rather than 25 or 30.

> I think one fundamental reason why the w32 stuff languishes so long is
> that neither of the maintainers are highly interested in w32.  That
> tends to get it a hit on priority wrt. other things, and since there are
> always many other things to do, that can be a problem.  Is that fair?

Actually, it is.  We all scratch our own itch; w32 doesn't give you guys
that itchy feeling.  That's life.

The other issue is that the regr. test process (and just bugfixing
process with its rebootstrap and reconf and rebuild) is just so
SLOOOOOOOOOOOW on cygwin and mingw. It's painful for ME and I put up
with it 'cause I care.  If you don't care as much, why put up with that?
(Granted, the regr. test process is better these days with some of the
changes last summer...but nothing is going to make emulated-posix-on-w32
very fast)

> No, it isn't.  But it happens nonetheless.  Does that mean we might be
> overly critical on w32 patches?  Maybe, for me I can't always reject
> that notion (backed by the GCS btw), esp. during times when I'm
> essentially the only person doing review.  That's nothing personal, but
> it turns out to be a problem nonetheless.  I reject the notion of being
> the only Libtool maintainer, and I'd rather step down than just open the
> floodgate for code that hasn't been properly reviewed or isn't in good
> shape.  Somebody else needs to take responsibility for that.

Oh, no, I don't think ANYBODY would ever say that ANY code -- much less
w32 code which, as discussed above, is invariably just plain ugly --
should be allowed with no review, or "well, it's w32 specific so we just
have to let it go".

> On the brighter side, haven't we made at least some good progress in the
> last few weeks?

Yes, quite a bit. I am pleased by that, as well as the progress made
getting Peter's stuff in.

--
Chuck