Build problems on OS X

Build problems on OS X

[Aaron Ecay]

In the process of updating to the latest sources, I've discovered that notmuch
no longer builds on OS X.  As a reply to this email, I'll be sending 4
patches.  The first two are bugfixes; the third adds the infrastructure to the
Makefiles to build a shared library on OS X, since the Mach-O conventions are
somewhat different than the Linux ones.  Modulo makefile style quibbles, this
patch should be correct.

The fourth patch I am not sure of.  Even after adding the proper command line
flags to build the shared binary on OS X, I get symbol not found errors for
Glib/Gmime symbols.  The shared binary links against the shared lib, which in
turn links against Glib and Gmime, but it appears that at least on OS X the
linker won't follow these second-order links, and the notmuch shared binary
must be linked against Glib/Gmime directly.  This patch fixes the build for
me, but it may not be correct on Linux/other Unices.  I can resubmit this
patch to add the extra linker flags only when building for OS X.  If anyone
knows how to get the OS X linker to behave like the Linux one in this regard,
that will likely be a better solution.

Aaron

[PATCH 1/4] Use C++ compiler to link notmuch binaries

[Aaron Ecay]

Since the binaries contain C++ code, it is necessary to use the C++
linker, or errors result on some platforms (OS X).

Signed-off-by: Aaron Ecay <aaronecay@gmail.com>
---
 Makefile.local |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile.local b/Makefile.local
index 71525e2..9e753cd 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -216,10 +216,10 @@ notmuch_client_srcs =		\
 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
 
 notmuch: $(notmuch_client_modules) lib/libnotmuch.a
-	$(call quiet,CC $(CFLAGS)) $^ $(FINAL_LIBNOTMUCH_LDFLAGS) -o $@
+	$(call quiet,CXX $(CFLAGS)) $^ $(FINAL_LIBNOTMUCH_LDFLAGS) -o $@
 
 notmuch-shared: $(notmuch_client_modules) lib/libnotmuch.so
-	$(call quiet,CC $(CFLAGS)) $(notmuch_client_modules) $(FINAL_NOTMUCH_LDFLAGS) -o $@
+	$(call quiet,CXX $(CFLAGS)) $(notmuch_client_modules) $(FINAL_NOTMUCH_LDFLAGS) -o $@
 
 notmuch.1.gz: notmuch.1
 	gzip --stdout $^ > $@
-- 
1.7.0.4

Re: [PATCH 1/4] Use C++ compiler to link notmuch binaries

[Carl Worth]

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

On Sun, 11 Apr 2010 19:44:51 -0400, Aaron Ecay <aaronecay@gmail.com> wrote:
> Since the binaries contain C++ code, it is necessary to use the C++
> linker, or errors result on some platforms (OS X).

Thanks. This one is merged and pushed.

-Carl

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

[PATCH 2/4] Fix up Makefile for build.

[Aaron Ecay]

Must set extra_c(xx)flags before including subdir Makefile.local's,
so that there is a blank slate that the subdirs can add on to.

Must include subdir Makefile.local's before global one, otherwise
the compat sources are not added to the list of those to be
compiled.

Signed-off-by: Aaron Ecay <aaronecay@gmail.com>
---
 Makefile              |    9 ++++++++-
 Makefile.local        |    5 -----
 compat/Makefile.local |    2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index 076efc7..faaaec6 100644
--- a/Makefile
+++ b/Makefile
@@ -9,6 +9,13 @@ subdirs = compat completion emacs lib
 global_deps = Makefile Makefile.local \
 	$(subdirs:%=%/Makefile) $(subdirs:%=%/Makefile.local)
 
+# Sub-directory Makefile.local fragments can append to these variables
+# to have directory-specific cflags as necessary.
+
+extra_cflags :=
+extra_cxxflags :=
+
 # Finally, include all of the Makefile.local fragments where all the
 # real work is done.
-include Makefile.local $(subdirs:%=%/Makefile.local)
+
+include $(subdirs:%=%/Makefile.local) Makefile.local
diff --git a/Makefile.local b/Makefile.local
index 9e753cd..c04044c 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -33,11 +33,6 @@ Makefile.config: configure
 	@echo ""
 	./configure
 
