[PATCH 0/3] build fixes

[PATCH 0/3] build fixes

[Felipe Contreras]

Hi,

The important one is #2, which allows to build on Fedora 13.

Felipe Contreras (3):
  configure: check platform beforehand
  build: fix DSO dependencies
  configure: optimize uname finding a bit

 Makefile.local |    3 --
 configure      |   74 ++++++++++++++++++++++++++++----------------------------
 2 files changed, 37 insertions(+), 40 deletions(-)

[PATCH 1/3] configure: check platform beforehand

[Felipe Contreras]

Will be needed in next patches.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 configure |   62 ++++++++++++++++++++++++++++++------------------------------
 1 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/configure b/configure
index c86a227..1eb4785 100755
--- a/configure
+++ b/configure
@@ -195,6 +195,37 @@ EOF
 
 errors=0
 
+libdir_in_ldconfig=0
+
+printf "Checking which platform we are on... "
+if [ `uname` = "Darwin" ] ; then
+    printf "Mac OS X.\n"
+    platform=MACOSX
+    linker_resolves_library_dependencies=0
+elif [ `uname` = "SunOS" ] ; then
+    printf "Solaris.\n"
+    platform=SOLARIS
+    linker_resolves_library_dependencies=0
+elif [ `uname` = "Linux" ] ; then
+    printf "Linux\n"
+    platform=LINUX
+    linker_resolves_library_dependencies=1
+    ldconfig_paths=$(/sbin/ldconfig -N -X -v 2>/dev/null | sed -n -e 's,^\(/.*\):\( (.*)\)\?$,\1,p')
+    for path in $ldconfig_paths; do
+	echo "Checking $path compared to $libdir_expanded"
+	if [ "$path" = "$libdir_expanded" ]; then
+	    libdir_in_ldconfig=1
+	fi
+    done
+else
+    printf "Unknown.\n"
+    cat <<EOF
+
+*** Warning: Unknown platform. Notmuch might or might not build correctly.
+
+EOF
+fi
+
 if pkg-config --version > /dev/null 2>&1; then
     have_pkg_config=1
 else
@@ -272,37 +303,6 @@ else
     have_emacs=0
 fi
 
-libdir_in_ldconfig=0
-
-printf "Checking which platform we are on... "
-if [ `uname` = "Darwin" ] ; then
-    printf "Mac OS X.\n"
-    platform=MACOSX
-    linker_resolves_library_dependencies=0
-elif [ `uname` = "SunOS" ] ; then
-    printf "Solaris.\n"
-    platform=SOLARIS
-    linker_resolves_library_dependencies=0
-elif [ `uname` = "Linux" ] ; then
-    printf "Linux\n"
-    platform=LINUX
-    linker_resolves_library_dependencies=1
-    ldconfig_paths=$(/sbin/ldconfig -N -X -v 2>/dev/null | sed -n -e 's,^\(/.*\):\( (.*)\)\?$,\1,p')
-    for path in $ldconfig_paths; do
-	echo "Checking $path compared to $libdir_expanded"
-	if [ "$path" = "$libdir_expanded" ]; then
-	    libdir_in_ldconfig=1
-	fi
-    done
-else
-    printf "Unknown.\n"
-    cat <<EOF
-
-*** Warning: Unknown platform. Notmuch might or might not build correctly.
-
-EOF
-fi
-
 if [ $errors -gt 0 ]; then
     cat <<EOF
 
-- 
1.7.0.1

[PATCH 2/3] build: fix DSO dependencies

[Felipe Contreras]

At least on Fedora 13, this doesn't link; the linker finds the
dependencies, and aborts saying we should include them.

/usr/bin/ld: gmime-filter-reply.o: undefined reference to symbol 'g_mime_filter_set_size'
/usr/bin/ld: note: 'g_mime_filter_set_size' is defined in DSO /usr/lib/libgmime-2.6.so.0 so try adding it to the linker command line
/usr/lib/libgmime-2.6.so.0: could not read symbols: Invalid operation

We do need to link at least to what we really use, the linker resolves
the dependencies of our dependencies at loading time. So let's only
specify what we use directly.

