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

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

[Dave Borowitz]

curl-config is usually 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.

Depending on the set of features curl is compiled with, there may be
more libraries required than the previous two options of -lssl and
-lidn. For example, with a vanilla build of libcurl-7.36.0 on Mac OS X
10.9:

$ ~/d/curl-out-7.36.0/lib/curl-config --libs
-L/Users/dborowitz/d/curl-out-7.36.0/lib -lcurl -lgssapi_krb5 -lresolv -lldap -lz

Use this only when CURLDIR is not explicitly specified, to continue
supporting older builds. Moreover, if CURL_CONFIG is unset or running
it returns no results (e.g. because it is missing), default to the old
behavior of blindly setting -lcurl.

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

diff --git a/Makefile b/Makefile
index 2128ce3..cb4ee37 100644
--- a/Makefile
+++ b/Makefile
@@ -34,8 +34,14 @@ 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'. If CURL_CONFIG is unset or points to a binary that
+# is not found, defaults to the CURLDIR behavior, or if CURLDIR is not set,
+# uses -lcurl with no additional library detection.
+#
 # 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 +149,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).
 #
@@ -1118,20 +1126,35 @@ ifdef NO_CURL
 	REMOTE_CURL_NAMES =
 else
 	ifdef CURLDIR
-		# 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
+		CURL_LIBCURL=
 	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
+		ifeq "$(CURL_CONFIG)" ""
+			CURL_LIBCURL =
+		else
+			CURL_LIBCURL := $(shell $(CURL_CONFIG) --libs)
 		endif
 	endif
-	ifdef NEEDS_IDN_WITH_CURL
-		CURL_LIBCURL += -lidn
+
+	ifeq "$(CURL_LIBCURL)" ""
+		ifdef CURLDIR
+			# 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
+		else
+			CURL_LIBCURL = -lcurl
+		endif
+		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
+		BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags)
 	endif
 
 	REMOTE_CURL_PRIMARY = git-remote-http$X
-- 
1.9.1.423.g4596e3a

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

[Dave Borowitz]

This requires more flags than can be guessed with the old-style
CURLDIR and related options, so is only supported when curl-config is
present.

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

diff --git a/Makefile b/Makefile
index cb4ee37..360d427 100644
--- a/Makefile
+++ b/Makefile
@@ -39,6 +39,9 @@ all::
 # is not found, defaults to the CURLDIR behavior, or if CURLDIR is not set,
 # uses -lcurl with no additional library detection.
 #
+# 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.
@@ -1137,6 +1140,9 @@ else
 	endif
 
 	ifeq "$(CURL_LIBCURL)" ""
+		ifdef CURL_STATIC
+                        $(error "CURL_STATIC must be used with CURL_CONFIG")
+		endif
 		ifdef CURLDIR
 			# Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
 			BASIC_CFLAGS += -I$(CURLDIR)/include
@@ -1155,6 +1161,12 @@ else
 		endif
 	else
 		BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags)
+		ifdef CURL_STATIC
+			CURL_LIBCURL = $(shell $(CURL_CONFIG) --static-libs)
+			ifeq "$(CURL_LIBCURL)" ""
+                                $(error libcurl not detected or not compiled with static support)
+			endif
+		endif
 	endif
 
 	REMOTE_CURL_PRIMARY = git-remote-http$X
-- 
1.9.1.423.g4596e3a

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

[Jonathan Nieder]

Hi,

Dave Borowitz wrote:

> curl-config is usually 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.

The previous version of these two patches is already part of "master".
Could you make an incremental patch?

Sorry for the fuss,
Jonathan

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

[Dave Borowitz]

On Mon, Apr 28, 2014 at 12:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Dave Borowitz wrote:
>
>> curl-config is usually 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.
>
> The previous version of these two patches is already part of "master".
> Could you make an incremental patch?

Done. Thanks for pointing that out, and sorry for the noise.

> Sorry for the fuss,
> Jonathan

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

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> Use this only when CURLDIR is not explicitly specified, to continue
> supporting older builds. Moreover, if CURL_CONFIG is unset or running
> it returns no results (e.g. because it is missing), default to the old
> behavior of blindly setting -lcurl.
>  	ifdef CURLDIR
> +		CURL_LIBCURL=
>  	else
> +		CURL_CONFIG ?= curl-config
> +		ifeq "$(CURL_CONFIG)" ""
> +			CURL_LIBCURL =
> +		else
> +			CURL_LIBCURL := $(shell $(CURL_CONFIG) --libs)
>  		endif

This "ifeq" is redundant and will never set CURL_LIBCURL to empty
without running the "else" part, I think.  In a Makefile, a variable
explicitly set to empty and a variable that is unset are treated the
same.

	$ cat >Makefile <<EOF
	CURL_CONFIG ?= curl-config
	ifeq "$(CURL_CONFIG)" ""
		X=Empty
	else
		X=NotEmpty
	endif

	ifdef "$(CURL_CONFIG)"
		Z=Defined
	else
		Z=Undefined
	endif

	all::
		@echo "$(X) $(Z)"
	EOF
	$ make -f Makefile CURL_CONFIG=""
	Empty Undefined

That does not mean the patch will give us a broken behaviour,
though.  It just means the ifeq/else part will be redundant.

>  	endif
> +
> +	ifeq "$(CURL_LIBCURL)" ""

This will catch the "$(shell $(CURL_CONFIG) --libs) assigned an
empty string to CURL_LIBCURL" case, so the result is good.

I haven't checked what it would look like if we turn this into an
incremental patch to be applied on top of 'master' (which would give
us a place to document better why we do not rely on the presense of
curl-config), but if we can do so, that would be more preferable
than having to revert the merge of the previous one and then
applying these two patches anew.

Thanks.

> +		ifdef CURLDIR
> +			# 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
> +		else
> +			CURL_LIBCURL = -lcurl
> +		endif
> +		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
> +		BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags)
>  	endif
>  
>  	REMOTE_CURL_PRIMARY = git-remote-http$X

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> That does not mean the patch will give us a broken behaviour,
> though.  It just means the ifeq/else part will be redundant.
>
>>  	endif
>> +
>> +	ifeq "$(CURL_LIBCURL)" ""
>
> This will catch the "$(shell $(CURL_CONFIG) --libs) assigned an
> empty string to CURL_LIBCURL" case, so the result is good.
>
> I haven't checked what it would look like if we turn this into an
> incremental patch to be applied on top of 'master' (which would give
> us a place to document better why we do not rely on the presense of
> curl-config), but if we can do so, that would be more preferable
> than having to revert the merge of the previous one and then
> applying these two patches anew.

And I just checked; it is not very pretty to call it "trivially
correct", and I would feel safer to revert the merge for 2.0, and
queue the new one for the next cycle, cooking it in 'pu' and then
'next' in the meantime.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> This "ifeq" is redundant and will never set CURL_LIBCURL to empty
> without running the "else" part, I think.  In a Makefile, a variable
> explicitly set to empty and a variable that is unset are treated the
> same....
> 	$ make -f Makefile CURL_CONFIG=""
> 	Empty Undefined

Oh, I was blind.  Please ignore this noise.

Thanks.