threads / patch / 36527

patchMakefile: default to -lcurl when no CURL_CONFIG or CURLDIR

Subject: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR

## tl;dr

10 messages between Apr 28, 2014 and Apr 28, 2014. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Dave Borowitz· Apr 28, 2014, 19:50 UTC · lore

The original implementation of CURL_CONFIG support did not match the original behavior of using -lcurl when CURLDIR was not set. This broke implementations that were lacking curl-config but did have libcurl installed along system libraries, such as MSysGit. In other words, the assumption that curl-config is always installed was incorrect.

Instead, if CURL_CONFIG is empty or returns an empty result (e.g. due
to curl-config being missing), use the old behavior of falling back to
-lcurl.
---
 Makefile | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)
Show changes to Makefile +25 −11
diff --git a/Makefile b/Makefile
index 74a929b..79b7442 100644
--- a/Makefile
+++ b/Makefile
@@ -35,7 +35,9 @@ all::
 # transports (neither smart nor dumb).
 #
 # Define CURL_CONFIG to the path to a curl-config binary other than the
-# default 'curl-config'.
+# 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 CURL_STATIC to statically link libcurl.  Only applies if
 # CURL_CONFIG is used.
@@ -1127,9 +1129,27 @@ 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_CONFIG ?= curl-config
+		ifeq "$(CURL_CONFIG)" ""
+			CURL_LIBCURL =
+		else
+			CURL_LIBCURL := $(shell $(CURL_CONFIG) --libs)
+		endif
+	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
+			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
@@ -1140,17 +1160,11 @@ else
 			CURL_LIBCURL += -lidn
 		endif
 	else
-		CURL_CONFIG ?= curl-config
 		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
-		else
-			CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs)
-			ifeq "$(CURL_LIBCURL)" ""
-				$(error libcurl not detected; try setting CURLDIR)
+                                $(error libcurl not detected or not compiled with static support)
 			endif
 		endif
 	endif

-- 
1.9.1.423.g4596e3a
Jonathan Nieder· Apr 28, 2014, 20:05 UTC · re: Dave Borowitz · lore

Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR

Dave Borowitz wrote:
Show 6 quoted lines
> Instead, if CURL_CONFIG is empty or returns an empty result (e.g. due
> to curl-config being missing), use the old behavior of falling back to
> -lcurl.
> ---
>  Makefile | 36 +++++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)
Sign-off?
[...]
Show 9 quoted lines
> +++ b/Makefile
> @@ -35,7 +35,9 @@ all::
>  # transports (neither smart nor dumb).
>  #
>  # Define CURL_CONFIG to the path to a curl-config binary other than the
> -# default 'curl-config'.
> +# 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.

I'm having a little trouble parsing this but don't have any better suggestion.

[...]
Show 8 quoted lines
> @@ -1127,9 +1129,27 @@ 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=

Tiny nit: elsewhere the makefile seems to prefer having a space before the '='.

If I explicitly set CURL_LIBCURL to the empty string and CURLDIR was set then my setting will still override the setting below and the behavior is unchanged from before this patch --- good.

If I explicitly set CURL_LIBCURL to empty and CURLDIR was unset then that used to produce an error so it was an invalid configuration and couldn't regress.

So this should be safe --- good.
> +	else
> +		CURL_CONFIG ?= curl-config

Not about this patch, but the above '?=' should probably be plain '=' for consistency with the rest of the makefile's behavior wrt envvars.

[...]
> -				$(error libcurl not detected; try setting CURLDIR)
> +                                $(error libcurl not detected or not compiled with static support)
Whitespace damage.
Except for the whitespace issues,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
Dave Borowitz· Apr 28, 2014, 20:46 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR

