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

[Gary V. Vaughan]

Hi Peter,

On 19 Sep 2012, at 21:50, Peter Rosin <address@hidden> 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)