# [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR 7 messages from 2014-04-28 to 2014-04-30. Participants: Dave Borowitz, Junio C Hamano, Jonathan Nieder, Erik Faye-Lund. Thread: https://gitlist.dev/t/36528 ## Dave Borowitz, 2014-04-28 21:01 Subject: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: <1398718883-5630-1-git-send-email-dborowitz@google.com> URL: https://gitlist.dev/e/1398718883-5630-1-git-send-email-dborowitz%40google.com ``` 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 --- 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 ``` ## Junio C Hamano, 2014-04-28 21:32 Subject: Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: URL: https://gitlist.dev/e/xmqqfvkxnnqu.fsf%40gitster.dls.corp.google.com In-Reply-To: <1398718883-5630-1-git-send-email-dborowitz@google.com> ``` Dave Borowitz 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 > --- Thanks. Will pick this version up. ``` ## Jonathan Nieder, 2014-04-28 23:30 Subject: Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: <20140428233016.GR9218@google.com> URL: https://gitlist.dev/e/20140428233016.GR9218%40google.com In-Reply-To: <1398718883-5630-1-git-send-email-dborowitz@google.com> ``` Dave Borowitz wrote: > Signed-off-by: Dave Borowitz > --- > Makefile | 41 ++++++++++++++++++++++++++++------------- > 1 file changed, 28 insertions(+), 13 deletions(-) For what it's worth, Reviewed-by: Jonathan Nieder Thanks for the quick turnaround. ``` ## Erik Faye-Lund, 2014-04-30 13:04 Subject: Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: URL: https://gitlist.dev/e/CABPQNSYDD7g3nOwb2ZaOQ9M9gQnjzQyKP4Zo-i8p4o-s30bk1Q%40mail.gmail.com In-Reply-To: <1398718883-5630-1-git-send-email-dborowitz@google.com> ``` On Mon, Apr 28, 2014 at 11:01 PM, Dave Borowitz 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 > --- > 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. ``` ## Junio C Hamano, 2014-04-30 15:13 Subject: Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: URL: https://gitlist.dev/e/xmqqk3a6hmv6.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Erik Faye-Lund 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= $ 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, 2014-04-30 16:05 Subject: Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: URL: https://gitlist.dev/e/CABPQNSZVFWhP%2BoTYpU2HeenJXwMsDNQGegwYZVb%3DbZ2%2BBRz3-w%40mail.gmail.com In-Reply-To: ``` On Wed, Apr 30, 2014 at 5:13 PM, Junio C Hamano 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. ``` ## Junio C Hamano, 2014-04-30 17:26 Subject: Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: URL: https://gitlist.dev/e/xmqqzjj2g24r.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Erik Faye-Lund writes: > On Wed, Apr 30, 2014 at 5:13 PM, Junio C Hamano 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? ```