On Mon, Apr 28, 2014 at 1:05 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 10 quoted lines
> Dave Borowitz wrote:
>
>> Instead, if CURL_CONFIG is empty or returns an empty result (e.g. due
>> to curl-config being missing), use the old behavior of falling back to
>> -lcurl.
>> ---
>>  Makefile | 36 +++++++++++++++++++++++++-----------
>>  1 file changed, 25 insertions(+), 11 deletions(-)
>
> Sign-off?
Oops.
Show 13 quoted lines
> [...]
>> +++ b/Makefile
>> @@ -35,7 +35,9 @@ all::
>>  # transports (neither smart nor dumb).
>>  #
>>  # Define CURL_CONFIG to the path to a curl-config binary other than the
>> -# default 'curl-config'.
>> +# 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.
>
> I'm having a little trouble parsing this but don't have any better
> suggestion.

How about: "If CURL_CONFIG is unset or points to a binary that is not found, defaults to the CURLDIR behavior. If CURLDIR is not set, this means using -lcurl with no additional library detection (other than NEEDS_*_WITH_CURL).

[...]
>> -                             $(error libcurl not detected; try setting CURLDIR)
>> +                                $(error libcurl not detected or not compiled with static support)
>
> Whitespace damage.
Yes, but intentional, because Makefile parsing is weird.

$ echo -e 'ifndef FOO\n\t$(error bad things)\nendif\n\nfoo:\n\ttouch foo' > mk1 && make -f mk1 foo mk1:2: *** commands commence before first target. Stop. $ echo -e 'ifndef FOO\n $(error bad things)\nendif\n\nfoo:\n\ttouch foo' > mk2 && make -f mk2 foo mk2:2: *** bad things. Stop.

See also: http://stackoverflow.com/questions/4713663/gnu-make-yields-commands-commence-before-first-target-error

> Except for the whitespace issues,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Thanks.
Dave Borowitz· Apr 28, 2014, 20:48 UTC · re: Dave Borowitz · lore

Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR

On Mon, Apr 28, 2014 at 1:46 PM, Dave Borowitz <dborowitz@google.com> wrote:
Show 5 quoted lines
> How about:
> "If CURL_CONFIG is unset or points to a binary that is not found,
> defaults to the CURLDIR behavior. If CURLDIR is not set, this means
> using -lcurl with no additional library detection (other than
> NEEDS_*_WITH_CURL).
Even better, I'll move the blurb about CURLDIR behavior to CURLDIR.
Jonathan Nieder· Apr 28, 2014, 20:50 UTC · re: Dave Borowitz · lore

Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR

Dave Borowitz wrote:
> On Mon, Apr 28, 2014 at 1:05 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Dave Borowitz wrote:
Show 18 quoted lines
>>> +++ b/Makefile
>>> @@ -35,7 +35,9 @@ all::
>>>  # transports (neither smart nor dumb).
>>>  #
>>>  # Define CURL_CONFIG to the path to a curl-config binary other than the
>>> -# default 'curl-config'.
>>> +# 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.
>>
>> I'm having a little trouble parsing this but don't have any better
>> suggestion.
>
> How about:
> "If CURL_CONFIG is unset or points to a binary that is not found,
> defaults to the CURLDIR behavior. If CURLDIR is not set, this means
> using -lcurl with no additional library detection (other than
> NEEDS_*_WITH_CURL).
Yep, that's clearer.
Show 14 quoted lines
> [...]
>>> -                             $(error libcurl not detected; try setting CURLDIR)
>>> +                                $(error libcurl not detected or not compiled with static support)
>>
>> Whitespace damage.
>
> Yes, but intentional, because Makefile parsing is weird.
>
> $ echo -e 'ifndef FOO\n\t$(error bad things)\nendif\n\nfoo:\n\ttouch
> foo' > mk1 && make -f mk1 foo
> mk1:2: *** commands commence before first target.  Stop.
> $ echo -e 'ifndef FOO\n  $(error bad things)\nendif\n\nfoo:\n\ttouch
> foo' > mk2 && make -f mk2 foo
> mk2:2: *** bad things.  Stop.

Gah. Maybe it should be left-justified to avoid accentally breaking it again.

Thanks. Jonathan

Dave Borowitz· Apr 28, 2014, 20:52 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR

