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 <[email protected]> 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 booooring 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$//' < conftest.nl >conftest.out || break
     cmp -s conftest.out conftest.nl || break
     # 10000 chars as input seems more than enough
-    test 10 -le "$lt_ac_count" && break
+    test 10 -lt "$lt_ac_count" && break
     lt_ac_count=`expr $lt_ac_count + 1`
     if test "$lt_ac_count" -gt "$lt_ac_max"; then
       lt_ac_max=$lt_ac_count

Looks like at least these lines need improvement:

	  # reverse -lt and -lte operand order
	  line = gensub (lhsx "-l(te?)" rhsx,   "\\1\\4 -g\\3 \"\\2\"\\5", "g", line);
	  line = gensub (lhs "-l(te?)" rhs,     "\\1\\4 -g\\3 \"\\2\"\\5", "g", line);

	  # reverse -gt and -gte operand order
	  line = gensub (lhsx "-g(te?)" rhsx,   "\\1\\4 -l\\3 \"\\2\"\\5", "g", line);
	  line = gensub (lhs "-g(te?)" rhs,     "\\1\\4 -l\\3 \"\\2\"\\5", "g", line);

What's this gte/lte thing anyway? Isn't it supposed to be ge/le?

That doesn't explain the dangerous looking -brtl change.

> 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

	test $X -eq 5

is much more readable than

	test 5 -eq $X

simply because it matches the way I'm thinking, i.e. if X equals 5
then do something. I never think the other way around, and that is
more of a burden to readability than to scan past the odd x if that's
needed to stop the possibility of special chars. But maybe that's
just me?

Cheers,
Peter