-# Sub-directory Makefile.local fragments can append to these variables
-# to have directory-specific cflags as necessary.
-extra_cflags :=
-extra_cxxflags :=
-
 # Smash together user's values with our extra values
 FINAL_CFLAGS = -DNOTMUCH_VERSION=$(VERSION) $(CFLAGS) $(WARN_CFLAGS) $(CONFIGURE_CFLAGS) $(extra_cflags)
 FINAL_CXXFLAGS = $(CXXFLAGS) $(WARN_CXXFLAGS) $(CONFIGURE_CXXFLAGS) $(extra_cflags) $(extra_cxxflags)
diff --git a/compat/Makefile.local b/compat/Makefile.local
index 81e6c70..50f6cd1 100644
--- a/compat/Makefile.local
+++ b/compat/Makefile.local
@@ -3,7 +3,7 @@
 dir := compat
 extra_cflags += -I$(dir)
 
-notmuch_compat_srcs =
+notmuch_compat_srcs :=
 
 ifneq ($(HAVE_GETLINE),1)
 notmuch_compat_srcs += $(dir)/getline.c $(dir)/getdelim.c
-- 
1.7.0.4

Re: [PATCH 2/4] Fix up Makefile for build.

[Carl Worth]

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

On Sun, 11 Apr 2010 19:44:52 -0400, Aaron Ecay <aaronecay@gmail.com> wrote:
> Must set extra_c(xx)flags before including subdir Makefile.local's,
> so that there is a blank slate that the subdirs can add on to.

That part looks just fine, but it's intermixed with:

> Must include subdir Makefile.local's before global one, otherwise
> the compat sources are not added to the list of those to be
> compiled.

This part, which seems not ideal. I would have preferred it if we could
have avoided fiddly ordering issues with our Makefile includes. The
concept is that we are writing a single, flat Makefile but just
maintaining it in pieces. And traditional Makefile rules don't depend on
ordering so much. But the change of this variable assignment:

But the patch switches to an order-dependent variable assignment for
notmuch_compat_srcs here:

> -notmuch_compat_srcs =
> +notmuch_compat_srcs :=

And testing this a bit, it does seem necessary to do this since we are
already using the order-dependent $(dir) variable in the expansion of
notmuch_compat_srcs.

So I've convinced myself, and pushed this patch as well. (I even tested
by manually setting HAVE_GETLINE to 0 in Makefile.config)

-Carl

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

[PATCH 3/4] Add infrastructure for building shared library on OS X.

[Aaron Ecay]

This patch adds a configure check for OS X (actually Darwin),
and sets up the Makefiles to build a proper shared library on
that platform.

Signed-off-by: Aaron Ecay <aaronecay@gmail.com>
---
 Makefile           |    2 +-
 Makefile.local     |    2 +-
 configure          |   13 +++++++++++++
 lib/Makefile.local |   16 ++++++++++++++--
 4 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index faaaec6..86e08fa 100644
--- a/Makefile
+++ b/Makefile
@@ -18,4 +18,4 @@ extra_cxxflags :=
 # Finally, include all of the Makefile.local fragments where all the
 # real work is done.
 
-include $(subdirs:%=%/Makefile.local) Makefile.local
+include Makefile.config $(subdirs:%=%/Makefile.local) Makefile.local
diff --git a/Makefile.local b/Makefile.local
index c04044c..99d5b64 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -213,7 +213,7 @@ notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
 notmuch: $(notmuch_client_modules) lib/libnotmuch.a
 	$(call quiet,CXX $(CFLAGS)) $^ $(FINAL_LIBNOTMUCH_LDFLAGS) -o $@
 
-notmuch-shared: $(notmuch_client_modules) lib/libnotmuch.so
+notmuch-shared: $(notmuch_client_modules) lib/$(LINKER_NAME)
 	$(call quiet,CXX $(CFLAGS)) $(notmuch_client_modules) $(FINAL_NOTMUCH_LDFLAGS) -o $@
 
 notmuch.1.gz: notmuch.1
