From: Carl Worth <cworth@cworth.org>
To: Aaron Ecay <aaronecay@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH 3/4] Add infrastructure for building shared library on OS
	X.
In-Reply-To: <1271029494-89014-3-git-send-email-aaronecay@gmail.com>
References: <4bc25ea0.86c3f10a.45a3.ffff80d3@mx.google.com>
	<1271029494-89014-3-git-send-email-aaronecay@gmail.com>
Date: Wed, 14 Apr 2010 11:01:58 -0700
Message-ID: <87sk6xgbft.fsf@yoom.home.cworth.org>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Precedence: list

--=-=-=

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

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iD8DBQFLxgMW6JDdNq8qSWgRAmONAKCmj4z2S7S8fAUxDGUUg4FT0B3k+ACgkLEY
4do3So28H0s+ueVR7DuRkQE=
=nVeg
-----END PGP SIGNATURE-----
--=-=-=--