[PATCH 1/2] Makefile: use curl-config to determine curl flags

[PATCH 1/2] Makefile: use curl-config to determine curl flags

[Dave Borowitz]

curl-config should always be installed alongside a curl distribution,
and its purpose is to provide flags for building against libcurl, so
use it instead of guessing flags and dependent libraries.

Allow overriding CURL_CONFIG to a custom path to curl-config, to
compile against a curl installation other than the first in PATH.

Use this only when CURLDIR is not explicitly specified, to continue
supporting older builds.

Signed-off-by: Dave Borowitz <dborowitz@google.com>
---
 Makefile | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/Makefile b/Makefile
index 2128ce3..d6330bc 100644
--- a/Makefile
+++ b/Makefile
@@ -34,8 +34,12 @@ all::
 # git-http-push are not built, and you cannot use http:// and https://
 # transports (neither smart nor dumb).
 #
+# Define CURL_CONFIG to the path to a curl-config binary other than the
+# default 'curl-config'.
+#
 # Define CURLDIR=/foo/bar if your curl header and library files are in
-# /foo/bar/include and /foo/bar/lib directories.
+# /foo/bar/include and /foo/bar/lib directories.  This overrides CURL_CONFIG,
+# but is less robust.
 #
 # Define NO_EXPAT if you do not have expat installed.  git-http-push is
 # not built, and you cannot push using http:// and https:// transports (dumb).
@@ -143,9 +147,11 @@ all::
 #
 # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
 #
-# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).
+# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).  Only used
+# if CURLDIR is set.
 #
-# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).
+# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).  Only
+# used if CURLDIR is set.
 #
 # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin).
 #
@@ -1121,18 +1127,23 @@ else
 		# Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
 		BASIC_CFLAGS += -I$(CURLDIR)/include
 		CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl
+		ifdef NEEDS_SSL_WITH_CURL
+			CURL_LIBCURL += -lssl
+			ifdef NEEDS_CRYPTO_WITH_SSL
+				CURL_LIBCURL += -lcrypto
+			endif
+		endif
+		ifdef NEEDS_IDN_WITH_CURL
+			CURL_LIBCURL += -lidn
+		endif
 	else
-		CURL_LIBCURL = -lcurl
-	endif
-	ifdef NEEDS_SSL_WITH_CURL
-		CURL_LIBCURL += -lssl
-		ifdef NEEDS_CRYPTO_WITH_SSL
-			CURL_LIBCURL += -lcrypto
+		CURL_CONFIG ?= curl-config
+		BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags)
+		CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs)
+		ifeq "$(CURL_LIBCURL)" ""
+			$(error curl not detected; try setting CURLDIR)
 		endif
 	endif
-	ifdef NEEDS_IDN_WITH_CURL
-		CURL_LIBCURL += -lidn
-	endif
 
 	REMOTE_CURL_PRIMARY = git-remote-http$X
 	REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$X
-- 
1.9.1.423.g4596e3a

[PATCH 2/2] Makefile: allow static linking against libcurl

[Dave Borowitz]

Signed-off-by: Dave Borowitz <dborowitz@google.com>
---
 Makefile | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index d6330bc..3c151d3 100644
--- a/Makefile
+++ b/Makefile
@@ -37,6 +37,9 @@ all::
 # Define CURL_CONFIG to the path to a curl-config binary other than the
 # default 'curl-config'.
 #
+# Define CURL_STATIC to statically link libcurl.  Only applies if
+# CURL_CONFIG is used.
+#
 # Define CURLDIR=/foo/bar if your curl header and library files are in
 # /foo/bar/include and /foo/bar/lib directories.  This overrides CURL_CONFIG,
 # but is less robust.
@@ -1139,7 +1142,11 @@ else
 	else
 		CURL_CONFIG ?= curl-config
 		BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags)
-		CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs)
+		ifdef CURL_STATIC
+			CURL_LIBCURL = $(shell $(CURL_CONFIG) --static-libs)
+		else
+			CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs)
+		endif
 		ifeq "$(CURL_LIBCURL)" ""
 			$(error curl not detected; try setting CURLDIR)
 		endif
-- 
1.9.1.423.g4596e3a

Re: [PATCH 1/2] Makefile: use curl-config to determine curl flags

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> curl-config should always be installed alongside a curl distribution,
> and its purpose is to provide flags for building against libcurl, so
> use it instead of guessing flags and dependent libraries.
>
> Allow overriding CURL_CONFIG to a custom path to curl-config, to
> compile against a curl installation other than the first in PATH.
>
> Use this only when CURLDIR is not explicitly specified, to continue
> supporting older builds.
>
> Signed-off-by: Dave Borowitz <dborowitz@google.com>