diff --git a/configure b/configure
index 5af7852..da45572 100755
--- a/configure
+++ b/configure
@@ -234,6 +234,15 @@ else
     have_emacs=0
 fi
 
+printf "Checking for Mac OS X (for shared library)... "
+if [ `uname` = "Darwin" ] ; then
+    printf "Yes.\n"
+    mac_os_x=1
+else
+    printf "No.\n"
+    mac_os_x=0
+fi
+
 if [ $errors -gt 0 ]; then
     cat <<EOF
 
@@ -384,6 +393,10 @@ zsh_completion_dir = \$(prefix)/share/zsh/functions/Completion/Unix
 # build its own version)
 HAVE_GETLINE = ${have_getline}
 
+# Whether we are building on OS X.  This will affect how we build the
+# shared library.
+MAC_OS_X = ${mac_os_x}
+
 # Flags needed to compile and link against Xapian
 XAPIAN_CXXFLAGS = ${xapian_cxxflags}
 XAPIAN_LDFLAGS = ${xapian_ldflags}
diff --git a/lib/Makefile.local b/lib/Makefile.local
index 0e3a4d1..7e73810 100644
--- a/lib/Makefile.local
+++ b/lib/Makefile.local
@@ -22,14 +22,26 @@ LIBNOTMUCH_VERSION_MINOR = 0
 # simply compatible changes to the implementation).
 LIBNOTMUCH_VERSION_RELEASE = 0
 
-LINKER_NAME = libnotmuch.so
+ifeq ($(MAC_OS_X),1)
+LIBRARY_SUFFIX = dylib
+# On OS X, library version numbers go before suffix.
+LINKER_NAME = libnotmuch.$(LIBRARY_SUFFIX)
+SONAME = libnotmuch.$(LIBNOTMUCH_VERSION_MAJOR).$(LIBRARY_SUFFIX)
+LIBNAME = libnotmuch.$(LIBNOTMUCH_VERSION_MAJOR).$(LIBNOTMUCH_VERSION_MINOR).$(LIBNOTMUCH_VERSION_RELEASE).$(LIBRARY_SUFFIX)
+LIBRARY_LINK_FLAG = -Wl,-dylib_install_name -Wl,$(SONAME)
+else
+LIBRARY_SUFFIX = so
+LINKER_NAME = libnotmuch.$(LIBRARY_SUFFIX)
 SONAME = $(LINKER_NAME).$(LIBNOTMUCH_VERSION_MAJOR)
 LIBNAME = $(SONAME).$(LIBNOTMUCH_VERSION_MINOR).$(LIBNOTMUCH_VERSION_RELEASE)
+LIBRARY_LINK_FLAG = -Wl,-soname=$(SONAME)
+endif
 
 dir := lib
 extra_cflags += -I$(dir) -fPIC
 
 libnotmuch_c_srcs =		\
+	$(notmuch_compat_srcs)  \
 	$(dir)/libsha1.c	\
 	$(dir)/message-file.c	\
 	$(dir)/messages.c	\
@@ -51,7 +63,7 @@ $(dir)/libnotmuch.a: $(libnotmuch_modules)
 	$(call quiet,AR) rcs $@ $^
 
 $(dir)/$(LIBNAME): $(libnotmuch_modules)
-	$(call quiet,CXX $(CXXFLAGS)) $^ $(FINAL_LIBNOTMUCH_LDFLAGS) -shared -Wl,-soname=$(SONAME) -o $@
+	$(call quiet,CXX $(CXXFLAGS)) $^ $(FINAL_LIBNOTMUCH_LDFLAGS) -shared $(LIBRARY_LINK_FLAG) -o $@
 
 $(dir)/$(SONAME): $(dir)/$(LIBNAME)
 	ln -sf $(LIBNAME) $@
-- 
1.7.0.4

Re: [PATCH 3/4] Add infrastructure for building shared library on OS X.

[Carl Worth]

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

On Sun, 11 Apr 2010 19:44:53 -0400, Aaron Ecay <aaronecay@gmail.com> wrote:
> This patch adds a configure check for OS X (actually Darwin),
> and sets up the Makefiles to build a proper shared library on
> that platform.
...
> -include $(subdirs:%=%/Makefile.local) Makefile.local
> +include Makefile.config $(subdirs:%=%/Makefile.local) Makefile.local

