* [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
@ 2014-04-28 21:01 Dave Borowitz
2014-04-28 21:32 ` Junio C Hamano
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Dave Borowitz @ 2014-04-28 21:01 UTC (permalink / raw)
To: git; +Cc: kusmabite, gitster, Dave Borowitz
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).
#
# 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
2014-04-28 21:01 [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Dave Borowitz
@ 2014-04-28 21:32 ` Junio C Hamano
2014-04-28 23:30 ` Jonathan Nieder
2014-04-30 13:04 ` Erik Faye-Lund
2 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2014-04-28 21:32 UTC (permalink / raw)
To: Dave Borowitz; +Cc: git, kusmabite
Dave Borowitz <dborowitz@google.com> writes:
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
2014-04-28 21:01 [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Dave Borowitz
2014-04-28 21:32 ` Junio C Hamano
@ 2014-04-28 23:30 ` Jonathan Nieder
2014-04-30 13:04 ` Erik Faye-Lund
2 siblings, 0 replies; 7+ messages in thread
From: Jonathan Nieder @ 2014-04-28 23:30 UTC (permalink / raw)
To: Dave Borowitz; +Cc: git, kusmabite, gitster
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
2014-04-28 21:01 [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Dave Borowitz
2014-04-28 21:32 ` Junio C Hamano
2014-04-28 23:30 ` Jonathan Nieder
@ 2014-04-30 13:04 ` Erik Faye-Lund
2014-04-30 15:13 ` Junio C Hamano
2 siblings, 1 reply; 7+ messages in thread
From: Erik Faye-Lund @ 2014-04-30 13:04 UTC (permalink / raw)
To: Dave Borowitz; +Cc: GIT Mailing-list, Junio C Hamano
On Mon, Apr 28, 2014 at 11:01 PM, Dave Borowitz <dborowitz@google.com> wrote:
> 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.
> #
> # 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
2014-04-30 13:04 ` Erik Faye-Lund
@ 2014-04-30 15:13 ` Junio C Hamano
2014-04-30 16:05 ` Erik Faye-Lund
0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2014-04-30 15:13 UTC (permalink / raw)
To: kusmabite; +Cc: Dave Borowitz, GIT Mailing-list
Erik Faye-Lund <kusmabite@gmail.com> writes:
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
2014-04-30 15:13 ` Junio C Hamano
@ 2014-04-30 16:05 ` Erik Faye-Lund
2014-04-30 17:26 ` Junio C Hamano
0 siblings, 1 reply; 7+ messages in thread
From: Erik Faye-Lund @ 2014-04-30 16:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Dave Borowitz, GIT Mailing-list
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
2014-04-30 16:05 ` Erik Faye-Lund
@ 2014-04-30 17:26 ` Junio C Hamano
0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2014-04-30 17:26 UTC (permalink / raw)
To: kusmabite; +Cc: Dave Borowitz, GIT Mailing-list
Erik Faye-Lund <kusmabite@gmail.com> writes:
> 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?
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-04-30 17:26 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-28 21:01 [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Dave Borowitz
2014-04-28 21:32 ` Junio C Hamano
2014-04-28 23:30 ` Jonathan Nieder
2014-04-30 13:04 ` Erik Faye-Lund
2014-04-30 15:13 ` Junio C Hamano
2014-04-30 16:05 ` Erik Faye-Lund
2014-04-30 17:26 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).