Re: [PATCH] [cygwin|mingw] fix dlpreopen with --disable-static (take 8?)
Hi Chuck,
On 3 Jul 2010, at 22:34, Charles Wilson wrote:
> It's non-timely and off-point "reviews" that I tire of.
>
> The non-timely bit is just a reflection of the manpower and free time
> issues that all open source projects are subject to, so that kinda goes
> with the territory. Nobody likes it, but...you just gotta live with it.
>
> By "off-point" I mean discussing non-germane or wishlist items as part
> of a review. If the reviewer isn't VERY careful, such off-topic dicta
> can appear to imply that acceptance of a particular patch is predicated
> on solving longstanding wishlist items or software design misfeatures
> that long predate the patch in question.
>
> Most potential contributors -- myself included -- are scratching their
> own itch: X doesn't work, and I want to fix it. It's discouraging to be
> told (or THINK you are told) "we won't fix X until you or somebody else
> fixes huge, gaping, gargantuan problems Y and Z. Those problems are
> really hard to solve, and have existed for years. They are SO hard that
> none of US experts have even tried to tackle it. BUT...we won't accept
> your patch for X until somebody steps up to the plate for Y and Z"
>
> Which...sounds a whole lot like "We appreciate the submission of your
> manuscript "The Life and Times of an New York Meter Maid". However, at
> this time there are no opportunities for additional entries in our New
> York True Life book series. Thank you for your interest in Bob's
> Publishing Company, and Keep Writing!"
>
> Unless the contributor of patch X goes off to scratch YOUR itch
> regarding Y and Z. That's not the way free software contributors are
> best motivated.
>
> On 6/28/2010 3:23 AM, Gary V. Vaughan wrote:
>> Nevertheless, please do remember that it is a *review*, and if you find
>> yourself disagreeing with something (excepting an outright veto of course),
>> Ralf and I are both acutely aware that you are the one doing the work on
>> these patches and the last thing we want to do is retard the progress of
>> Libtool on Windows, so please don't be afraid to say "on balance, I'd
>> rather see this patch move Libtool forward in some small way without
>> addressing issue X right now". Often, we'll concede in exchange for a
>> TODO item! :)
>
> That's an...interesting take. I've never assumed that ANY contribution
> would be acceptable unless it received an actual approval by a
> maintainer. I mean, really: here's this patch, and no single maintainer
> has endorsed it without some significant objection -- and I should feel
> free as a non-maintainer to say "well, I disagree, so I'm pushing anyway"??
Just to clarify, that isn't what I meant -- rather, "well, I disagree, and
as you can see, despite its faults, patch X alone already makes things
less bad than they were previously. Is it okay if I push X in its current
state, and then tackle Y later? I've no inclination to work on Z, but
I'll put a reference to this thread in libtool/TODO so it isn't entirely
forgotten."
And I believe that we'll often say "sure", or "good point, go ahead".
> Maybe you mean "after a few more rounds of negotiation on the mailing
> list, maintainers may acquiesce with reservation to an un-revised
> version of particular patch"...
That too.
>> On 28 Jun 2010, at 13:10, Ralf Wildenhues wrote:
>>> * Charles Wilson wrote on Mon, Jun 28, 2010 at 12:05:40AM CEST:
On 6/27/2010 4:43 PM, Ralf Wildenhues wrote:
> I don't see this method as the new method of choice. We already have a
> mechanism for years to transport values to the libtool script with
> _LT_DECLs and _LT_TAGDECLs, and at least for small code snippets,
Yeah, that's the problem.
>>
>> I wrote _LT_DECL and _LT_TAGDECL to propagate shell variable declarations to
>> the libtool script, and I fear it will behave badly if we try to use that
>> mechanism for shoehorning anything else through, especially because it works
>> by doing a *lot* of booking-keeping at m4 time.
>
> That's what I thought. Windows postinstall_cmds is pretty much the
> outer limit, IMO:
>
> postinstall_cmds="base_file=\\\`basename \\\${file}\\\`~
> dlpath=\\\`\$SHELL 2>&1 -c '. \$dir/'\\\${base_file}'i; echo
> \\\$dlname'\\\`~
> dldir=\$destdir/\\\`dirname \\\$dlpath\\\`~
> test -d \\\$dldir || mkdir -p \\\$dldir~
> \$install_prog \$dir/\$dlname \\\$dldir/\$dlname~
> chmod a+x \\\$dldir/\$dlname~
> if test -n '\$stripme' && test -n '\$striplib'; then
>eval '\$striplib \\\$dldir/\$dlname' || exit \\\$?;
> fi"
Agreed.
>> _LT_PROG_XSI_REPLACE is designed for swapping out fallback implementations
>> of full functions (suitably decorated) for more efficient implementations
>> based on the build-time environment. I think that is exactly what you're
>> trying to do, but it seems to me like you might be able to work more
>> effectively in reverse: by putting the full Windows required implementations
>> into ltmain.m4sh, and using _LT_PROG_XSI
MSVC: Make export.at pass
Hi!
I'd like to make the "Export test" work on MSVC.
Therefore I'm asking if I can cherry-pick commit
89a3cdc7a57314fb3ca8f57bf47afd20809e5608
"* tests/export.at [MSVC]: dllimport all imported variables."
into master (the patch is also attached).
Previous discussion:
http://lists.gnu.org/archive/html/libtool-patches/2008-08/msg00021.html
Cheers,
Peter
commit 89a3cdc7a57314fb3ca8f57bf47afd20809e5608
Author: Peter Rosin
Date: Sat Aug 9 22:48:50 2008 +0200
* tests/export.at [MSVC]: dllimport all imported variables.
Signed-off-by: Ralf Wildenhues
diff --git a/ChangeLog b/ChangeLog
index 537fdb5..e4e5484 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2008-08-09 Peter Rosin
+
+ * tests/export.at [MSVC]: dllimport all imported variables.
+
2008-08-02 Peter Rosin
Add MSVC support.
diff --git a/tests/export.at b/tests/export.at
index a330d40..39adfbc 100644
--- a/tests/export.at
+++ b/tests/export.at
@@ -93,32 +93,48 @@ v15
AT_DATA(main.c,
[[
+/* w32 fun. With GCC, you can have auto-import, which will work for
+ * functions and non-const variables. With MSVC, you have to explicitly
+ * import all variables. Depending on the amount of portability, you
+ * need these annotations: LIBA_SCOPE for GCC, and also LIBA_SCOPE_VAR
+ * for MSVC. Of course you can merge both to one, this test only avoids
+ * that in order to expose the auto-import feature on Cygwin.
+ *
+ * For users, it's best to realize that they should not provide any
+ * non-function API at all.
+ */
#if defined(LIBA_DLL_IMPORT)
# if defined(_WIN32) || defined(WIN32) || defined(__CYGWIN__)
#define LIBA_SCOPE extern __declspec(dllimport)
+#if defined(_MSC_VER)
+# define LIBA_SCOPE_VAR LIBA_SCOPE
+#endif
# endif
#endif
#if !defined(LIBA_SCOPE)
# define LIBA_SCOPE extern
#endif
+#if !defined(LIBA_SCOPE_VAR)
+# define LIBA_SCOPE_VAR extern
+#endif
#ifdef __cplusplus
extern "C" {
#endif
-extern int v1;
-extern int v3, v4;
+LIBA_SCOPE_VAR int v1;
+LIBA_SCOPE_VAR int v3, v4;
LIBA_SCOPE const int v5, v6;
-extern const char* v7;
-extern const char v8[];
+LIBA_SCOPE_VAR const char* v7;
+LIBA_SCOPE_VAR const char v8[];
extern int v9(void);
-extern int (*v10) (void);
-extern int (*v11) (void);
+LIBA_SCOPE_VAR int (*v10) (void);
+LIBA_SCOPE_VAR int (*v11) (void);
LIBA_SCOPE int (*const v12) (void);
#ifdef __cplusplus
}
#endif
typedef struct { int arr[1000]; } large;
-extern large v13, v14, v15;
+LIBA_SCOPE_VAR large v13, v14, v15;
int main (void)
{
Re: MSVC: Fix ccache test for MSVC.
Hi Ralf, Den 2010-07-04 21:51 skrev Ralf Wildenhues: Done, thanks for the reminder. Thanks! I could do it myself but I don't know what to do about the year 2008 that would appear in the ChangeLog, so I'm leaving it in your capable hands. Thanks! Well, git cherry-pick ...&& $EDITOR ChangeLog&& git commit -avs --amend easy! Well, I didn't really need help with git mechanics (this time), the question was (again) what the desired content of ChangeLog is when facing ancient commits. I guess you answered that question with your actions though :-) Cheers, Peter
Re: [SCM] GNU Libtool branch, master, updated. v2.2.10-49-gc13532a
Den 2010-07-04 20:26 skrev Ralf Wildenhues: Oh well. So, is anyone of you working on a testsuite addition, so I'd be doing double work? I'm not, I haven't even looked at what the code does. I was only looking through the patch, hunting for "that other regression" when I found this hunk which looked suspicious and asked a quick question. Then Gary answered with "OK to push", so I figured I'd have to write a patch first. It seemed so simple and I had Gary behind me so I didn't even run a test. But it seems nothing is ever as simple as it looks... Cheers, Peter
Re: [SCM] GNU Libtool branch, master, updated. v2.2.10-49-gc13532a
On 5 Jul 2010, at 12:36, Gary V. Vaughan wrote: > On Jul 5, 2010, at 1:26 AM, Ralf Wildenhues wrote: >> So, is anyone of you working on a testsuite addition, so I'd >> be doing double work? > > I plan to look at it eventually, and have it on my todo list to try and make > a teat ^Wtest *blush* > that covers that part of the code... But I haven't even looked at the bogus > lines to analyse what is required to reach them with an appropriate test. > Likely I won't get to it for at least a week, so if you see what is needed > already, or want to work on the test case and fix, that is perfectly fine by > me :-) Cheers, -- Gary V. Vaughan (g...@gnu.org)
Re: [SCM] GNU Libtool branch, master, updated. v2.2.10-49-gc13532a
Hallo Ralf, On Jul 5, 2010, at 1:26 AM, Ralf Wildenhues wrote: > Hi Gary, Peter, > > * Gary V. Vaughan wrote on Sat, Jul 03, 2010 at 10:23:12AM CEST: >> On 3 Jul 2010, at 15:15, Peter Rosin wrote: >>> Fix typo in "Add func_append_quoted and do..." >>> >>> * libltdl/config/ltmain.m4sh (func_mode_compile): Use >>> func_append_quoted instead of func_append. > [...] >> In an ideal world, we'd have test coverage of this part of the >> code, > > at least after the bug was found and fixed ;-) > >> and I would never have made the bogus commit in the >> first place :( > > Oh well. So, is anyone of you working on a testsuite addition, so I'd > be doing double work? I plan to look at it eventually, and have it on my todo list to try and make a teat that covers that part of the code... But I haven't even looked at the bogus lines to analyse what is required to reach them with an appropriate test. Likely I won't get to it for at least a week, so if you see what is needed already, or want to work on the test case and fix, that is perfectly fine by me :-) Cheers, -- Gary V. Vaughan (g...@gnu.org)
*_cmds file name expansion bug (was: MSVC: For MSVC, embed the manifest as a resource in the executable.)
Hi Peter, * Peter Rosin wrote on Thu, Jul 01, 2010 at 10:30:56PM CEST: > I.e. *.exe get expanded. That doesn't happen with "*.[eE][xX][eE])": > [...] > This all seems awfully fragile. I don't know how to quote > this expression in libtool.m4 and need help. Someone please? Ouch. Our expansion code is borked. It's ok that you have a workaround now, but that means we need to fix this. I guess that probably means testing for and setting noglob when the shell supports it. Thanks for the report, Ralf
Re: MSVC: Fix ccache test for MSVC.
Hi Peter, * Peter Rosin wrote on Wed, Jun 30, 2010 at 09:10:02PM CEST: > Can you please cherry-pick this commit of yours into master? > "Fix ccache test for MSVC." > 563c2406deb4d5ed0ebf5faf5c8b798d52ee6ac2 Done, thanks for the reminder. > I could do it myself but I don't know what to do about the > year 2008 that would appear in the ChangeLog, so I'm leaving > it in your capable hands. Thanks! Well, git cherry-pick ... && $EDITOR ChangeLog && git commit -avs --amend easy! > The patch was previously mentioned here: > http://lists.gnu.org/archive/html/libtool-patches/2008-08/msg00046.html Cheers, Ralf
Re: [PATCH] [cygwin|mingw] fix dlpreopen with --disable-static (take 8?)
Hi Charles, * Charles Wilson wrote on Sat, Jul 03, 2010 at 06:10:57PM CEST: > On 6/28/2010 2:10 AM, Ralf Wildenhues wrote: > > * Charles Wilson wrote on Mon, Jun 28, 2010 at 12:05:40AM CEST: > >> It obviously isn't SUPPOSED to be dead -- or it wouldn't be there. > > > > Well, I wouldn't put my money on that reasoning. > > . 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. OK, thanks. > >> 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. No hard feelings at all on my side. Except that I do have somewhat of a bad conscience for letting all the w32 stuff go on for so long. Let's hope we can improve that in the future, too. > 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). Yep. I agree that a bigger cleanup could help here, and I agree that it's better to tackle that as an orthogonal issue. Maybe in the end a different structuring will even be easier once all the w32 stuff works, because then we can maybe see the bigger picture. > >> 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. OK. It is certainly helpful for review if you mention this with some code that could not end up in libtool.m4 that way. I realize I'm violently agreeing with you here as well, because your patch postings are usually very detailedly explained, as to which approaches you took and why and why not. > > 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. Yep. > 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
Re: [PATCH] [cygwin|mingw] fix dlpreopen with --disable-static (take 7)
* Charles Wilson wrote on Sun, Jul 04, 2010 at 06:15:06AM CEST: > On 6/26/2010 2:51 PM, Ralf Wildenhues wrote: > > 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. FWIW, I'm likely > > not available most of next week; if we find issues later, then I guess > > they'll just have to be fixed afterwards. > > As discussed previously, this version fixes (almost) all of the noted > issues. I didn't change the eval stuff, deferring instead to Ralf's > promised patch to take care of all of that at once. Thanks for the re-do. > 112: override pic_flag at configure time FAILED (pic_flag.at:48) I will fix this failure. The test has a few issues, also on other systems. > 112 appears to be a new test, and is ELF specific (-fpic/-fPIC has no > meaning on cygwin). It probably should be skipped. > > > So...this is what I intend to push, barring objections. No objections from me, please wait however long I asked you to wait (basically long enough so others have had a chance to chime in). Thanks, Ralf
Re: [SCM] GNU Libtool branch, master, updated. v2.2.10-49-gc13532a
[ moving from -commit to -patches, I think the former was for commits only; maybe it should have reply-to set? ] Hi Gary, Peter, * Gary V. Vaughan wrote on Sat, Jul 03, 2010 at 10:23:12AM CEST: > On 3 Jul 2010, at 15:15, Peter Rosin wrote: > >Fix typo in "Add func_append_quoted and do..." > > > >* libltdl/config/ltmain.m4sh (func_mode_compile): Use > >func_append_quoted instead of func_append. [...] > In an ideal world, we'd have test coverage of this part of the > code, at least after the bug was found and fixed ;-) > and I would never have made the bogus commit in the > first place :( Oh well. So, is anyone of you working on a testsuite addition, so I'd be doing double work? Thanks, Ralf
Re: [PATCH] [cygwin|mingw] fix dlpreopen with --disable-static (take 8?)
On Sat, 3 Jul 2010, Charles Wilson wrote: No one is threatening your committer status. I didn't say they were. But if I *did* misbehave -- well, I could hardly be surprised by the inevitable consequences, could I? Doesn't take a genius to predict those consequences, either. Misbehavior is characterized by intent and subsequent acceptance of responsibility. If someone commits a change which de-stabilizes the software, then they should expect to take responsibility for the clean-up. Likewise, committing a change which will knowingly de-stabilize the software is harmful, unless there is a plan made in advance for how the issue will soon be rectified. Sometimes temporary instability is ok because the end justifies the means. Bob -- Bob Friesenhahn bfrie...@simple.dallas.tx.us, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer,http://www.GraphicsMagick.org/