This first hunk looks unrelated to what's described in the commit
message.

It also results in Makefile.config being included from both the toplevel
Makefile and the toplevel Makefile.local, so that seems wrong.

I had wanted to keep the top-level Makefile totally generic, (so that if
a project wanted to imitate the notmuch flat-Makefile build system that
would be easy). And perhaps including Makefile.config here violates
that.

But I'd be willing to accept that if necessary---should just remove the
include of Makefile.config from Makefile.local then I think.

> +printf "Checking for Mac OS X (for shared library)... "
> +if [ `uname` = "Darwin" ] ; then
> +    printf "Yes.\n"
> +    mac_os_x=1
> +else
> +    printf "No.\n"
> +    mac_os_x=0
> +fi
> +

Instead of inventing a new mac_os_x variable, we should follow the GNU
configure conventions of build_cpu, build_vendor, build_os
variables. We're already allowing the user to assign to these by passing
a --build option to configure (though not yet doing anything with the
values).

But now that you've got something you actually do want to do with the
values, we should use those same variables. It might not even be crazy
to copy in config.guess (or pieces of it). Though, frankly, it's not
doing anything for Darwin unlike you're doing above, so you might as
well just use your own code as you have.

>  libnotmuch_c_srcs =		\
> +	$(notmuch_compat_srcs)  \
>  	$(dir)/libsha1.c	\
>  	$(dir)/message-file.c	\
>  	$(dir)/messages.c	\

This again looks like an independent fix that should be in a separate
commit.

-Carl

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

Re: [PATCH 3/4] Add infrastructure for building shared library on OS X.

[Carl Worth]

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

On Wed, 14 Apr 2010 11:01:58 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Sun, 11 Apr 2010 19:44:53 -0400, Aaron Ecay <aaronecay@gmail.com> wrote:
> > -include $(subdirs:%=%/Makefile.local) Makefile.local
> > +include Makefile.config $(subdirs:%=%/Makefile.local) Makefile.local
> 
> This first hunk looks unrelated to what's described in the commit
> message.

