GNOME Bugzilla – Bug 735068
Latest pygobject fails to link test helper libraries on OS X
Last modified: 2014-08-21 21:36:10 UTC
The problem seems to be introduced in 9eaeba9079c23d7e2837f62e8ed2b26c018351b6 where -no-undefined is added to the link flags. On OS X, just as on windows, libtesthelper.la should be explicitly linked against python to avoid unreferenced symbols.
Is this just a matter of adding PYTHON_LIBS to LIBADD instead of LDFLAGS? Does the following fix the problem? diff --git a/tests/Makefile.am b/tests/Makefile.am index 62631dc..3187372 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -55,8 +55,8 @@ CLEANFILES += Regress-1.0.gir Regress-1.0.typelib GIMarshallingTests-1.0. check_LTLIBRARIES += testhelper.la testhelper_la_CFLAGS = -I$(top_srcdir)/gi $(PYTHON_INCLUDES) $(GLIB_CFLAGS) -testhelper_la_LDFLAGS = -module -avoid-version -no-undefined $(PYTHON_LIBS) -testhelper_la_LIBADD = $(GLIB_LIBS) +testhelper_la_LDFLAGS = -module -avoid-version -no-undefined +testhelper_la_LIBADD = $(GLIB_LIBS) $(PYTHON_LDFLAGS) $(PYTHON_LIBS) testhelper_la_SOURCES = \ testhelpermodule.c \
I think it's more that PYTHON_LIBS is only set for win32, as per: https://git.gnome.org/browse/pygobject/tree/configure.ac#n129
Created attachment 283933 [details] [review] configure.ac: Merge multiple platform case statements Merge platform_win32 and os_win32 variables and define "link_python_libs" for win32 as well as darwin. Notes: I can't test this on either win32 or darwin so I will need confirmation that it works on both platforms before committing. Also note there is a bit of a platform dance in gi/Makefile.am which is related, should we be doing the same thing in tests/Makefile.am? This way darwin wouldn't need the -no-undefined flag. In any case it is unclear to me what the disadvantage of -no-undefined and also not just always requiring linking to Python libs is. Is it just to avoid build time dependencies when possible? https://git.gnome.org/browse/pygobject/tree/gi/Makefile.am?id=3.13.90#n17
Review of attachment 283933 [details] [review]: I can confirm that this works on OS X. ::: configure.ac @@ +70,3 @@ ;; esac +AC_MSG_RESULT([$os_win32]) Nitpick, but this is misleading. It says before it's checking for Darwin and Win32, but on Darwin it will say 'no'
Created attachment 284033 [details] [review] configure.ac: Merge multiple platform case statements Updated with more specific text noting build time linking. Fixed copy paste bug where link_python_libs was yes for all platforms.
Review of attachment 284033 [details] [review]: See inline comment. I can't test for windows, but Ignacio probably could try it out quickly. ::: configure.ac @@ +71,3 @@ esac +AC_MSG_RESULT([$link_python_libs]) +AM_CONDITIONAL(OS_WIN32, test "$os_win32" = "yes") I can't test, but this removes the PLATFORM_WIN32 conditional altogether. Was it never used? Also, before there was apparently a distinction where PLATFORM_WIN32 was true for cygwin, but OS_WIN32 was not.
Review of attachment 284033 [details] [review]: ::: configure.ac @@ +71,3 @@ esac +AC_MSG_RESULT([$link_python_libs]) +AM_CONDITIONAL(OS_WIN32, test "$os_win32" = "yes") Yep, PLATFORM_WIN32 wasn't in use. I believe a lot of this is cargo culted automake snippets. I pinged Ignacio earlier and he said he would test it.
Just tested and it works fine for me. See though that I used msys2 not cygwin. But I guess that if somebody actually cares about cygwin they should just test and report. I'd say let's get it in
Thanks for testing.