Sounds logically the right thing to do.  Was there a particular
platform that needed this (i.e. cannot be made to work with the
existing CURLDIR and "guessing flags and dependeent libraries")
that may be worth mentioning in the log message?

> ---
>  Makefile | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 2128ce3..d6330bc 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -34,8 +34,12 @@ all::
>  # git-http-push are not built, and you cannot use http:// and https://
>  # transports (neither smart nor dumb).
>  #
> +# Define CURL_CONFIG to the path to a curl-config binary other than the
> +# default 'curl-config'.
> +#
>  # Define CURLDIR=/foo/bar if your curl header and library files are in
> -# /foo/bar/include and /foo/bar/lib directories.
> +# /foo/bar/include and /foo/bar/lib directories.  This overrides CURL_CONFIG,
> +# but is less robust.
>  #
>  # Define NO_EXPAT if you do not have expat installed.  git-http-push is
>  # not built, and you cannot push using http:// and https:// transports (dumb).
> @@ -143,9 +147,11 @@ all::
>  #
>  # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
>  #
> -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).
> +# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).  Only used
> +# if CURLDIR is set.
>  #
> -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).
> +# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).  Only
> +# used if CURLDIR is set.
>  #
>  # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin).
>  #
> @@ -1121,18 +1127,23 @@ else
>  		# Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
>  		BASIC_CFLAGS += -I$(CURLDIR)/include
>  		CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl
> +		ifdef NEEDS_SSL_WITH_CURL
> +			CURL_LIBCURL += -lssl
> +			ifdef NEEDS_CRYPTO_WITH_SSL
> +				CURL_LIBCURL += -lcrypto
> +			endif
> +		endif
> +		ifdef NEEDS_IDN_WITH_CURL
> +			CURL_LIBCURL += -lidn
> +		endif
>  	else
> -		CURL_LIBCURL = -lcurl
> -	endif
> -	ifdef NEEDS_SSL_WITH_CURL
> -		CURL_LIBCURL += -lssl
> -		ifdef NEEDS_CRYPTO_WITH_SSL
> -			CURL_LIBCURL += -lcrypto
> +		CURL_CONFIG ?= curl-config
> +		BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags)
> +		CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs)
> +		ifeq "$(CURL_LIBCURL)" ""
> +			$(error curl not detected; try setting CURLDIR)
>  		endif
>  	endif
> -	ifdef NEEDS_IDN_WITH_CURL
> -		CURL_LIBCURL += -lidn
> -	endif
>  
>  	REMOTE_CURL_PRIMARY = git-remote-http$X
>  	REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$X

Re: [PATCH 1/2] Makefile: use curl-config to determine curl flags

[Dave Borowitz]

On Mon, Apr 14, 2014 at 4:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dave Borowitz <dborowitz@google.com> writes:
>
>> curl-config should always be installed alongside a curl distribution,
>> and its purpose is to provide flags for building against libcurl, so
>> use it instead of guessing flags and dependent libraries.
>>
>> Allow overriding CURL_CONFIG to a custom path to curl-config, to
>> compile against a curl installation other than the first in PATH.
>>
>> Use this only when CURLDIR is not explicitly specified, to continue
>> supporting older builds.
>>
>> Signed-off-by: Dave Borowitz <dborowitz@google.com>
>
> Sounds logically the right thing to do.  Was there a particular
> platform that needed this (i.e. cannot be made to work with the
> existing CURLDIR and "guessing flags and dependeent libraries")
> that may be worth mentioning in the log message?

My end goal is to statically link git on Mac OS X (10.9) against a
newer version of libcurl than ships with the OS. The normal CURLDIR
approach should work with system libcurl:

$ /usr/bin/curl-config --libs
-lcurl

But it gets a bit more complicated with a recent curl version. This
likely has to do with the set of enabled options; I passed flags to
./configure based on the build script "MacOSX-Framework" included in
the curl distribution:
$ ~/d/curl-out-7.36.0/bin/curl-config --libs
-L/Users/dborowitz/d/curl-out-7.36.0/lib -lcurl -lgssapi_krb5 -lresolv
-lldap -lz

And with --static-libs there's just that much more:
$ ~/d/curl-out-7.36.0/bin/curl-config --static-libs
/Users/dborowitz/d/curl-out-7.36.0/lib/libcurl.a
-Wl,-syslibroot,/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk
-arch x86_64 -Wl,-headerpad_max_install_names -framework
CoreFoundation -framework Security -lgssapi_krb5 -lresolv -lldap -lz

