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 <p...@lysator.liu.se> 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 <p...@lysator.liu.se> >>>> --- >>>> 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 <p...@lysator.liu.se> 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