threads / patch / 36405

patch, 2 partsMakefile: use curl-config to determine curl flags

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

## tl;dr

5 messages between Apr 12, 2014 and Apr 15, 2014. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Dave Borowitz· Apr 12, 2014, 23:15 UTC · lore

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(-)

Show changes to Makefile +23 −12

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

Dave Borowitz· Apr 12, 2014, 23:15 UTC · re: Dave Borowitz · lore

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

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

Show changes to Makefile +8 −1

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

Junio C Hamano· Apr 14, 2014, 20:22 UTC · re: Dave Borowitz · lore

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

Dave Borowitz <dborowitz@google.com> writes:

Show 11 quoted lines

> 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?

Show 69 quoted lines

> ---
>  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

Dave Borowitz· Apr 15, 2014, 10:20 UTC · re: Junio C Hamano · lore

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

On Mon, Apr 14, 2014 at 4:22 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 18 quoted lines

> 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.

Show 69 quoted lines

>> ---
>>  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

Junio C Hamano· Apr 15, 2014, 16:50 UTC · re: Dave Borowitz · lore

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

Dave Borowitz <dborowitz@google.com> writes:

Show 23 quoted lines

> 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.

Show 5 quoted lines

> 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.