|
Description
Simon McVittie
2012-02-21 13:54:47 UTC
Created attachment 57424 [details] [review] Make libgabble-plugins.la a libtool convenience library on non-Windows, non-Android This avoids putting an unversioned shared library in the public library directory, which is discouraged. It's possible that this is also causing crashes with non-default compiler/linker options (kenvandine reported <http://paste.ubuntu.com/853268/>, and I've seen a similar crash with ld -Bsymbolic). I'm not surprised it could be a problem, because Wocky ends up linked into libgabble-plugins.so, and (separately!) into the executable. This is safe if Wocky is a shared library - but if Wocky is a static library (which it is, on sensible platforms, because it isn't stable yet), we end up with two complete copies of Wocky, and they fight. The Android build compiles Wocky as a shared library even though it isn't stable, because everything is a shared library on Android. I believe the Windows build might be using a "system" Wocky too? We don't necessarily need to use exactly the solution I propose, but we do need to fix this somehow. (In reply to comment #0) > * Put it in a private directory (e.g. alongside the plugins) > > * Give it a SONAME, but one that changes every release (i.e. change from > -avoid-version to -release $(VERSION)) Because both libgabble-plugins.la and libgabble-convenience.la link Wocky, if we leave libgabble-plugins shared, we'll have to make Wocky shared too, by using one of these techniques. For "put it in a private directory" we could use ${libdir}/telepathy/gabble-private or something (the executable will end up with an RPATH). We can't re-use ${libdir}/telepathy/gabble-0 (the plugins directory) because the name of libgabble-plugins.so itself matches the naming convention for plugins. (In reply to comment #2) > It's possible that this is also causing crashes with non-default > compiler/linker options (kenvandine reported <http://paste.ubuntu.com/853268/>, > and I've seen a similar crash with ld -Bsymbolic). It seems some Ubuntu releases do default to using gcc -Wl,-Bsymbolic-functions to link: https://bugs.launchpad.net/ubuntu/+source/lives/+bug/481085 https://bugs.launchpad.net/ubuntu/+source/dpkg/+bug/234901 This was deliberate (<https://wiki.ubuntu.com/DistCompilerFlags>), presumably to speed up application startup (and possibly runtime speed) by reducing relocations. I can confirm that every Gabble test fails with "gcc -Wl,-Bsymbolic,--no-add-needed,--warn-common,-z,now,--fatal-warnings" (which basically means "please break my package if it does anything slightly suspicious"). With my patch, the same compiler options work fine. I've amended the commit message accordingly: > Make libgabble-plugins.la a libtool convenience library on non-Windows, > non-Android > > This avoids putting an unversioned shared library in the public library > directory, which is discouraged. It also avoids crashes when linked with > -Bsymbolic-functions (or -Bsymbolic), which result from having two copies of > Wocky (one PIC/shared in libgabble-plugins.so, and one non-PIC/static in > libgabble-convenience.la) which fight. -Bsymbolic-functions are the default on > some Ubuntu releases, to reduce relocations. Without -Bsymbolic-functions, the > non-PIC copy of Wocky in the executable takes precedence (the normal ELF ABI > rules, which allow "interposing" symbols). Created attachment 57517 [details] [review] [alternative part 1: Wocky] If configured with --enable-shared-suffix=*, install a shared library The suffix should be something like gabble-0.15.4, to yield libwocky-gabble-0.15.4.so ("the Wocky from Gabble 0.15.4"). --- This is part 1 of an alternative approach. Created attachment 57518 [details] [review] [alternative part 2: Gabble] Build Wocky as a version-specific shared library --- This is part 2 of the alternative approach; the Wocky submodule update is part 1. This also builds OK with -Bsymbolic, etc. When installed it looks something like this: /usr/local/libexec/telepathy-gabble /usr/local/lib/telepathy/gabble-0/libconsole.so (etc.) /usr/local/lib/libgabble-plugins-0.15.4.1.so /usr/local/lib/libwocky-telepathy-gabble-0.15.4.1.so # these symlinks would not be packaged in distributions, in practice /usr/local/lib/libwocky.so -> libwocky-telepathy-gabble-0.15.4.1.so /usr/local/lib/libgabble-plugins.so -> libgabble-plugins-0.15.4.1.so Shared library linkages: % objdump -x /home/smcv/tmp/usr/local/libexec/telepathy-gabble | grep NEEDED NEEDED libgabble-plugins-0.15.4.1.so NEEDED libdbus-glib-1.so.2 NEEDED libdbus-1.so.3 NEEDED libwocky-telepathy-gabble-0.15.4.1.so NEEDED libxml2.so.2 NEEDED libsqlite3.so.0 NEEDED libgnutls.so.26 NEEDED libtelepathy-glib.so.0 NEEDED libsoup-2.4.so.1 NEEDED libgio-2.0.so.0 NEEDED libnice.so.10 NEEDED libgobject-2.0.so.0 NEEDED libgthread-2.0.so.0 NEEDED libgmodule-2.0.so.0 NEEDED librt.so.1 NEEDED libglib-2.0.so.0 NEEDED libpthread.so.0 NEEDED libc.so.6 % objdump -x /home/smcv/tmp/usr/local/lib/libgabble-plugins-0.15.4.1.so | grep NEEDED NEEDED libdbus-glib-1.so.2 NEEDED libdbus-1.so.3 NEEDED libwocky-telepathy-gabble-0.15.4.1.so NEEDED libtelepathy-glib.so.0 NEEDED libsoup-2.4.so.1 NEEDED libgio-2.0.so.0 NEEDED libnice.so.10 NEEDED libgobject-2.0.so.0 NEEDED libgthread-2.0.so.0 NEEDED libgmodule-2.0.so.0 NEEDED librt.so.1 NEEDED libglib-2.0.so.0 NEEDED libpthread.so.0 NEEDED libc.so.6 % objdump -x /home/smcv/tmp/usr/local/lib/libwocky-telepathy-gabble-0.15.4.1.so|grep NEEDED NEEDED libgio-2.0.so.0 NEEDED libgobject-2.0.so.0 NEEDED libgmodule-2.0.so.0 NEEDED libgthread-2.0.so.0 NEEDED librt.so.1 NEEDED libglib-2.0.so.0 NEEDED libxml2.so.2 NEEDED libsqlite3.so.0 NEEDED libgnutls.so.26 NEEDED libpthread.so.0 NEEDED libc.so.6 % objdump -x /home/smcv/tmp/usr/local/lib/telepathy/gabble-0/libconsole.so|grep NEEDED NEEDED libpthread.so.0 NEEDED libc.so.6 Created attachment 57519 [details] [review] [alternative part 3: Gabble] Link plugins in the same way on Unix as on Windows --- Optional part 3 of the alternative approach. This changes the linkage of libconsole.so to: % objdump -x /home/smcv/tmp/usr/local/lib/telepathy/gabble-0/libconsole.so|grep NEEDED NEEDED libwocky-telepathy-gabble-0.15.4.1.so NEEDED libdbus-glib-1.so.2 NEEDED libdbus-1.so.3 NEEDED libgio-2.0.so.0 NEEDED libgobject-2.0.so.0 NEEDED libgmodule-2.0.so.0 NEEDED libgthread-2.0.so.0 NEEDED librt.so.1 NEEDED libglib-2.0.so.0 NEEDED libtelepathy-glib.so.0 NEEDED libgabble-plugins-0.15.4.1.so NEEDED libpthread.so.0 NEEDED libc.so.6 (In reply to comment #5) > The suffix should be something like gabble-0.15.4, to yield > libwocky-gabble-0.15.4.so ("the Wocky from Gabble 0.15.4"). I believe this would come out as libwocky-gabble-0.15.4.dll on Windows. Created attachment 57520 [details] [review] Use the standard EXEEXT variable rather than reinventing it Not directly related, I just happened to notice it among the Windows changes. Created attachment 57521 [details] [review] [alternative part 0: Wocky] Use LIBADD properly, rather than putting libraries in LDFLAGS --- Attachment #57517 [details] requires this to go in first. If Attachment #57424 [details] is the preferred approach, this could go in Wocky anyway - it's general build-system cleanup. Branches in cgit: Wocky: http://cgit.freedesktop.org/~smcv/wocky/log?h=shareable-46417 Gabble: http://cgit.freedesktop.org/~smcv/telepathy-gabble/log?h=make-library-sane-46417 (make libgabble-plugins static); or http://cgit.freedesktop.org/~smcv/telepathy-gabble/log?h=shared-wocky-46417 (make Wocky and libgabble-plugins shared and version-specific) This makes a world of sense. Can you put the patches to a branch somewhere, so that we could easily test that the system still works OK on windows and android? I'm afraid this needs to be cloned to a Salut bug as well additionally - it has basically the same problem. (In reply to comment #11) > Can you put the patches to a branch somewhere, so that we could easily test > that the system still works OK on windows and android? Already there, see Comment #10 for cgit. Having done the work, I think I prefer the more complex but more-future-proof "make Unix behave like Windows, but with sane libraries" version: if we're going to be constrained by the "everything is a DLL" requirements of Windows/Android, then the build system should be equally strict on Unix (scatter -no-undefined liberally), to make sure we don't accidentally break Windows/Android. Repositories/branches to clone: ssh://people.freedesktop.org/~smcv/wocky.git shareable-46417 ssh://people.freedesktop.org/~smcv/telepathy-gabble shared-wocky-46417 (s/ssh/git/ for anongit access.) (In reply to comment #12) > I'm afraid this needs to be cloned to a Salut bug as well additionally - it has > basically the same problem. The Wocky change is the same (obviously), and the Gabble bits should hopefully move across to Salut nicely. I suggest testing/merging this branch first, then following up with a corresponding change to Salut. I can review the Salut branch, or do it myself if someone will review it. (In reply to comment #11) > This makes a world of sense. Is that a positive review? If not, could someone review this, please? (In reply to comment #13) > (In reply to comment #11) > > Can you put the patches to a branch somewhere, so that we could easily test > > that the system still works OK on windows and android? > > Already there, see Comment #10 for cgit. Having done the work, I think I > prefer [...] > ssh://people.freedesktop.org/~smcv/wocky.git shareable-46417 > ssh://people.freedesktop.org/~smcv/telepathy-gabble shared-wocky-46417 Has anyone tested these on Android and/or Windows? Do they work? *** Bug 46830 has been marked as a duplicate of this bug. *** Olli, Jonny: could you review this please? I consider it to be a blocker for Gabble 0.16: it breaks Gabble with Ubuntu's default compiler options, and in any case we shouldn't have a stable branch that installs an unversioned library. The options are: * Revert the patches that made gabble-plugins a shared library, until we can do them properly. Regresses Windows support, potentially regresses Android support, but makes Unix support good again. If nobody reviews a better solution, I'm very tempted to do this. * Attachment #57521 [details], Attachment #57517 [details], Attachment #57518 [details], Attachment #57519 [details]: turn Wocky into a private shared library (whose name is specific to the version of the project it's embedded in), turn gabble-plugins from an unversioned to a versioned shared library. Helps to keep our Windows/Android builds working, by making our Unix build similar. * Attachment #57424 [details]: revert the plugins library to static on Unix. Minimal-impact, doesn't help Windows/Android, can't hurt Windows/Android either. * Maintain Wocky as a proper shared library with an ABI, and use a system copy. Not a good idea for 0.16 I don't think, since we should freeze that branch somewhat soon, for GNOME 3.4. (In reply to comment #17) > Olli, Jonny: could you review this please? I consider it to be a blocker for > Gabble 0.16: it breaks Gabble with Ubuntu's default compiler options, and in > any case we shouldn't have a stable branch that installs an unversioned > library. > > The options are: > > * Attachment #57521 [details], Attachment #57517 [details], Attachment #57518 [details], Attachment #57519 [details]: > turn Wocky into a private shared library (whose name is specific to the > version of the project it's embedded in), turn gabble-plugins from an > unversioned to a versioned shared library. Helps to keep our Windows/Android > builds working, by making our Unix build similar. > Yeah, this is the most sensible option in the long run. We have the buildbot to catch broken windows builds - but it's better if it already breaks the Linux developer's build so they notice it right away. I've reviewed these patches; the build system changes look OK. Siraj is testing them now with the Ytstenut plugin with his Windows build env and will report here how it fared in a minute. The Android buildbot tells us if we happen to break android with this, but that shouldn't happen if both Windows and Linux builds work OK. ++ for merging if Siraj doesn't notice any problems. (In reply to comment #18) > (In reply to comment #17) > > Olli, Jonny: could you review this please? I consider it to be a blocker for > > Gabble 0.16: it breaks Gabble with Ubuntu's default compiler options, and in > > any case we shouldn't have a stable branch that installs an unversioned > > library. > > > > The options are: > > > > * Attachment #57521 [details], Attachment #57517 [details], Attachment #57518 [details], Attachment #57519 [details]: > > turn Wocky into a private shared library (whose name is specific to the > > version of the project it's embedded in), turn gabble-plugins from an > > unversioned to a versioned shared library. Helps to keep our Windows/Android > > builds working, by making our Unix build similar. > > > > Yeah, this is the most sensible option in the long run. We have the buildbot to > catch broken windows builds - but it's better if it already breaks the Linux > developer's build so they notice it right away. > > I've reviewed these patches; the build system changes look OK. Siraj is testing > them now with the Ytstenut plugin with his Windows build env and will report > here how it fared in a minute. The Android buildbot tells us if we happen to > break android with this, but that shouldn't happen if both Windows and Linux > builds work OK. > > ++ for merging if Siraj doesn't notice any problems. Hi :) The changes are good :). I cross compiled gabble/wocky with the proposed patch set and then compiled yts-plugins and they all compiled fine *note when compiling yts-plugins I had to pass --with-shared-wocky and appened the wocky-uninstalled.pc dir to PKG_CONFIG_PATH. after that everything compiled just fine. BR. Siraj (In reply to comment #19) > *note when compiling yts-plugins I had to pass --with-shared-wocky and appened > the wocky-uninstalled.pc dir to PKG_CONFIG_PATH. after that everything compiled > just fine. > > BR. > > Siraj Out-of-tree plugins now have to explicitly link against Wocky, because it's no longer contained in the plugins library. (In reply to comment #20) > Out-of-tree plugins now have to explicitly link against Wocky, because it's no > longer contained in the plugins library. I think that's reasonable, tbh. How about I add the Wocky library to gabble-plugins.pc? I've pushed the changes so far, thanks for reviewing. More patches on the way (dropping severity). Created attachment 58233 [details] [review] [1/6] telepathy-gabble.pc: link to Wocky --- This should fix the regression that Siraj noticed, of needing to link Wocky explicitly. We can't use an installed wocky.pc, because until Wocky is stable, there isn't one... so just link libwocky directly. Created attachment 58234 [details] [review] [2/6] configure.ac: remove apostrophe from a help message so syntax highlighting works --- It was annoying me. Created attachment 58235 [details] [review] [3/6] Avoid Wocky trying to install into --prefix=NONE --- +# If you don't specify --prefix, it starts off as NONE. Autoconf +# would normally do this defaulting for us later, but that's too +# late to help Wocky. This resulted in "make install DESTDIR=$HOME/tmp" (using the default prefix, /usr/local) installing Wocky headers to $HOME/tmpNONE/include instead of the expected $HOME/tmp/usr/local/include. Created attachment 58236 [details] [review] [4/6] Avoid non-portable use of += in configure.ac Created attachment 58237 [details] [review] [5/6] Install non-ABI-stable libraries used by plugins to a private directory This avoids having Gabble and Salut, or old Gabble and a future stable Wocky, fight over the libwocky.so symlink. If you're building for a tightly controlled platform where Gabble and Salut are definitely using the same Wocky version, you can put them in the normal libdir with ./configure pluginexeclibdir='${libdir}' or (when Salut has been updated with this change) make them share a private library directory: ./configure pluginexeclibdir='${libdir}/telepathy/ytstenut-1.0' Created attachment 58238 [details] [review] [6/6] Replace plugindir with an AC_ARG_VAR so it can be passed to configure This lets you configure the plugin directory: ./configure pluginexecdir='${libdir}/my-gabble-plugins' The directory-name variable has 'exec' in it because Automake installs unknown directory names with 'exec' in their variable name during "make install-exec", and other unknown directory names during "make install-data". All your six patches look good. Thanks, merged for 0.15.6. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.