Bug 46417

Summary: Gabble shouldn't install an unversioned library to ${libdir}
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: gabbleAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium CC: jonny.lamb, ken.vandine, olli.salli, siraj
Version: git masterKeywords: patch
Hardware: Other   
OS: All   
URL: http://cgit.freedesktop.org/~smcv/telepathy-gabble/commit?h=make-library-sane-46417
Whiteboard:
i915 platform: i915 features:
Attachments: Make libgabble-plugins.la a libtool convenience library on non-Windows, non-Android
[alternative part 1: Wocky] If configured with --enable-shared-suffix=*, install a shared library
[alternative part 2: Gabble] Build Wocky as a version-specific shared library
[alternative part 3: Gabble] Link plugins in the same way on Unix as on Windows
Use the standard EXEEXT variable rather than reinventing it
[alternative part 0: Wocky] Use LIBADD properly, rather than putting libraries in LDFLAGS
[1/6] telepathy-gabble.pc: link to Wocky
[2/6] configure.ac: remove apostrophe from a help message so syntax highlighting works
[3/6] Avoid Wocky trying to install into --prefix=NONE
[4/6] Avoid non-portable use of += in configure.ac
[5/6] Install non-ABI-stable libraries used by plugins to a private directory
[6/6] Replace plugindir with an AC_ARG_VAR so it can be passed to configure

Description Simon McVittie 2012-02-21 13:54:47 UTC
W: telepathy-gabble: shlib-without-versioned-soname usr/lib/libgabble-plugins.so libgabble-plugins.so
N: 
N:    The listed shared library in a public library directory has an SONAME
N:    that does not contain any versioning information, either after the .so
N:    or before it and set off by a hyphen. It cannot therefore be represented
N:    in the shlibs system, and if linked by binaries its interface cannot
N:    safely change. There is no backward-compatible way to migrate programs
N:    linked against it to a new ABI.
N:    
N:    Normally, this means the shared library is a private library for a
N:    particular application and is not meant for general use. Policy
N:    recommends that such libraries be installed in a subdirectory of
N:    /usr/lib rather than in a public shared library directory.

Lintian is right about this, and so is Debian Policy. We shouldn't be putting an unstable library with no version information in the library search path.

When (if ever) the Gabble plugin interface is stable (prerequisite: Wocky is stable), we can install it as a shared library with proper versioning. That day is not today.

Options include:

* Make it shared only on platforms where it has to be shared for technical
  reasons (Windows, Android), and a static convenience library on
  sensible platforms

* 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))

I've prepared a patch for the first of these, which I'll attach soon.
Comment 1 Simon McVittie 2012-02-21 14:09:17 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.
Comment 2 Simon McVittie 2012-02-23 03:13:21 UTC
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.
Comment 3 Simon McVittie 2012-02-23 03:20:19 UTC
(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.
Comment 4 Simon McVittie 2012-02-23 03:35:56 UTC
(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).
Comment 5 Simon McVittie 2012-02-23 04:11:44 UTC
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.
Comment 6 Simon McVittie 2012-02-23 04:18:36 UTC
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
Comment 7 Simon McVittie 2012-02-23 04:21:42 UTC
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
Comment 8 Simon McVittie 2012-02-23 04:24:56 UTC
(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.
Comment 9 Simon McVittie 2012-02-23 04:31:05 UTC
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.
Comment 10 Simon McVittie 2012-02-23 04:36:35 UTC
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)
Comment 11 Olli Salli 2012-02-23 09:06:43 UTC
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?
Comment 12 Olli Salli 2012-02-23 09:08:57 UTC
I'm afraid this needs to be cloned to a Salut bug as well additionally - it has basically the same problem.
Comment 13 Simon McVittie 2012-02-24 02:25:46 UTC
(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.)
Comment 14 Simon McVittie 2012-02-24 02:29:50 UTC
(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.
Comment 15 Simon McVittie 2012-03-01 06:59:34 UTC
(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?
Comment 16 Simon McVittie 2012-03-02 02:28:22 UTC
*** Bug 46830 has been marked as a duplicate of this bug. ***
Comment 17 Simon McVittie 2012-03-08 03:10:14 UTC
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.
Comment 18 Olli Salli 2012-03-08 11:49:52 UTC
(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.
Comment 19 Siraj Razick 2012-03-08 14:53:49 UTC
(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
Comment 20 Olli Salli 2012-03-08 15:01:28 UTC
(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.
Comment 21 Simon McVittie 2012-03-09 03:39:08 UTC
(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?
Comment 22 Simon McVittie 2012-03-09 05:42:47 UTC
I've pushed the changes so far, thanks for reviewing. More patches on the way (dropping severity).
Comment 23 Simon McVittie 2012-03-09 05:44:41 UTC
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.
Comment 24 Simon McVittie 2012-03-09 05:45:07 UTC
Created attachment 58234 [details] [review]
[2/6] configure.ac: remove apostrophe from a help message so  syntax highlighting works

---

It was annoying me.
Comment 25 Simon McVittie 2012-03-09 05:48:37 UTC
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.
Comment 26 Simon McVittie 2012-03-09 05:48:58 UTC
Created attachment 58236 [details] [review]
[4/6] Avoid non-portable use of += in configure.ac
Comment 27 Simon McVittie 2012-03-09 05:49:24 UTC
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'
Comment 28 Simon McVittie 2012-03-09 05:50:09 UTC
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".
Comment 29 Jonny Lamb 2012-03-22 13:41:33 UTC
All your six patches look good.
Comment 30 Simon McVittie 2012-03-23 08:13:44 UTC
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.