For more information, see:
https://fedoraproject.org/wiki/UnderstandingDSOLinkChange

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

NOTE: I'm not sure about using $(CC) as a linker in !linux platforms, but if
that doesn't work, there's already an 'ifeq' that checks for that so it can be
moved there.

 Makefile.local |    3 ---
 configure      |   11 +++++------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/Makefile.local b/Makefile.local
index bc61a3c..cc8b23b 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -33,10 +33,7 @@ FINAL_CFLAGS = -DNOTMUCH_VERSION=$(VERSION) $(CFLAGS) $(WARN_CFLAGS) $(CONFIGURE
 FINAL_CXXFLAGS = $(CXXFLAGS) $(WARN_CXXFLAGS) $(CONFIGURE_CXXFLAGS) $(extra_cflags) $(extra_cxxflags)
 FINAL_NOTMUCH_LDFLAGS = $(LDFLAGS) -Llib -lnotmuch
 FINAL_NOTMUCH_LINKER = CC
-ifneq ($(LINKER_RESOLVES_LIBRARY_DEPENDENCIES),1)
 FINAL_NOTMUCH_LDFLAGS += $(CONFIGURE_LDFLAGS)
-FINAL_NOTMUCH_LINKER = CXX
-endif
 ifeq ($(PLATFORM),LINUX)
 ifeq ($(LIBDIR_IN_LDCONFIG),0)
 FINAL_NOTMUCH_LDFLAGS += -Wl,--enable-new-dtags -Wl,-rpath,$(libdir)
diff --git a/configure b/configure
index 1eb4785..ff775f0 100755
--- a/configure
+++ b/configure
@@ -255,7 +255,11 @@ for gmimepc in gmime-2.6 gmime-2.4; do
 	printf "Yes ($gmimepc).\n"
 	have_gmime=1
 	gmime_cflags=$(pkg-config --cflags $gmimepc)
-	gmime_ldflags=$(pkg-config --libs $gmimepc)
+	if [ $linker_resolves_library_dependencies = "1" ]; then
+		gmime_ldflags="-lgmime-2.6 -lgobject-2.0 -lglib-2.0"
+	else
+		gmime_ldflags=$(pkg-config --libs $gmimepc)
+	fi
     fi
 done
 if [ "$have_gmime" = "0" ]; then
@@ -481,11 +485,6 @@ HAVE_STRCASESTR = ${have_strcasestr}
 # Supported platforms (so far) are: LINUX, MACOSX, SOLARIS
 PLATFORM = ${platform}
 
-# Whether the linker will automatically resolve the dependency of one
-# library on another (if not, then linking a binary requires linking
-# directly against both)
-LINKER_RESOLVES_LIBRARY_DEPENDENCIES = ${linker_resolves_library_dependencies}
-
 # Flags needed to compile and link against Xapian
 XAPIAN_CXXFLAGS = ${xapian_cxxflags}
 XAPIAN_LDFLAGS = ${xapian_ldflags}
-- 
1.7.0.1

[PATCH 3/3] configure: optimize uname finding a bit

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 configure |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index ff775f0..3b4df37 100755
--- a/configure
+++ b/configure
@@ -198,15 +198,16 @@ errors=0
 libdir_in_ldconfig=0
 
 printf "Checking which platform we are on... "
-if [ `uname` = "Darwin" ] ; then
+uname=`uname`
+if [ $uname = "Darwin" ] ; then
     printf "Mac OS X.\n"
     platform=MACOSX
     linker_resolves_library_dependencies=0
-elif [ `uname` = "SunOS" ] ; then
+elif [ $uname = "SunOS" ] ; then
     printf "Solaris.\n"
     platform=SOLARIS
     linker_resolves_library_dependencies=0
-elif [ `uname` = "Linux" ] ; then
+elif [ $uname = "Linux" ] ; then
     printf "Linux\n"
     platform=LINUX
     linker_resolves_library_dependencies=1
-- 
1.7.0.1

Re: [PATCH 0/3] build fixes

[Felipe Contreras]

Hi,

Is anything wrong with these patches?

On Sat, Jun 5, 2010 at 2:05 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> The important one is #2, which allows to build on Fedora 13.
>
> Felipe Contreras (3):
>  configure: check platform beforehand
>  build: fix DSO dependencies
>  configure: optimize uname finding a bit
>
>  Makefile.local |    3 --
>  configure      |   74 ++++++++++++++++++++++++++++----------------------------
>  2 files changed, 37 insertions(+), 40 deletions(-)

-- 
Felipe Contreras

Re: [PATCH 0/3] build fixes

[Dirk Hohndel]

On Mon, 14 Jun 2010 17:17:00 +0300, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> Hi,
> 
> Is anything wrong with these patches?

cworth - our fearless leader - is extremely busy with his day job right
now. He tends to respond to all the patches on the mailing list as soon
as he finds time; sometimes within minutes, sometimes it takes weeks... 
you appear to have hit one of the slow spots.

/D

-- 
Dirk Hohndel
Intel Open Source Technology Center

Re: [PATCH 0/3] build fixes

[Felipe Contreras]

On Tue, Jun 15, 2010 at 10:34 PM, Dirk Hohndel <hohndel@infradead.org> wrote:
> On Mon, 14 Jun 2010 17:17:00 +0300, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> Is anything wrong with these patches?
>
> cworth - our fearless leader - is extremely busy with his day job right
> now. He tends to respond to all the patches on the mailing list as soon
> as he finds time; sometimes within minutes, sometimes it takes weeks...
> you appear to have hit one of the slow spots.

Or months.

However, I don't see what prevents other people from commenting.
Considering this notmuch is not currently building on Fedora 13, and
people have reported such issue in the mailing list, I think this
should be high priority. Besides, it's only a matter of time before
other distributions start failing to compile too.

-- 
Felipe Contreras

Re: [PATCH 2/3] build: fix DSO dependencies

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]

