Re: [PATCH] tests: feed -no-undefined when linking libtool libraries

[Peter Rosin]

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 <address@hidden> 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 <address@hidden>
>>>> ---
>>>> 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 <address@hidden>
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
+    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.
 
--
1.7.9