threads / patch / 36528

v2Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR

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

## tl;dr

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

replies: 6people: 4as markdown or json

Dave Borowitz· Apr 28, 2014, 21:01 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.

Signed-off-by: Dave Borowitz <dborowitz@google.com>
---
 Makefile | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)
Show changes to Makefile +28 −13
diff --git a/Makefile b/Makefile
index 74a929b..81e8214 100644
--- a/Makefile
+++ b/Makefile
@@ -35,14 +35,17 @@ 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.
 #
 # 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.
+# /foo/bar/include and /foo/bar/lib directories.  This overrides
+# CURL_CONFIG, but is less robust.  If not set, and CURL_CONFIG is not set,
+# uses -lcurl with no additional library detection (other than
+# NEEDS_*_WITH_CURL).
 #
 # 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).
@@ -1127,9 +1130,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 +1161,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
Junio C Hamano· Apr 28, 2014, 21:32 UTC · re: Dave Borowitz · lore

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

Dave Borowitz <dborowitz@google.com> writes:
Show 12 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.
>
> Signed-off-by: Dave Borowitz <dborowitz@google.com>
> ---
Thanks. Will pick this version up.
Jonathan Nieder· Apr 28, 2014, 23:30 UTC · re: Dave Borowitz · lore

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

Dave Borowitz wrote:
> Signed-off-by: Dave Borowitz <dborowitz@google.com>
> ---
>  Makefile | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks for the quick turnaround.
Erik Faye-Lund· Apr 30, 2014, 13:04 UTC · re: Dave Borowitz · lore

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

On Mon, Apr 28, 2014 at 11:01 PM, Dave Borowitz <dborowitz@google.com> wrote:
Show 37 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.
>
> Signed-off-by: Dave Borowitz <dborowitz@google.com>
> ---
>  Makefile | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 74a929b..81e8214 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -35,14 +35,17 @@ 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.
>  #
>  # 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.
> +# /foo/bar/include and /foo/bar/lib directories.  This overrides
> +# CURL_CONFIG, but is less robust.  If not set, and CURL_CONFIG is not set,
> +# uses -lcurl with no additional library detection (other than
> +# NEEDS_*_WITH_CURL).

This is wrong, no? With CURL_CONFIG not set, it currently *does* run curl-config, see below.

Show 18 quoted lines
>  #
>  # 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).
> @@ -1127,9 +1130,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

Doesn't that definition just define CURL_CONFIG unconditionally? How are the first condition ever supposed to get triggered?

$ make
make: curl-config: Command not found
GIT_VERSION = 1.9.2.462.gf3f11fa
make: curl-config: Command not found
    * new build flags
    * new link flags
    * new prefix flags
    GEN common-cmds.h
...
Yuck.
Junio C Hamano· Apr 30, 2014, 15:13 UTC · re: Erik Faye-Lund · lore

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

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 27 quoted lines
> This is wrong, no? With CURL_CONFIG not set, it currently *does* run
> curl-config, see below.
> ...
>>         ifdef CURLDIR
>> +               CURL_LIBCURL =
>> +       else
>> +               CURL_CONFIG = curl-config
>> +               ifeq "$(CURL_CONFIG)" ""
>> +                       CURL_LIBCURL =
>> +               else
>> +                       CURL_LIBCURL := $(shell $(CURL_CONFIG) --libs)
>> +               endif
>
> Doesn't that definition just define CURL_CONFIG unconditionally? How
> are the first condition ever supposed to get triggered?
>
> $ make
> make: curl-config: Command not found
> GIT_VERSION = 1.9.2.462.gf3f11fa
> make: curl-config: Command not found
>     * new build flags
>     * new link flags
>     * new prefix flags
>     GEN common-cmds.h
> ...
>
> Yuck.

An earlier iteration of the patch used "CURL_CONFIG ?= curl-config", but that would not have been much different:

        $ 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) CURL_CONFIG=<$(CURL_CONFIG)>"
        EOF
        $ make
        NotEmpty Defined CURL_CONFIG=<curl-config>
        $ make CURL_CONFIG=""
        Empty Undefined CURL_CONFIG=<>
        $ CURL_CONFIG="" make
        Empty Undefined CURL_CONFIG=<>

As the first one (the default) will still use curl-config and passing an explicit CURL_CONFIG="" on the command line would be the only way to squelch this unpleasantness. If you change

	CURL_CONFIG ?= curl-config
to
	CURL_CONFIG = curl-config

in the above illustration, the first two would be the same result as above, and the last one will behave the same as the first one---an environment set to empty is still protected from the default defined in the Makefile.

I think something along the lines of 
	ifdef CURLDIR
        	CURL_LIBCURL =
	else
		CURL_CONFIG = curl-config
		CURL_LIBCURL := $(shell sh -c '$(CURL_CONFIG) --libs' 2>/dev/null)
	fi
may be the right way to write this?

Note that $(shell $(CURL_CONFIG) --libs) when CURL_CONFIG is empty would barf when $(CURL_CONFIG) expands to an empty string.

Erik Faye-Lund· Apr 30, 2014, 16:05 UTC · re: Junio C Hamano · lore

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

On Wed, Apr 30, 2014 at 5:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> I think something along the lines of
>
>         ifdef CURLDIR
>                 CURL_LIBCURL =
>         else
>                 CURL_CONFIG = curl-config
>                 CURL_LIBCURL := $(shell sh -c '$(CURL_CONFIG) --libs' 2>/dev/null)
>         fi
>
> may be the right way to write this?
>
> Note that $(shell $(CURL_CONFIG) --libs) when CURL_CONFIG is empty
> would barf when $(CURL_CONFIG) expands to an empty string.

There's still the fact that msysGit does *not* need CURLDIR, but doesn't have curl-config either. So I think this one will also complain for us.

Junio C Hamano· Apr 30, 2014, 17:26 UTC · re: Erik Faye-Lund · lore

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

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 18 quoted lines
> On Wed, Apr 30, 2014 at 5:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> I think something along the lines of
>>
>>         ifdef CURLDIR
>>                 CURL_LIBCURL =
>>         else
>>                 CURL_CONFIG = curl-config
>>                 CURL_LIBCURL := $(shell sh -c '$(CURL_CONFIG) --libs' 2>/dev/null)
>>         fi
>>
>> may be the right way to write this?
>>
>> Note that $(shell $(CURL_CONFIG) --libs) when CURL_CONFIG is empty
>> would barf when $(CURL_CONFIG) expands to an empty string.
>
> There's still the fact that msysGit does *not* need CURLDIR, but
> doesn't have curl-config either. So I think this one will also
> complain for us.
Even with 2>/dev/null?

← back to recent threads