On Mon, Apr 28, 2014 at 1:50 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 9 quoted lines
>> $ echo -e 'ifndef FOO\n\t$(error bad things)\nendif\n\nfoo:\n\ttouch
>> foo' > mk1 && make -f mk1 foo
>> mk1:2: *** commands commence before first target.  Stop.
>> $ echo -e 'ifndef FOO\n  $(error bad things)\nendif\n\nfoo:\n\ttouch
>> foo' > mk2 && make -f mk2 foo
>> mk2:2: *** bad things.  Stop.
>
> Gah.  Maybe it should be left-justified to avoid accentally breaking
> it again.

I am ok with that on the basis that it is better to be ugly but obvious. I suspect if the Makefile were more littered with conditional $(error)s we would reconsider using tabs as indentation.

> Thanks.
> Jonathan
Junio C Hamano· Apr 28, 2014, 21:27 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR

Jonathan Nieder <jrnieder@gmail.com> writes:
> Gah.  Maybe it should be left-justified to avoid accentally breaking
> it again.

;-). Yes, $(error) is not the usual part of "to build this target, follow this recipe" part.

But let's take the last round as-is and go with it for 2.0.
Tahnks.
Junio C Hamano· Apr 28, 2014, 21:04 UTC · re: Dave Borowitz · lore

Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR

Dave Borowitz <dborowitz@google.com> writes:
Show 13 quoted lines
> On Mon, Apr 28, 2014 at 1:05 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Dave Borowitz wrote:
>>
>>> Instead, if CURL_CONFIG is empty or returns an empty result (e.g. due
>>> to curl-config being missing), use the old behavior of falling back to
>>> -lcurl.
>>> ---
>>>  Makefile | 36 +++++++++++++++++++++++++-----------
>>>  1 file changed, 25 insertions(+), 11 deletions(-)
>>
>> Sign-off?
>
> Oops.
Will forge your sign-off while queuing ;-)
Show 6 quoted lines
>
> How about:
> "If CURL_CONFIG is unset or points to a binary that is not found,
> defaults to the CURLDIR behavior. If CURLDIR is not set, this means
> using -lcurl with no additional library detection (other than
> NEEDS_*_WITH_CURL).
... with this rephrasing.
Show 6 quoted lines
>>> -                             $(error libcurl not detected; try setting CURLDIR)
>>> +                                $(error libcurl not detected or not compiled with static support)
>>
>> Whitespace damage.
>
> Yes, but intentional, because Makefile parsing is weird.
Thanks.
Junio C Hamano· Apr 28, 2014, 20:45 UTC · re: Dave Borowitz · lore

Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR

Dave Borowitz <dborowitz@google.com> writes:
Show 10 quoted lines
> The original implementation of CURL_CONFIG support did not match the
> original behavior of using -lcurl when CURLDIR was not set. This broke
> implementations that were lacking curl-config but did have libcurl
> installed along system libraries, such as MSysGit. In other words, the
> assumption that curl-config is always installed was incorrect.
>
> Instead, if CURL_CONFIG is empty or returns an empty result (e.g. due
> to curl-config being missing), use the old behavior of falling back to
> -lcurl.
> ---
Sign-off?

I still think the implementation of "If CURL_CONFIG is unset" bit is a bit redundant, though.

Show 6 quoted lines
> +		CURL_CONFIG ?= curl-config
> +		ifeq "$(CURL_CONFIG)" ""
> +			CURL_LIBCURL =
> +		else
> +			CURL_LIBCURL := $(shell $(CURL_CONFIG) --libs)
> +		endif
Other than that, the remainder of the change looks correct to me.
Thanks.
Dave Borowitz· Apr 28, 2014, 20:56 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR

On Mon, Apr 28, 2014 at 1:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I still think the implementation of "If CURL_CONFIG is unset" bit is
> a bit redundant, though.

If CURL_CONFIG is unset, then $(shell $(CURL_CONFIG) --libs) produces "make: --libs: Command not found", which may be confusing.

← back to recent threads