Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
Hi Peter,
On Oct 3, 2012, at 4:53 PM, Peter Rosin wrote:
> On 2012-10-03 05:45, Gary V. Vaughan wrote:
>> On Oct 3, 2012, at 12:32 AM, Peter Rosin wrote:
>>> I just fired up a test suite run...
>
> Good news! Clean runs on Cygwin, MSYS/MinGW and MSYS/MSVC.
Awesome.
I'll push the merge now, along with some other resurrected patches that
got stuck in the review queue last year.
> Now into nitpicking and other useless ramblings...
>
> The patch "libtool: unroll nested if into a single case statement" has
> whitespace issues (leading spaces instead of tabs on a few lines), and it
> should perhaps use a simple catch-all "*" (retaining the "fast_install is
> set to needless" comment) as the last case instead of "*,needless" that
> you have put there. But I guess it's harmless as $fast_install is not any
> random string, sans hacks, so no rereredo request from me, just a
> nitpick that I saw when reviewing the "libtool: ..." changes at the
> tip of gary/reredo-test-operand-order. Can always be cleaned up later,
> if someone has the energy.
Agreed on all counts. Having now split out all the unrelated patches has me
wondering how on earth all that extra crud got in there... especially since
every
review used to be like a few weeks on the rack.
> I also wonder about the relationship between "libtool: fold if into a
> compound OR statement when more readable" and the next commit "libtool:
> simplify multiple string tests". What I mean is that about half the
> tests from the later commit fit the pattern of the first, why did you
> for example not change
>
>if test yes,no = "$avoid_version,$need_version"; then
> major=
> versuffix=
> verstring=
>fi
>
> into
>
>test yes,no = "$avoid_version,$need_version" && {
> major=
> versuffix=
> verstring=
>}
>
> (or some such) when you where at it? (but to me it looks like churn)
Yep :( But, remember these changes are all to match the conflated original
changeset so that we end up with a useful diff showing errors in the original
that can be applied easily to the current tip of master. That said, your
comments are more than valid, and a lot of that stuff shouldn't really have
gotten in and/or should have been done consistently.
With my newly self appointed benevolent dictator hat on, and the commit
process being infinitely less arduous as a result, I hope to come back and
undo some of this damage prior to freezing for the next release.
> Anyway, to sum up, I'm ok with merging reredo branch into master.
> Lets put this to rest now, thanks!
And thanks to you too :)
Cheers,
--
Gary V. Vaughan (gary AT gnu DOT org)
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
On 2012-10-03 05:45, Gary V. Vaughan wrote:
> Hi Peter,
>
> On Oct 3, 2012, at 12:32 AM, Peter Rosin wrote:
>> [snipped loads of stuff]
>
> [snipped a bit more stuff]
>
>> You're saying that you are about to:
>>
>> $ git checkout master
>> $ git merge --strategy=recursive -X theirs gary/reredo-test-operand-order \
>> -m 'bla bla bla'
>>
>> right?
>
> Pretty much. But that's just to show the merge in git history, the end
> effect will be identical to having committed the diff back to master.
>
>> Is that better than committing the diff between reredo-... and master
>> as a revert-light? Why else would you have gone through all the trouble
>> of making all the smaller commits in the first place?
>
> It's better to show the branch merged in for future archaeology; the less
> time anyone else has to waste staring at 962aa91, the better a place the
> world will be. And I'm all about making the world a better place! :-)
>
>> I just fired up a test suite run...
Good news! Clean runs on Cygwin, MSYS/MinGW and MSYS/MSVC.
> Thanks! I'm crossing my fingers -- I'll sleep a lot better when we've put
> this one behind us.
Now into nitpicking and other useless ramblings...
The patch "libtool: unroll nested if into a single case statement" has
whitespace issues (leading spaces instead of tabs on a few lines), and it
should perhaps use a simple catch-all "*" (retaining the "fast_install is
set to needless" comment) as the last case instead of "*,needless" that
you have put there. But I guess it's harmless as $fast_install is not any
random string, sans hacks, so no rereredo request from me, just a
nitpick that I saw when reviewing the "libtool: ..." changes at the
tip of gary/reredo-test-operand-order. Can always be cleaned up later,
if someone has the energy.
I also wonder about the relationship between "libtool: fold if into a
compound OR statement when more readable" and the next commit "libtool:
simplify multiple string tests". What I mean is that about half the
tests from the later commit fit the pattern of the first, why did you
for example not change
if test yes,no = "$avoid_version,$need_version"; then
major=
versuffix=
verstring=
fi
into
test yes,no = "$avoid_version,$need_version" && {
major=
versuffix=
verstring=
}
(or some such) when you where at it? (but to me it looks like churn)
Anyway, to sum up, I'm ok with merging reredo branch into master.
Lets put this to rest now, thanks!
Cheers,
Peter
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
Hi Peter, On Oct 3, 2012, at 12:32 AM, Peter Rosin wrote: > [snipped loads of stuff] [snipped a bit more stuff] > You're saying that you are about to: > > $ git checkout master > $ git merge --strategy=recursive -X theirs gary/reredo-test-operand-order \ > -m 'bla bla bla' > > right? Pretty much. But that's just to show the merge in git history, the end effect will be identical to having committed the diff back to master. > Is that better than committing the diff between reredo-... and master > as a revert-light? Why else would you have gone through all the trouble > of making all the smaller commits in the first place? It's better to show the branch merged in for future archaeology; the less time anyone else has to waste staring at 962aa91, the better a place the world will be. And I'm all about making the world a better place! :-) > I just fired up a test suite run... Thanks! I'm crossing my fingers -- I'll sleep a lot better when we've put this one behind us. Cheers, -- Gary V. Vaughan (gary AT gnu DOT org)
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
[snipped loads of stuff] On 2012-10-02 16:54, Gary V. Vaughan wrote: > The new branch gary/reredo-test-operand-order (notice the double re) has > everything broken down into digestible chunks. All the differences between > that and master now look like improvements upon the original hand rolled > version made by my recent scripted revisions, or else outright errors in > the original corrected by the script. All the errors you flagged on the > list are corrected, as well as several others. That last bit makes me curious :-) > Assuming my running 'make dist' doesn't have any regressions compared to > master, and unless you have additional problems with Windows using the > gary/reredo-test-operand-order branch, I plan to merge the differences > back into master in a day or two. You're saying that you are about to: $ git checkout master $ git merge --strategy=recursive -X theirs gary/reredo-test-operand-order \ -m 'bla bla bla' right? Is that better than committing the diff between reredo-... and master as a revert-light? Why else would you have gone through all the trouble of making all the smaller commits in the first place? I just fired up a test suite run... Cheers, Peter [who is not as alert as he used to when reading diffs]
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
Hi Peter, Sorry for the delay, it was more complicated than I had imagined :-o On 28 Sep 2012, at 11:08, "Gary V. Vaughan" wrote: > On 27 ก.ย. 2012, at 5:03, Peter Rosin wrote: >> On 2012-09-26 14:57, Peter Rosin wrote: >>> On 2012-09-22 05:31, Gary V. Vaughan wrote: >> >> [Heavily snipped] >> >>> Why not commit the sc_prohibit_test_const_follows_var improvement >>> last, after fixing all violations? >> >> That doesn't make sense, sorry. But the idea would have worked >> initially, before the check first existed. I.e., write the check, >> fixup violations and commit in smaller digestible chunks, then >> finally commit the actual check preventing any new violations from >> creeping in. > > Even then, I'm on the fence, since that doesn't leave a revision to > demonstrate that the syntax-check is finding the violations... on the > other hand, we also don't want to push "broken" commits since that > obviously screws up git bisection. On reflection, I think I prefer to continue to commit a new syntax check rule along with all the fixes necessary to make it pass. Although, I'm horrified by how much unrelated crap crept into 962aa91, and will not be tackling any patches even close to this size by hand in future :( If not, I will branch before 962aa91, run the script, and then apply the rest of master to that branch one patch at a time until I arrive at a diff that I can apply to master itself, rather than using revert as I did on the current temporary branch. >>> >>> I have to say, given all these difficulties, is it really worth it? >>> Besides, I think >> >> Hmmm, I said that in the wrong context. Your plan above seems like the >> best path forward. I guess I was too "excited" about the bugs and didn't >> read properly, sorry again about my poor communication skills. > > No apologies necessary. I'm working on the revised redo, which will > result in a much smaller patch for master later today or tomorrow. It took me longer than anticipated due to being a lot more work after untangling all those unrelated parts that I've split out on the branch. The new branch gary/reredo-test-operand-order (notice the double re) has everything broken down into digestible chunks. All the differences between that and master now look like improvements upon the original hand rolled version made by my recent scripted revisions, or else outright errors in the original corrected by the script. All the errors you flagged on the list are corrected, as well as several others. Assuming my running 'make dist' doesn't have any regressions compared to master, and unless you have additional problems with Windows using the gary/reredo-test-operand-order branch, I plan to merge the differences back into master in a day or two. >> Some of the noise between master and redo-test-operand-order are a bunch >> of hunks with this pattern: >> - if test bar $op $foo; then >> + if test bar $op $foo ; then > > Nice catch. I already tweaked the script regexs to fix that once, but must > have missed one of the ways for that to happen :( The script was correctly keeping semi-colons with the same spacing as was present prior to running the script. The differences are because I fixed the errant spacing manually in the original patch. I'll write another syntax-check, and fix any remaining offenders after master is fully up to date again. >> BTW, here is another strange-looking hunk from >> "git diff master redo-test-operand-order" >> >> diff --git a/m4/libtool.m4 b/m4/libtool.m4 >> index 4413a6d..8ec9beb 100644 >> --- a/m4/libtool.m4 >> +++ b/m4/libtool.m4 >> @@ -5563,7 +5563,7 @@ _LT_EOF >> ;; >>esac >> >> -if test sni = "$host_vendor"; then >> +if test x$host_vendor = xsni; then >> case $host in >> sysv4 | sysv4.2uw2* | sysv4.3* | sysv5*) >> _LT_TAGVAR(export_dynamic_flag_spec, $1)='$wl-Blargedynsym' >> >> What has caused the difference in this hunk? Why hasn't the script >> caught this instance? And why isn't the syntax-check triggered? >> Are the "missing" quotes the key here? > > Not sure... investigating now. The lack of quotes on both sides was not picked up by my script nor by the syntax check, although I made the manual changes in the original patch. Once the branches are back in sync, I'll beef up the syntax check and fix any new violations it finds. Thanks again for keeping me honest with this one. Please let me know if you'd like me to wait more than a day or two to give you time to review. Cheers, -- Gary V. Vaughan (gary AT gnu DOT org)
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
Hi Peter, On 27 ก.ย. 2012, at 5:03, Peter Rosin wrote: > On 2012-09-26 14:57, Peter Rosin wrote: >> On 2012-09-22 05:31, Gary V. Vaughan wrote: > > [Heavily snipped] > >> Why not commit the sc_prohibit_test_const_follows_var improvement >> last, after fixing all violations? > > That doesn't make sense, sorry. But the idea would have worked > initially, before the check first existed. I.e., write the check, > fixup violations and commit in smaller digestible chunks, then > finally commit the actual check preventing any new violations from > creeping in. Even then, I'm on the fence, since that doesn't leave a revision to demonstrate that the syntax-check is finding the violations... on the other hand, we also don't want to push "broken" commits since that obviously screws up git bisection. >>> If not, I will branch before 962aa91, run the script, and then apply >>> the rest of master to that branch one patch at a time until I arrive >>> at a diff that I can apply to master itself, rather than using revert >>> as I did on the current temporary branch. >> >> I have to say, given all these difficulties, is it really worth it? >> Besides, I think > > Hmmm, I said that in the wrong context. Your plan above seems like the > best path forward. I guess I was too "excited" about the bugs and didn't > read properly, sorry again about my poor communication skills. No apologies necessary. I'm working on the revised redo, which will result in a much smaller patch for master later today or tomorrow. > Some of the noise between master and redo-test-operand-order are a bunch > of hunks with this pattern: > - if test bar $op $foo; then > + if test bar $op $foo ; then Nice catch. I already tweaked the script regexs to fix that once, but must have missed one of the ways for that to happen :( > You should perhaps add a commit to redo-test-operand-order which silences > that and makes other more substantial changes stand out more before you > proceed with the above plan? There are perhaps other harmless changes > that can be excluded from the "light" revert? Because who needs the > oscillation? Agreed. It was all these edge cases that made me abandon the scripted solution last year. I'm working on improving the script first to catch this and the other glitches you found. > BTW, here is another strange-looking hunk from > "git diff master redo-test-operand-order" > > diff --git a/m4/libtool.m4 b/m4/libtool.m4 > index 4413a6d..8ec9beb 100644 > --- a/m4/libtool.m4 > +++ b/m4/libtool.m4 > @@ -5563,7 +5563,7 @@ _LT_EOF > ;; > esac > > -if test sni = "$host_vendor"; then > +if test x$host_vendor = xsni; then > case $host in > sysv4 | sysv4.2uw2* | sysv4.3* | sysv5*) >_LT_TAGVAR(export_dynamic_flag_spec, $1)='$wl-Blargedynsym' > > What has caused the difference in this hunk? Why hasn't the script > caught this instance? And why isn't the syntax-check triggered? > Are the "missing" quotes the key here? Not sure... investigating now. > The check is named sc_prohibit_test_const_follows_var, so one > would guess that it should prohibit "test x$foo = xbar", no? Or are > you supposed to be able to dodge the check by needlessly adding x > in front of LHS $variables? Not at all. But the long list of substitutions must be interacting unexpectedly. The idea is for the syntax-check rule to flag any of: test x"$foo" = "xbar" test x"$foo" = xbar test "x$foo" = "xbar" test "x$foo" = xbar test x$foo = xbar where 'x' is one of '.', 'x', or 'X'. I think we're getting much closer to a working solution though. Many thanks for taking the time to review. Cheers, -- Gary V. Vaughan (gary AT gnu DOT org)
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
On 2012-09-26 14:57, Peter Rosin wrote: > On 2012-09-22 05:31, Gary V. Vaughan wrote: [Heavily snipped] > Why not commit the sc_prohibit_test_const_follows_var improvement > last, after fixing all violations? That doesn't make sense, sorry. But the idea would have worked initially, before the check first existed. I.e., write the check, fixup violations and commit in smaller digestible chunks, then finally commit the actual check preventing any new violations from creeping in. >> If not, I will branch before 962aa91, run the script, and then apply >> the rest of master to that branch one patch at a time until I arrive >> at a diff that I can apply to master itself, rather than using revert >> as I did on the current temporary branch. > > I have to say, given all these difficulties, is it really worth it? > Besides, I think Hmmm, I said that in the wrong context. Your plan above seems like the best path forward. I guess I was too "excited" about the bugs and didn't read properly, sorry again about my poor communication skills. Some of the noise between master and redo-test-operand-order are a bunch of hunks with this pattern: - if test bar $op $foo; then + if test bar $op $foo ; then You should perhaps add a commit to redo-test-operand-order which silences that and makes other more substantial changes stand out more before you proceed with the above plan? There are perhaps other harmless changes that can be excluded from the "light" revert? Because who needs the oscillation? BTW, here is another strange-looking hunk from "git diff master redo-test-operand-order" diff --git a/m4/libtool.m4 b/m4/libtool.m4 index 4413a6d..8ec9beb 100644 --- a/m4/libtool.m4 +++ b/m4/libtool.m4 @@ -5563,7 +5563,7 @@ _LT_EOF ;; esac -if test sni = "$host_vendor"; then +if test x$host_vendor = xsni; then case $host in sysv4 | sysv4.2uw2* | sysv4.3* | sysv5*) _LT_TAGVAR(export_dynamic_flag_spec, $1)='$wl-Blargedynsym' What has caused the difference in this hunk? Why hasn't the script caught this instance? And why isn't the syntax-check triggered? Are the "missing" quotes the key here? The check is named sc_prohibit_test_const_follows_var, so one would guess that it should prohibit "test x$foo = xbar", no? Or are you supposed to be able to dodge the check by needlessly adding x in front of LHS $variables? Cheers, Peter
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
On 2012-09-22 05:31, Gary V. Vaughan wrote:
> Hi Peter,
>
> On 19 Sep 2012, at 21:50, Peter Rosin wrote:
>
>> On 2012-09-19 16:20, Gary V. Vaughan wrote:
>>> Hi Peter,
>>>
>>> My bad, I'm embarrassed to say. I started to write a script to make the
>>> appropriate changes, but ended up doing it manually rather than adding
>>> more and more corner cases to the throwaway script... a poor choice in
>>> hindsight :-(
>>
>> It's easy to be wise after the fact... If you do redo it, may I suggest
>> breaking up the patch in smaller pieces for bisectability?
>
> I've pushed a temporary branch called gary/redo-test-operand-order to
> savannah with seven changesets that reverts the manual version of the
> buggy original and redoes it with a painstaking awk script (also checked
> in, for the morbidly curious).
>
> I'm on the fence about committing in smaller pieces... the policy for
> libtool has always been to make sure the testsuite passes (at least
> on the committer's machine) for every changeset, so that bisecting
> using one of the test cases doesn't fail unexpectedly on another
> commit that was intentionally pushed with know failures. On the other
> hand, the original was a monster, so I can see the benefits of splitting
> it up a bit too.
Why not commit the sc_prohibit_test_const_follows_var improvement
last, after fixing all violations?
BTW, the revert is also a monster, and I think the current split
is too coarse. It felt better with an autogenerated patch, but
that feeling disappeared when I reviewed the results, see below
for details. You still have hundreds of changes to fundamentally
different parts of the code in a single commit. It's impossible
(well, at least annoyingly time consuming) to find the offending
change if such a commit is found to cause a regression.
>>> I think it will be safer to revert the broken patch, and then
>>> write a script to reapply those changes automatically as I
>>> should have done originally, and only then to merge the result
>>> back to head. If you hold off for a few days, I'll do that as
>>> penance for my sins when I get back to my computer.
>>
>> I'm not sure a revert of such a big patch is possible after
>> 50-odd more patches? But I was thinking revert too after
>> reading it for a while... Who knows what else hides in
>> there?
>
> Well, I wrote and applied the script, diffed the results, and
> the testsuite has no regressions for me. I get a weird failure
> in distcheck which tries to run distclean in _build/tests/cdemo
> aftec removing _build/tests/cdemo/Makefile, that I haven't had
> time to check against current master to see if it is a new regression
> caused by my patch.
>
>>> But please hold on to your test case to verify that I've made
>>> a better job of things on the do over.
>>
>> That's easy, on Cygwin:
>> make check-local TESTSUITEFLAGS="-k Runpath"
>> is supposed to PASS (not FAIL) and
>> make check-local TESTSUITEFLAGS="-k relpaths"
>> is supposed to XFAIL (not XPASS)
>>
>> (mentioning it here so that I don't forget myself)
>
> Cool. When you have time, please let me know whether the temporary
> branch I made works properly for you. I'll do some more regression
> testing, and if all goes well, I'll transplant the branch onto
> master.
No issues in the testsuite here that I notice. Not trusting that
however...
> My awk script also matches your changes in these hunks, so I'm
> moderately confident that it will have caught any other lurkers too.
...and diffing between master (-) and redo-test-operand-order (+) got
me these among a bunch of bring stuff. But I wonder what I missed
this time?
diff --git a/m4/argz.m4 b/m4/argz.m4
index ad1743e..09568ad 100644
--- a/m4/argz.m4
+++ b/m4/argz.m4
@@ -55,11 +55,11 @@ AS_IF([test -z "$ARGZ_H"],
lt_os_major=${2-0}
lt_os_minor=${3-0}
lt_os_micro=${4-0}
-if test 1 -le "$lt_os_major" \
+if test 1 -lt "$lt_os_major" \
|| { test 1 -eq "$lt_os_major" \
- && { test 5 -le "$lt_os_minor" \
+ && { test 5 -lt "$lt_os_minor" \
|| { test 5 -eq "$lt_os_minor" \
- && test 24 -le "$lt_os_micro"; }; }; }; then
+ && test 24 -lt "$lt_os_micro"; }; }; }; then
lt_cv_sys_argz_works=yes
fi
fi
diff --git a/m4/libtool.m4 b/m4/libtool.m4
index 4413a6d..8ec9beb 100644
--- a/m4/libtool.m4
+++ b/m4/libtool.m4
@@ -4982,7 +4982,7 @@ _LT_EOF
# need to do runtime linking.
case $host_os in aix4.[[23]]|aix4.[[23]].*|aix[[5-9]]*)
for ld_flag in $LDFLAGS; do
- if (test x-brtl = "x$ld_flag" || test x-Wl,-brtl = "x$ld_flag"); then
+ if (test -brtl = "$ld_flag" || test -Wl,-brtl = "$ld_flag"); then
aix_use_runtimelinking=yes
break
fi
@@ -7730,7 +7730,7 @@ for lt_ac_sed in $lt_ac_sed_list /usr/xpg4/bin/sed; do
$lt_ac_sed -e 's/a$//' <
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
> Hi Peter, Ping? > On 19 Sep 2012, at 21:50, Peter Rosin wrote: > >> On 2012-09-19 16:20, Gary V. Vaughan wrote: >>> Hi Peter, >>> >>> My bad, I'm embarrassed to say. I started to write a script to make the >>> appropriate changes, but ended up doing it manually rather than adding >>> more and more corner cases to the throwaway script... a poor choice in >>> hindsight :-( >> >> It's easy to be wise after the fact... If you do redo it, may I suggest >> breaking up the patch in smaller pieces for bisectability? > > I've pushed a temporary branch called gary/redo-test-operand-order to > savannah with seven changesets that reverts the manual version of the > buggy original and redoes it with a painstaking awk script (also checked > in, for the morbidly curious). > > I'm on the fence about committing in smaller pieces... the policy for > libtool has always been to make sure the testsuite passes (at least > on the committer's machine) for every changeset, so that bisecting > using one of the test cases doesn't fail unexpectedly on another > commit that was intentionally pushed with know failures. On the other > hand, the original was a monster, so I can see the benefits of splitting > it up a bit too. > > >>> I think it will be safer to revert the broken patch, and then >>> write a script to reapply those changes automatically as I >>> should have done originally, and only then to merge the result >>> back to head. If you hold off for a few days, I'll do that as >>> penance for my sins when I get back to my computer. >> >> I'm not sure a revert of such a big patch is possible after >> 50-odd more patches? But I was thinking revert too after >> reading it for a while... Who knows what else hides in >> there? > > Well, I wrote and applied the script, diffed the results, and > the testsuite has no regressions for me. I get a weird failure > in distcheck which tries to run distclean in _build/tests/cdemo > aftec removing _build/tests/cdemo/Makefile, that I haven't had > time to check against current master to see if it is a new regression > caused by my patch. > >>> But please hold on to your test case to verify that I've made >>> a better job of things on the do over. >> >> That's easy, on Cygwin: >>make check-local TESTSUITEFLAGS="-k Runpath" >> is supposed to PASS (not FAIL) and >>make check-local TESTSUITEFLAGS="-k relpaths" >> is supposed to XFAIL (not XPASS) >> >> (mentioning it here so that I don't forget myself) > > Cool. When you have time, please let me know whether the temporary > branch I made works properly for you. I'll do some more regression > testing, and if all goes well, I'll transplant the branch onto > master. > >> Commit v2.4.2-120-g962aa91 >> syntax-check: fix violations and implement sc_prohibit_test_const_follows_var >> inadvertenty stomped some comparisons. >> >> * build-aux/ltmain.m4sh (func_mode_compile): Reverse test when checking >> if non-PIC is attempted in shared libraries. >> (func_mode_link): Check for dlprefiles, not dlfiles. >> (func_mode_link): Reverse test so that dependencies are checked when >> pass_all is not in effect. >> --- >> build-aux/ltmain.m4sh |6 +++--- >> 1 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh >> index 14f3c37..0b8defa 100644 >> --- a/build-aux/ltmain.m4sh >> +++ b/build-aux/ltmain.m4sh >> @@ -1350,7 +1350,7 @@ func_mode_compile () >> pic_mode=default >> ;; >>esac >> -if test yes = "$pic_mode" && test pass_all != "$deplibs_check_method"; >> then >> +if test no = "$pic_mode" && test pass_all != "$deplibs_check_method"; >> then >> # non-PIC code in shared libraries is not supported >> pic_mode=default >>fi >> @@ -5151,7 +5151,7 @@ func_mode_link () >> fi >> >> # CHECK ME: I think I busted this. -Ossama >> - if test dlfiles = "$prev"; then >> + if test dlprefiles = "$prev"; then >> # Preload the old-style object. >> func_append dlprefiles " $pic_object" >> prev= >> @@ -6191,7 +6191,7 @@ func_mode_link () >> fi >> elif test yes = "$build_libtool_libs"; then >> # Not a shared library >> - if test pass_all = "$deplibs_check_method"; then >> + if test pass_all != "$deplibs_check_method"; then >> # We're trying link a shared library against a static one >> # but the system doesn't support it. > > > My awk script also matches your changes in these hunks, so I'm > moderately confident that it will have caught any other lurkers too. > > If not, I will branch before 962aa91, run the script, and then apply > the rest of master to that branch one patch at a time until I arrive > at a diff that I can apply to master itself, rather than using revert > as I did on the current temporary branch. > > Cheers, > -- > Gary V. Vaughan (gary AT gnu DOT org) >
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
Hi Peter, On 19 Sep 2012, at 21:50, Peter Rosin wrote: > On 2012-09-19 16:20, Gary V. Vaughan wrote: >> Hi Peter, >> >> My bad, I'm embarrassed to say. I started to write a script to make the >> appropriate changes, but ended up doing it manually rather than adding >> more and more corner cases to the throwaway script... a poor choice in >> hindsight :-( > > It's easy to be wise after the fact... If you do redo it, may I suggest > breaking up the patch in smaller pieces for bisectability? I've pushed a temporary branch called gary/redo-test-operand-order to savannah with seven changesets that reverts the manual version of the buggy original and redoes it with a painstaking awk script (also checked in, for the morbidly curious). I'm on the fence about committing in smaller pieces... the policy for libtool has always been to make sure the testsuite passes (at least on the committer's machine) for every changeset, so that bisecting using one of the test cases doesn't fail unexpectedly on another commit that was intentionally pushed with know failures. On the other hand, the original was a monster, so I can see the benefits of splitting it up a bit too. >> I think it will be safer to revert the broken patch, and then >> write a script to reapply those changes automatically as I >> should have done originally, and only then to merge the result >> back to head. If you hold off for a few days, I'll do that as >> penance for my sins when I get back to my computer. > > I'm not sure a revert of such a big patch is possible after > 50-odd more patches? But I was thinking revert too after > reading it for a while... Who knows what else hides in > there? Well, I wrote and applied the script, diffed the results, and the testsuite has no regressions for me. I get a weird failure in distcheck which tries to run distclean in _build/tests/cdemo aftec removing _build/tests/cdemo/Makefile, that I haven't had time to check against current master to see if it is a new regression caused by my patch. >> But please hold on to your test case to verify that I've made >> a better job of things on the do over. > > That's easy, on Cygwin: > make check-local TESTSUITEFLAGS="-k Runpath" > is supposed to PASS (not FAIL) and > make check-local TESTSUITEFLAGS="-k relpaths" > is supposed to XFAIL (not XPASS) > > (mentioning it here so that I don't forget myself) Cool. When you have time, please let me know whether the temporary branch I made works properly for you. I'll do some more regression testing, and if all goes well, I'll transplant the branch onto master. > Commit v2.4.2-120-g962aa91 > syntax-check: fix violations and implement sc_prohibit_test_const_follows_var > inadvertenty stomped some comparisons. > > * build-aux/ltmain.m4sh (func_mode_compile): Reverse test when checking > if non-PIC is attempted in shared libraries. > (func_mode_link): Check for dlprefiles, not dlfiles. > (func_mode_link): Reverse test so that dependencies are checked when > pass_all is not in effect. > --- > build-aux/ltmain.m4sh |6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh > index 14f3c37..0b8defa 100644 > --- a/build-aux/ltmain.m4sh > +++ b/build-aux/ltmain.m4sh > @@ -1350,7 +1350,7 @@ func_mode_compile () > pic_mode=default > ;; > esac > -if test yes = "$pic_mode" && test pass_all != "$deplibs_check_method"; > then > +if test no = "$pic_mode" && test pass_all != "$deplibs_check_method"; > then > # non-PIC code in shared libraries is not supported > pic_mode=default > fi > @@ -5151,7 +5151,7 @@ func_mode_link () >fi > ># CHECK ME: I think I busted this. -Ossama > - if test dlfiles = "$prev"; then > + if test dlprefiles = "$prev"; then > # Preload the old-style object. > func_append dlprefiles " $pic_object" > prev= > @@ -6191,7 +6191,7 @@ func_mode_link () > fi >elif test yes = "$build_libtool_libs"; then > # Not a shared library > - if test pass_all = "$deplibs_check_method"; then > + if test pass_all != "$deplibs_check_method"; then ># We're trying link a shared library against a static one ># but the system doesn't support it. My awk script also matches your changes in these hunks, so I'm moderately confident that it will have caught any other lurkers too. If not, I will branch before 962aa91, run the script, and then apply the rest of master to that branch one patch at a time until I arrive at a diff that I can apply to master itself, rather than using revert as I did on the current temporary branch. Cheers, -- Gary V. Vaughan (gary AT gnu DOT org)
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
On 2012-09-19 21:32, Roumen Petrov wrote: > Peter Rosin wrote: >> On 2012-09-19 09:31, Peter Rosin wrote: >>> * tests/runpath-in-lalib.at: Make sure shared libraries are created >>> on Windows by passing -no-undefined. Otherwise libb.la fails to record >>> a dependency on liba.la, and the final link of the program then fails >>> with undefined symbols. >>> >>> Signed-off-by: Peter Rosin >>> --- >>> tests/runpath-in-lalib.at |1 + >>> 1 files changed, 1 insertions(+), 0 deletions(-) >>> >>> Ok to push? > No idea . Test pass to me with current repository code, cross build , run in > emulated . You haven't finished reading the thread. The problem was deeper and the regression has been identified as an accidentally inverted test. Adding -no-undefined was not the correct answer (I hinted at that suspicion in my original message), but if your setup works with git master, then your setup does not behave as the real MinGW behaves when trying to link with undefined symbols. Cheers, Peter
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
Peter Rosin wrote: On 2012-09-19 09:31, Peter Rosin wrote: * tests/runpath-in-lalib.at: Make sure shared libraries are created on Windows by passing -no-undefined. Otherwise libb.la fails to record a dependency on liba.la, and the final link of the program then fails with undefined symbols. Signed-off-by: Peter Rosin --- tests/runpath-in-lalib.at |1 + 1 files changed, 1 insertions(+), 0 deletions(-) Ok to push? No idea . Test pass to me with current repository code, cross build , run in emulated . Roumen
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
On 2012-09-19 16:20, Gary V. Vaughan wrote: > Hi Peter, > > My bad, I'm embarrassed to say. I started to write a script to make the > appropriate changes, but ended up doing it manually rather than adding > more and more corner cases to the throwaway script... a poor choice in > hindsight :-( It's easy to be wise after the fact... If you do redo it, may I suggest breaking up the patch in smaller pieces for bisectability? > On 19 ก.ย. 2012, at 19:27, Peter Rosin wrote: >> On 2012-09-19 11:26, Peter Rosin wrote: >>> On 2012-09-19 09:31, Peter Rosin wrote: * tests/runpath-in-lalib.at: Make sure shared libraries are created on Windows by passing -no-undefined. Otherwise libb.la fails to record a dependency on liba.la, and the final link of the program then fails with undefined symbols. Signed-off-by: Peter Rosin --- tests/runpath-in-lalib.at |1 + 1 files changed, 1 insertions(+), 0 deletions(-) Ok to push? Or maybe the failure is deeper than this? Should libb.la record a dependency on liba.la even if libb.la is static only? >>> >>> I likely is deeper, it seems this is a regression since 2.4.2. >> >> I have bisected this regression to 962aa919f51cdf8e2cee4fb2d1d9bafa34d50887 >> syntax-check: fix violations and implement >> sc_prohibit_test_const_follows_var. >> >> I looked through that insanely huge patch and it was not fun. I did >> manage to find a couple of problems: >> >> -if test "$pic_mode" = no && test "$deplibs_check_method" != pass_all; >> then >> +if test yes = "$pic_mode" && test pass_all != "$deplibs_check_method"; >> then >> >> -if test "$prev" = dlprefiles; then >> +if test dlfiles = "$prev"; then >> >> -if test "x`$SED 1q $export_symbols`" != xEXPORTS; then >> +if test EXPORTS = "`$SED 1q $export_symbols`"; then >> >> -if test "x[$]$2" = xyes; then >> +if test yes != "[$]$2"; then >> >> However, my eyes must have glazed over because it is not enough to fix those >> bugs. > > I guess my whole brain glazed over while I was checking and rechecking > before pushing, so I'm not surprised. > >> Comparing to master, I notice that: >> >> * The export_symbols change has a fixup in >> b804ffabee2ce373d9bac6ae2b235ec68e0b55e8 >> fixup: restore EXPORTS test >> * The "x[$]$2" change has a fixup in >> 11869b9c9eb8bcc8cb6a615141f522a447377324 >> m4: fix logic error leading to -fno-rtti being added wrongly. >> >> I have removed a long rant on my opinion of the offending patch, it >> would do no good anyway... > > Thanks for finding it, and sparing me from the additional shame. > >> Bottom line: More eyes needed on that patch! >> >> Ok to push the below? > > I think it will be safer to revert the broken patch, and then > write a script to reapply those changes automatically as I > should have done originally, and only then to merge the result > back to head. If you hold off for a few days, I'll do that as > penance for my sins when I get back to my computer. I'm not sure a revert of such a big patch is possible after 50-odd more patches? But I was thinking revert too after reading it for a while... Who knows what else hides in there? > But please hold on to your test case to verify that I've made > a better job of things on the do over. That's easy, on Cygwin: make check-local TESTSUITEFLAGS="-k Runpath" is supposed to PASS (not FAIL) and make check-local TESTSUITEFLAGS="-k relpaths" is supposed to XFAIL (not XPASS) (mentioning it here so that I don't forget myself) Meanwhile, I found another one by diffing the --debug output from building libb.la in runpath-in-lalib.at with/without the offender: - if test "$deplibs_check_method" != pass_all; then + if test pass_all = "$deplibs_check_method"; then A fixup of this change actually makes the above tests behave on Cygwin/MinGW. So, in case it proves too hard to revert and redo, the below is my current fixup-patch. Cheers, Peter >From 072c4beb6564c39099d4c691620e2fac5f32f7ed Mon Sep 17 00:00:00 2001 From: Peter Rosin Date: Wed, 19 Sep 2012 16:38:34 +0200 Subject: [PATCH] fixup: restore stomped tests Commit v2.4.2-120-g962aa91 syntax-check: fix violations and implement sc_prohibit_test_const_follows_var inadvertenty stomped some comparisons. * build-aux/ltmain.m4sh (func_mode_compile): Reverse test when checking if non-PIC is attempted in shared libraries. (func_mode_link): Check for dlprefiles, not dlfiles. (func_mode_link): Reverse test so that dependencies are checked when pass_all is not in effect. --- build-aux/ltmain.m4sh |6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh index 14f3c37..0b8defa 100644 --- a/build-aux/ltmain.m4sh +++ b/build-aux/ltmain.m4sh @@ -1350,7 +1350,7 @@ func_mode_compile () pic_mode=default ;; esac -if test yes = "$pic_mode" && test pass_all != "$deplibs_check_method"; then +
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
Hi Peter, My bad, I'm embarrassed to say. I started to write a script to make the appropriate changes, but ended up doing it manually rather than adding more and more corner cases to the throwaway script... a poor choice in hindsight :-( On 19 ก.ย. 2012, at 19:27, Peter Rosin wrote: > On 2012-09-19 11:26, Peter Rosin wrote: >> On 2012-09-19 09:31, Peter Rosin wrote: >>> * tests/runpath-in-lalib.at: Make sure shared libraries are created >>> on Windows by passing -no-undefined. Otherwise libb.la fails to record >>> a dependency on liba.la, and the final link of the program then fails >>> with undefined symbols. >>> >>> Signed-off-by: Peter Rosin >>> --- >>> tests/runpath-in-lalib.at |1 + >>> 1 files changed, 1 insertions(+), 0 deletions(-) >>> >>> Ok to push? >>> Or maybe the failure is deeper than this? Should libb.la record a >>> dependency on liba.la even if libb.la is static only? >> >> I likely is deeper, it seems this is a regression since 2.4.2. > > I have bisected this regression to 962aa919f51cdf8e2cee4fb2d1d9bafa34d50887 > syntax-check: fix violations and implement sc_prohibit_test_const_follows_var. > > I looked through that insanely huge patch and it was not fun. I did > manage to find a couple of problems: > > -if test "$pic_mode" = no && test "$deplibs_check_method" != pass_all; > then > +if test yes = "$pic_mode" && test pass_all != "$deplibs_check_method"; > then > > -if test "$prev" = dlprefiles; then > +if test dlfiles = "$prev"; then > > -if test "x`$SED 1q $export_symbols`" != xEXPORTS; then > +if test EXPORTS = "`$SED 1q $export_symbols`"; then > > -if test "x[$]$2" = xyes; then > +if test yes != "[$]$2"; then > > However, my eyes must have glazed over because it is not enough to fix those > bugs. I guess my whole brain glazed over while I was checking and rechecking before pushing, so I'm not surprised. > Comparing to master, I notice that: > > * The export_symbols change has a fixup in > b804ffabee2ce373d9bac6ae2b235ec68e0b55e8 > fixup: restore EXPORTS test > * The "x[$]$2" change has a fixup in > 11869b9c9eb8bcc8cb6a615141f522a447377324 > m4: fix logic error leading to -fno-rtti being added wrongly. > > I have removed a long rant on my opinion of the offending patch, it > would do no good anyway... Thanks for finding it, and sparing me from the additional shame. > Bottom line: More eyes needed on that patch! > > Ok to push the below? I think it will be safer to revert the broken patch, and then write a script to reapply those changes automatically as I should have done originally, and only then to merge the result back to head. If you hold off for a few days, I'll do that as penance for my sins when I get back to my computer. But please hold on to your test case to verify that I've made a better job of things on the do over. > Cheers, > Peter Cheers, -- Gary V. Vaughan (gary AT gnu DOT org)
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
On 2012-09-19 11:26, Peter Rosin wrote: > On 2012-09-19 09:31, Peter Rosin wrote: >> * tests/runpath-in-lalib.at: Make sure shared libraries are created >> on Windows by passing -no-undefined. Otherwise libb.la fails to record >> a dependency on liba.la, and the final link of the program then fails >> with undefined symbols. >> >> Signed-off-by: Peter Rosin >> --- >> tests/runpath-in-lalib.at |1 + >> 1 files changed, 1 insertions(+), 0 deletions(-) >> >> Ok to push? >> Or maybe the failure is deeper than this? Should libb.la record a >> dependency on liba.la even if libb.la is static only? >> >> The relevant difference in libb.la with this patch is this (I have >> elided changes to dlopen and library_names which are empty when >> no shared library is built): >> >> @@ -17,7 +17,7 @@ >> inherited_linker_flags='' >> >> # Libraries that this one depends upon. >> -dependency_libs=' >> -R/home/peda/libtool/git/cygwin/tests/testsuite.dir/047/foobar ' >> +dependency_libs=' >> -R/home/peda/libtool/git/cygwin/tests/testsuite.dir/047/foobar >> /home/peda/libtool/git/cygwin/tests/testsuite.dir/047/liba.la' >> >> # Names of additional weak libraries provided by this library >> weak_library_names='' > > I likely is deeper, it seems this is a regression since 2.4.2. I have bisected this regression to 962aa919f51cdf8e2cee4fb2d1d9bafa34d50887 syntax-check: fix violations and implement sc_prohibit_test_const_follows_var. I looked through that insanely huge patch and it was not fun. I did manage to find a couple of problems: -if test "$pic_mode" = no && test "$deplibs_check_method" != pass_all; then +if test yes = "$pic_mode" && test pass_all != "$deplibs_check_method"; then - if test "$prev" = dlprefiles; then + if test dlfiles = "$prev"; then - if test "x`$SED 1q $export_symbols`" != xEXPORTS; then + if test EXPORTS = "`$SED 1q $export_symbols`"; then -if test "x[$]$2" = xyes; then +if test yes != "[$]$2"; then However, my eyes must have glazed over because it is not enough to fix those bugs. Comparing to master, I notice that: * The export_symbols change has a fixup in b804ffabee2ce373d9bac6ae2b235ec68e0b55e8 fixup: restore EXPORTS test * The "x[$]$2" change has a fixup in 11869b9c9eb8bcc8cb6a615141f522a447377324 m4: fix logic error leading to -fno-rtti being added wrongly. I have removed a long rant on my opinion of the offending patch, it would do no good anyway... Bottom line: More eyes needed on that patch! Ok to push the below? Cheers, Peter >From 79d4c09db4317f2f96ba7cbbfc06a8a9da0ff984 Mon Sep 17 00:00:00 2001 From: Peter Rosin Date: Wed, 19 Sep 2012 14:23:26 +0200 Subject: [PATCH] fixup: restore stomped tests Commit v2.4.2-120-g962aa91 syntax-check: fix violations and implement sc_prohibit_test_const_follows_var inadvertedly stomped some comparisons. * build-aux/ltmain.m4sh (func_mode_compile): Reverse test when checking if non-PIC is attempted in shared libraries. (func_mode_link): Check for dlprefiles, not dlfiles. --- build-aux/ltmain.m4sh |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh index 14f3c37..69722ef 100644 --- a/build-aux/ltmain.m4sh +++ b/build-aux/ltmain.m4sh @@ -1350,7 +1350,7 @@ func_mode_compile () pic_mode=default ;; esac -if test yes = "$pic_mode" && test pass_all != "$deplibs_check_method"; then +if test no = "$pic_mode" && test pass_all != "$deplibs_check_method"; then # non-PIC code in shared libraries is not supported pic_mode=default fi @@ -5151,7 +5151,7 @@ func_mode_link () fi # CHECK ME: I think I busted this. -Ossama - if test dlfiles = "$prev"; then + if test dlprefiles = "$prev"; then # Preload the old-style object. func_append dlprefiles " $pic_object" prev=
Re: [PATCH] tests: feed -no-undefined when linking libtool libraries
On 2012-09-19 09:31, Peter Rosin wrote: > * tests/runpath-in-lalib.at: Make sure shared libraries are created > on Windows by passing -no-undefined. Otherwise libb.la fails to record > a dependency on liba.la, and the final link of the program then fails > with undefined symbols. > > Signed-off-by: Peter Rosin > --- > tests/runpath-in-lalib.at |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > Ok to push? > Or maybe the failure is deeper than this? Should libb.la record a > dependency on liba.la even if libb.la is static only? > > The relevant difference in libb.la with this patch is this (I have > elided changes to dlopen and library_names which are empty when > no shared library is built): > > @@ -17,7 +17,7 @@ > inherited_linker_flags='' > > # Libraries that this one depends upon. > -dependency_libs=' > -R/home/peda/libtool/git/cygwin/tests/testsuite.dir/047/foobar ' > +dependency_libs=' > -R/home/peda/libtool/git/cygwin/tests/testsuite.dir/047/foobar > /home/peda/libtool/git/cygwin/tests/testsuite.dir/047/liba.la' > > # Names of additional weak libraries provided by this library > weak_library_names='' I likely is deeper, it seems this is a regression since 2.4.2. Cheers, Peter
[PATCH] tests: feed -no-undefined when linking libtool libraries
* tests/runpath-in-lalib.at: Make sure shared libraries are created on Windows by passing -no-undefined. Otherwise libb.la fails to record a dependency on liba.la, and the final link of the program then fails with undefined symbols. Signed-off-by: Peter Rosin --- tests/runpath-in-lalib.at |1 + 1 files changed, 1 insertions(+), 0 deletions(-) Ok to push? Or maybe the failure is deeper than this? Should libb.la record a dependency on liba.la even if libb.la is static only? The relevant difference in libb.la with this patch is this (I have elided changes to dlopen and library_names which are empty when no shared library is built): @@ -17,7 +17,7 @@ inherited_linker_flags='' # Libraries that this one depends upon. -dependency_libs=' -R/home/peda/libtool/git/cygwin/tests/testsuite.dir/047/foobar ' +dependency_libs=' -R/home/peda/libtool/git/cygwin/tests/testsuite.dir/047/foobar /home/peda/libtool/git/cygwin/tests/testsuite.dir/047/liba.la' # Names of additional weak libraries provided by this library weak_library_names='' Cheers, Peter diff --git a/tests/runpath-in-lalib.at b/tests/runpath-in-lalib.at index bdd1279..7c433d3 100644 --- a/tests/runpath-in-lalib.at +++ b/tests/runpath-in-lalib.at @@ -41,6 +41,7 @@ instdir=`pwd`/inst libdir=$instdir/lib bindir=$instdir/bin addrunpath=`pwd`/foobar +LDFLAGS="$LDFLAGS -no-undefined" mkdir $instdir $libdir $bindir -- 1.7.9