On Sat,  5 Jun 2010 14:05:14 +0300, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> At least on Fedora 13, this doesn't link; the linker finds the
> dependencies, and aborts saying we should include them.
...
> We do need to link at least to what we really use, the linker resolves
> the dependencies of our dependencies at loading time. So let's only
> specify what we use directly.

Hi Felipe,

You're certainly right that the linking was bogus. The notmuch binary
was only linking directly against libnotmuch (which in turned linked
against GMime, Xapian, and talloc). But meanwhile, the notmuch binary
is also directly using GMime and talloc so should be linking directly
against those.

> -ifneq ($(LINKER_RESOLVES_LIBRARY_DEPENDENCIES),1)
>  FINAL_NOTMUCH_LDFLAGS += $(CONFIGURE_LDFLAGS)
> -FINAL_NOTMUCH_LINKER = CXX
> -endif

But the change above causes the notmuch binary to also link directly
against Xapian, (which the binary does not use directly), so that's
undesired.

> -	gmime_ldflags=$(pkg-config --libs $gmimepc)
> +	if [ $linker_resolves_library_dependencies = "1" ]; then
> +		gmime_ldflags="-lgmime-2.6 -lgobject-2.0 -lglib-2.0"
> +	else
> +		gmime_ldflags=$(pkg-config --libs $gmimepc)
> +	fi

This part I don't understand. Why is it necessary to avoid using
pkg-config in this case? That sounds to me like a maintenance
nightmare. If the pkg-config information for GMime is wrong then we
should get that fixed, and not workaround it.

So, finally, I implemented a much more narrow fix for the linking
problem, (simply adding $(GMIME_LDFLAGS) and $(TALLOC_LDFLAGS) to the
FINAL_NOTMUCH_LDFLAGS assignement).

I tested this by installing binutils-gold on my Debian system. This
caused a compilation failure before my patch, but compilation succeeds
after my patch. I'm optimistic that this means that a Fedora compilation
will work as well now. Can you test that please?

-Carl

PS. For the other two patches you sent. I couldn't see that #1 (moving
the platform-detection earlier in configure) is necessary. But #3 seems
obviously correct, so I've pushed that now.

I do apologize that it has been many months since you posted these
patches, and that you got no review of them until now. But thanks indeed
for sending them!

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] build: fix DSO dependencies

[Felipe Contreras]

Hello,