Indeed, on further testing this looks like it should have been part of
the previous commit that includes the sub-directory Makefile.local files
before the top-level Makefile.local. Without this piece, I was seeing
the various compat/*.c files being compiled unconditionally.

> But I'd be willing to accept that if necessary---should just remove the
> include of Makefile.config from Makefile.local then I think.

I've gone ahead and done that now. I moved everything concerning
Makefile.config, (dependency, include, and generation), from
Makefile.local up to the top-level Makefile.

-Carl

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

Re: [PATCH 3/4] Add infrastructure for building shared library on OS X.

[Carl Worth]

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

On Wed, 14 Apr 2010 11:01:58 -0700, Carl Worth <cworth@cworth.org> wrote:
> > +printf "Checking for Mac OS X (for shared library)... "
> > +if [ `uname` = "Darwin" ] ; then
> > +    printf "Yes.\n"
> > +    mac_os_x=1
> > +else
> > +    printf "No.\n"
> > +    mac_os_x=0
> > +fi
> > +
> 
> Instead of inventing a new mac_os_x variable, we should follow the GNU
> configure conventions of build_cpu, build_vendor, build_os
> variables.

I've gone ahead and pushed the above. We can still fix to use build_os
internally instead of mac_os_x, but that's fairly cosmetic.

I also pushed out my own version of the fix for FINAL_NOTMUCH_LDFLAGS,
(which does the extra linking on OS X, but not Linux).

Please, anyone that's interested, test notmuch on your favorite
platforms and let us know what's still broken. I did do a little testing
and research with the OS X machine I could find here, but I'm no expert,
nor do I have access to many other systems.

-Carl

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

[PATCH 4/4] Add CONFIGURE_LDFLAGS to the notmuch-shared buld command line.

[Aaron Ecay]

Otherwise, symbol not found errors result on OS X.  I am not sure
this is the correct solution for the problem, but it gets the build
working.

Signed-off-by: Aaron Ecay <aaronecay@gmail.com>
---
 Makefile.local |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile.local b/Makefile.local
index 99d5b64..8d5fda4 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -36,7 +36,7 @@ Makefile.config: configure
 # Smash together user's values with our extra values
 FINAL_CFLAGS = -DNOTMUCH_VERSION=$(VERSION) $(CFLAGS) $(WARN_CFLAGS) $(CONFIGURE_CFLAGS) $(extra_cflags)
 FINAL_CXXFLAGS = $(CXXFLAGS) $(WARN_CXXFLAGS) $(CONFIGURE_CXXFLAGS) $(extra_cflags) $(extra_cxxflags)
-FINAL_NOTMUCH_LDFLAGS = $(LDFLAGS) -Llib -lnotmuch
+FINAL_NOTMUCH_LDFLAGS = $(LDFLAGS) -Llib -lnotmuch $(CONFIGURE_LDFLAGS)
 FINAL_LIBNOTMUCH_LDFLAGS = $(LDFLAGS) $(CONFIGURE_LDFLAGS)
 
 .PHONY: all
-- 
1.7.0.4

Re: [PATCH 4/4] Add CONFIGURE_LDFLAGS to the notmuch-shared buld command line.

[Carl Worth]

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

On Sun, 11 Apr 2010 19:44:54 -0400, Aaron Ecay <aaronecay@gmail.com> wrote:
> Otherwise, symbol not found errors result on OS X.  I am not sure
> this is the correct solution for the problem, but it gets the build
> working.
...
> -FINAL_NOTMUCH_LDFLAGS = $(LDFLAGS) -Llib -lnotmuch
> +FINAL_NOTMUCH_LDFLAGS = $(LDFLAGS) -Llib -lnotmuch $(CONFIGURE_LDFLAGS)
>  FINAL_LIBNOTMUCH_LDFLAGS = $(LDFLAGS) $(CONFIGURE_LDFLAGS)

As I mentioned earlier in the thread, this isn't the correct solution
for Linux. Previously we had just FINAL_LDFLAGS that were used for both
the binary and the library. We split this into FINAL_NOTMUCH_LDFLAGS and
FINAL_LIBNOTMUCH_LDFLAGS precisely to avoid excess library dependencies
on Linux.

So the above patch would undo that which is not what we want.

-Carl

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

Re: Build problems on OS X

[Carl Worth]

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

On Sun, 11 Apr 2010 19:43:27 -0400, Aaron Ecay <aaronecay@gmail.com> wrote:
> In the process of updating to the latest sources, I've discovered that notmuch
> no longer builds on OS X.

Hi Aaron,

Thanks so much for following up here. This transition to
building/installing a shared library (and not using libtool) is the big
one as for as build-system portability goes. So I did expect to see a
patch series here. I'm optimistic that going forward, we won't have
constant breakage.

Though I did have a suggestion to call ldconfig implicitly when doing
"make install". Is that the same command on OS X or is it different?

> As a reply to this email, I'll be sending 4 patches.

I'll follow up to some of these individually.

> The fourth patch I am not sure of.  Even after adding the proper command line
> flags to build the shared binary on OS X, I get symbol not found errors for
> Glib/Gmime symbols.  The shared binary links against the shared lib, which in
> turn links against Glib and Gmime, but it appears that at least on OS X the
> linker won't follow these second-order links, and the notmuch shared binary
> must be linked against Glib/Gmime directly.  This patch fixes the build for
> me, but it may not be correct on Linux/other Unices.

You're correct that it is incorrect on Linux. :-)

Debian's program for checking package correctness (lintian) complains if
a binary is linked directly against a library when the binary doesn't
actually reference any symbols from that library, (but instead
references symbols from other libraries that in turn reference the
library). So we do want to avoid this excess linking.

> I can resubmit this patch to add the extra linker flags only when
> building for OS X.  If anyone knows how to get the OS X linker to
> behave like the Linux one in this regard, that will likely be a better
> solution.

Reworking that patch would be great. I don't have any suggestion for
fixing the linker though. :-P

-Carl



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