# [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR 10 messages from 2014-04-28 to 2014-04-28. Participants: Dave Borowitz, Jonathan Nieder, Junio C Hamano. Thread: https://gitlist.dev/t/36527 ## Dave Borowitz, 2014-04-28 19:50 Subject: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: <1398714653-1050-1-git-send-email-dborowitz@google.com> URL: https://gitlist.dev/e/1398714653-1050-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. --- Makefile | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) 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, 2014-04-28 20:05 Subject: Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: <20140428200550.GO9218@google.com> URL: https://gitlist.dev/e/20140428200550.GO9218%40google.com In-Reply-To: <1398714653-1050-1-git-send-email-dborowitz@google.com> ``` 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? [...] > +++ 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. [...] > @@ -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 Thanks. ``` ## Junio C Hamano, 2014-04-28 20:45 Subject: Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: URL: https://gitlist.dev/e/xmqqlhupp4hx.fsf%40gitster.dls.corp.google.com In-Reply-To: <1398714653-1050-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. > --- Sign-off? I still think the implementation of "If CURL_CONFIG is unset" bit is a bit redundant, though. > + 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, 2014-04-28 20:46 Subject: Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: URL: https://gitlist.dev/e/CAD0k6qR766hrgUtyiGzZ9KM5woknfsyUm3Mf1-Pm3M-LrL2Zhg%40mail.gmail.com In-Reply-To: <20140428200550.GO9218@google.com> ``` On Mon, Apr 28, 2014 at 1:05 PM, Jonathan Nieder 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. > [...] >> +++ 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 > > Thanks. ``` ## Dave Borowitz, 2014-04-28 20:48 Subject: Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: URL: https://gitlist.dev/e/CAD0k6qQ%2BjbFqxpUfH9pm1koicpkqesCbUXxbcW_%2BocgLDUOkjQ%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 28, 2014 at 1:46 PM, Dave Borowitz wrote: > 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, 2014-04-28 20:50 Subject: Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: <20140428205058.GQ9218@google.com> URL: https://gitlist.dev/e/20140428205058.GQ9218%40google.com In-Reply-To: ``` Dave Borowitz wrote: > On Mon, Apr 28, 2014 at 1:05 PM, Jonathan Nieder wrote: >> Dave Borowitz wrote: >>> +++ 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. > [...] >>> - $(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, 2014-04-28 20:52 Subject: Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: URL: https://gitlist.dev/e/CAD0k6qSxBq%2BVAZ2CH-vR-Pyk4FjKKeJmngBU5T9Z-cwSH1LffQ%40mail.gmail.com In-Reply-To: <20140428205058.GQ9218@google.com> ``` On Mon, Apr 28, 2014 at 1:50 PM, Jonathan Nieder wrote: >> $ 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 ``` ## Dave Borowitz, 2014-04-28 20:56 Subject: Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: URL: https://gitlist.dev/e/CAD0k6qRgLfgCTr%3DErc%3DTxJ7_cnSuQLo%2Bswv6Y%3DZ6-OqEzs%3DHwg%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 28, 2014 at 1:45 PM, Junio C Hamano 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. ``` ## Junio C Hamano, 2014-04-28 21:04 Subject: Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: URL: https://gitlist.dev/e/xmqqtx9dnp23.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Dave Borowitz writes: > On Mon, Apr 28, 2014 at 1:05 PM, Jonathan Nieder 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 ;-) > > 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. >>> - $(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, 2014-04-28 21:27 Subject: Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Message-ID: URL: https://gitlist.dev/e/xmqqk3a9no09.fsf%40gitster.dls.corp.google.com In-Reply-To: <20140428205058.GQ9218@google.com> ``` Jonathan Nieder 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. ```