On Sat, Oct 30, 2010 at 12:37 AM, Carl Worth <cworth@cworth.org> wrote:
> On Sat,  5 Jun 2010 14:05:14 +0300, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> At least on Fedora 13, this doesn't link; the linker finds the
>> dependencies, and aborts saying we should include them.
> ...
>> We do need to link at least to what we really use, the linker resolves
>> the dependencies of our dependencies at loading time. So let's only
>> specify what we use directly.
>
> You're certainly right that the linking was bogus. The notmuch binary
> was only linking directly against libnotmuch (which in turned linked
> against GMime, Xapian, and talloc). But meanwhile, the notmuch binary
> is also directly using GMime and talloc so should be linking directly
> against those.
>
>> -ifneq ($(LINKER_RESOLVES_LIBRARY_DEPENDENCIES),1)
>>  FINAL_NOTMUCH_LDFLAGS += $(CONFIGURE_LDFLAGS)
>> -FINAL_NOTMUCH_LINKER = CXX
>> -endif
>
> But the change above causes the notmuch binary to also link directly
> against Xapian, (which the binary does not use directly), so that's
> undesired.

Yes, I wanted to fix that in subsequent patches, but my first
objective was to get it to build.

>> -     gmime_ldflags=$(pkg-config --libs $gmimepc)
>> +     if [ $linker_resolves_library_dependencies = "1" ]; then
>> +             gmime_ldflags="-lgmime-2.6 -lgobject-2.0 -lglib-2.0"
>> +     else
>> +             gmime_ldflags=$(pkg-config --libs $gmimepc)
>> +     fi
>
> This part I don't understand. Why is it necessary to avoid using
> pkg-config in this case? That sounds to me like a maintenance
> nightmare. If the pkg-config information for GMime is wrong then we
> should get that fixed, and not workaround it.

Well, it's not possible: pkg-config is supposed to work on win32 and
osx, so all the dependencies must be there, so:

% pkg-config --libs gmime-2.6
-pthread -lgmime-2.6 -lgio-2.0 -lgobject-2.0 -lgmodule-2.0
-lgthread-2.0 -lrt -lglib-2.0

Means we would be linking to many libraries we are not going to use directly.

Fortunately, I found the solution after writing that patch:
-Wl,--as-needed. With this the linker would automatically figure out
that we actually want to link only to -lgmime-2.6 -lgobject-2.0
-lglib-2.0.

> So, finally, I implemented a much more narrow fix for the linking
> problem, (simply adding $(GMIME_LDFLAGS) and $(TALLOC_LDFLAGS) to the
> FINAL_NOTMUCH_LDFLAGS assignement).
>
> I tested this by installing binutils-gold on my Debian system. This
> caused a compilation failure before my patch, but compilation succeeds
> after my patch. I'm optimistic that this means that a Fedora compilation
> will work as well now. Can you test that please?

Patch #1 is not needed if gmime_ldflags in patch #2 are not changed
conditionally, which can be achieved by --as-needed.

I just sent my proposed updated patches.

-- 
Felipe Contreras

Re: [PATCH 2/3] build: fix DSO dependencies

[Jed Brown]

On Fri, Oct 29, 2010 at 18:17, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Well, it's not possible: pkg-config is supposed to work on win32 and
> osx, so all the dependencies must be there, so:
>
> % pkg-config --libs gmime-2.6
> -pthread -lgmime-2.6 -lgio-2.0 -lgobject-2.0 -lgmodule-2.0
> -lgthread-2.0 -lrt -lglib-2.0

This is what the Libs.private field is for (see the pkg-config man page).  E.g.

$ pkg-config --libs libavdevice
-lavdevice
$ pkg-config --libs --static libavdevice
-pthread -lavdevice -lavformat -lavcodec -ldl -lX11 -lXext -lXfixes
-lasound -lxvidcore -lx264 -lvpx -lvorbisenc -lvorbis -ltheoraenc
-ltheoradec -logg -lschroedinger-1.0 -lpthread -lorc-0.4 -lopenjpeg
-lopencore-amrwb -lopencore-amrnb -lmp3lame -lfaac -lva -lm -lbz2 -lz
-lavcore -lavutil

Jed