Created attachment 145731 [details] [review]
check for libice when using libsm

This looks wrong to me. On my system, sm.pc contains

Requires: ice xproto

Was this changed? If so, when, where (your distro only, or upstream), and why?

As per sm.pc.in it's:
Requires: ice xproto
Requires.private: ice
Cflags: -I${includedir}
Libs: -L${libdir} -lSM

That means pkg-config sm --libs only outputs -lSM, so you're not linking against ICE.

Not so, here:

$ pkg-config --libs sm
-lSM -lICE
$ pkg-config --version
0.22

Requires: means that ice's libs are pulled in too. Did pkg-config break this?

This is on karmic:

$ pkg-config --libs sm
-lSM  
$ pkg-config --version
0.22

However this is /usr/lib/pkgconfig/sm.pc:

prefix=/usr
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include

Name: SM
Description: X Session Management Library
Version: 1.1.0
Requires.private: ice xproto
Cflags: -I${includedir}
Libs: -L${libdir} -lSM

From ubuntu change log in libsm 2:1.1.0-2

libsm (2:1.1.0-2) unstable; urgency=low

  * Upload to unstable.

 -- Julien Cristau <jcristau@debian.org>  Mon, 16 Feb 2009 01:33:27 +0100

libsm (2:1.1.0-1) experimental; urgency=low

  [ Brice Goglin ]
  * Use ${binary:Version} instead of the deprecated ${Source-Version}.

  [ Julien Cristau ]
  * New upstream release.
    - generate client IDs using libuuid instead of gethostbyname()
  * Switch to running autoreconf at build time; clean up in debian/rules
    clean, and build-depend on automake, libtool and xutils-dev.
  * Drop -1 debian revisions from build-deps.
  * Stop handling nostrip explicitly in debian/rules (dh_strip does it
    already), and allow parallel builds using sample code from policy.
  * Add myself to Uploaders.
  * Don't export a dependency on ice in sm.pc.

(In reply to comment #4)
> Requires: means that ice's libs are pulled in too. Did pkg-config break this?

No, but in the .pc there's Requires.private: ice, that might be the reason.

Upstream still has the Requires: ice [http://cgit.freedesktop.org/xorg/lib/libSM/tree/sm.pc.in], so this is an ubuntu-only change. I can't find any bug filed for this on libsm at bfo, nor can I find a launchpad bug for this change in ubuntu. Turns out this originated in debian, but there's no bug filed in debian either where one might find an explanation.

Now this change is totally wrong, and should be reverted immediately in debian and ubuntu. However, that still means that it's broken at least in jaunty, so I guess we'll have to make this change :-(((

Ubuntu really seems not wanting one to file bugs, but after jumping through many hoops I finally succeeded to file a bug on launchpad for this! See https://bugs.launchpad.net/ubuntu/+source/libsm/+bug/455614 .

Also I found many other Gnome modules that use eggsmclient and only check for sm but not ice, too (e.g. totem, gedit, evince, gnome-terminal). Are we expected to fix them all just so debian & ubuntu can continue with this nonsense‽

Re comment 5: If you're on karmic, which has this broken pc file, does linking fail for you too like comment 0 says? I don't think so, at least you haven't complained so far :) — and jaunty's broken too, so we should have heard about this earlier...

Builds/links just fine here on karmic. Did on jaunty as well.

Builds fine on Ubuntu because libSM includes the ICE headers with the full path from /usr/include and ld automatically brings in libICE as it is a dependency of libSM.

The change in Ubuntu should be fixed because it is:
a) inconsistent between OSs
b) removes dependency information (pkg-config cannot detect that libSM will not work without libICE)
c) cannot handle libICE headers being installed in a different location (e.g. by overriding PKG_CONFIG_PATH for a test version).

However, as eggsmclient-xsmp.c explicitly includes libICE headers it should require libICE CFLAGS and LIBS.

b) and c) are not true as the header information is still provided by pkg-config.

This change is correct according to the pkg-config authors:
http://err.no/personal/blog/2008/Mar/25

a) still stands.  I have no idea why Debian considers this of great importance to change and not to request upstream to make the change but it will have to be raised with them.

Note that Luca's patch is valid as libgames-support/eggsmclient-xsmp.c contains references to libICE thus it must link against it.

I can answer for a). 

One of our goals in Debian is to reduce the number of unneeded dependencies. The largest source of such dependencies is brought - now that libtool has been half-nuked - by pkg-config files using abusive Requires.

Reducing the number of undeeded direct dependencies is of great importance, since it eases transitions between versions of libraries a lot, be it for introducing new symbols or when changing sonames.

(In reply to comment #15)
> Reducing the number of undeeded direct dependencies is of great importance,
> since it eases transitions between versions of libraries a lot, be it for
> introducing new symbols or when changing sonames.

So to make things easier for you when the library changes ABI in future, you break the library API/ABI yourself by changing its .pc file? Seems rather strange to me.

(In reply to comment #16)
> So to make things easier for you when the library changes ABI in future, you
> break the library API/ABI yourself by changing its .pc file? Seems rather
> strange to me.

Applications relying on a side effect of the former dependency system in .pc files (which has been fixed by introducing Requires.private) are, from my POV, buggy to begin with.

Similarly they will break when using the “gold” branch of binutils (which I hope to see some day by default) if they use an indirect dependency without linking to it.

I’d also add you might consider this part of the API, although I find this reasoning a bit twisted, but not of the ABI: execution of existing packages is not affected in any way by changes in the .pc files. Transient compilation breakage is accepted, but runtime breakage is not.

(In reply to comment #16)
> So to make things easier for you when the library changes ABI in future, you
> break the library API/ABI yourself by changing its .pc file? Seems rather
> strange to me.

That's unrelated. Per comment #13, gnome-games is already buggy. Isn't it?

"However, as eggsmclient-xsmp.c explicitly includes libICE headers it should
require libICE CFLAGS and LIBS."

Pushed the patch due to comment #13.

http://git.gnome.org/browse/gnome-games/commit/?id=9cf9d4734e07fb6ba2a28947749927cd152e52ed