So I don't think specifying NEEDS_*_WITH_CURL scales to this use case.

While writing this up I also noticed an issue with the second patch,
namely that `curl-config --static-libs` is empty when curl is not
built for static linking.

I will reroll with a more detailed commit message and a fix to the second patch.

>> ---
>>  Makefile | 35 +++++++++++++++++++++++------------
>>  1 file changed, 23 insertions(+), 12 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 2128ce3..d6330bc 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -34,8 +34,12 @@ all::
>>  # git-http-push are not built, and you cannot use http:// and https://
>>  # transports (neither smart nor dumb).
>>  #
>> +# Define CURL_CONFIG to the path to a curl-config binary other than the
>> +# default 'curl-config'.
>> +#
>>  # Define CURLDIR=/foo/bar if your curl header and library files are in
>> -# /foo/bar/include and /foo/bar/lib directories.
>> +# /foo/bar/include and /foo/bar/lib directories.  This overrides CURL_CONFIG,
>> +# but is less robust.
>>  #
>>  # Define NO_EXPAT if you do not have expat installed.  git-http-push is
>>  # not built, and you cannot push using http:// and https:// transports (dumb).
>> @@ -143,9 +147,11 @@ all::
>>  #
>>  # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
>>  #
>> -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).
>> +# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).  Only used
>> +# if CURLDIR is set.
>>  #
>> -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).
>> +# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).  Only
>> +# used if CURLDIR is set.
>>  #
>>  # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin).
>>  #
>> @@ -1121,18 +1127,23 @@ else
>>               # Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
>>               BASIC_CFLAGS += -I$(CURLDIR)/include
>>               CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl
>> +             ifdef NEEDS_SSL_WITH_CURL
>> +                     CURL_LIBCURL += -lssl
>> +                     ifdef NEEDS_CRYPTO_WITH_SSL
>> +                             CURL_LIBCURL += -lcrypto
>> +                     endif
>> +             endif
>> +             ifdef NEEDS_IDN_WITH_CURL
>> +                     CURL_LIBCURL += -lidn
>> +             endif
>>       else
>> -             CURL_LIBCURL = -lcurl
>> -     endif
>> -     ifdef NEEDS_SSL_WITH_CURL
>> -             CURL_LIBCURL += -lssl
>> -             ifdef NEEDS_CRYPTO_WITH_SSL
>> -                     CURL_LIBCURL += -lcrypto
>> +             CURL_CONFIG ?= curl-config
>> +             BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags)
>> +             CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs)
>> +             ifeq "$(CURL_LIBCURL)" ""
>> +                     $(error curl not detected; try setting CURLDIR)
>>               endif
>>       endif
>> -     ifdef NEEDS_IDN_WITH_CURL
>> -             CURL_LIBCURL += -lidn
>> -     endif
>>
>>       REMOTE_CURL_PRIMARY = git-remote-http$X
>>       REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$X

Re: [PATCH 1/2] Makefile: use curl-config to determine curl flags

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> My end goal is to statically link git on Mac OS X (10.9) against a
> newer version of libcurl than ships with the OS. The normal CURLDIR
> approach should work with system libcurl:
>
> $ /usr/bin/curl-config --libs
> -lcurl
>
> But it gets a bit more complicated with a recent curl version. This
> likely has to do with the set of enabled options; I passed flags to
> ./configure based on the build script "MacOSX-Framework" included in
> the curl distribution:
> $ ~/d/curl-out-7.36.0/bin/curl-config --libs
> -L/Users/dborowitz/d/curl-out-7.36.0/lib -lcurl -lgssapi_krb5 -lresolv
> -lldap -lz
>
> And with --static-libs there's just that much more:
> $ ~/d/curl-out-7.36.0/bin/curl-config --static-libs
> /Users/dborowitz/d/curl-out-7.36.0/lib/libcurl.a
> -Wl,-syslibroot,/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk
> -arch x86_64 -Wl,-headerpad_max_install_names -framework
> CoreFoundation -framework Security -lgssapi_krb5 -lresolv -lldap -lz
>
> So I don't think specifying NEEDS_*_WITH_CURL scales to this use case.

Thanks.  As I said, the kosher way to learn how to link with libcURL
is by asking curl-config about the details of options to give to the
compiler and the linker, so I am all for this change.

> While writing this up I also noticed an issue with the second patch,
> namely that `curl-config --static-libs` is empty when curl is not
> built for static linking.
>
> I will reroll with a more detailed commit message and a fix to the second patch.

I love it whenever I see the contents of the patch improved after
spending a bit more time trying to describe the problem and the
solution (which is time worth spending).

Thanks.