From: Jonathan Nieder <jrnieder@gmail.com>
To: Dave Borowitz <dborowitz@google.com>
Cc: git@vger.kernel.org, kusmabite@gmail.com, gitster@pobox.com
Subject: Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
Date: Mon, 28 Apr 2014 13:05:51 -0700 [thread overview]
Message-ID: <20140428200550.GO9218@google.com> (raw)
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 <jrnieder@gmail.com>
Thanks.
next prev parent reply other threads:[~2014-04-28 20:05 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-04-28 19:50 [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR Dave Borowitz
2014-04-28 20:05 ` Jonathan Nieder [this message]
2014-04-28 20:46 ` Dave Borowitz
2014-04-28 20:48 ` Dave Borowitz
2014-04-28 20:50 ` Jonathan Nieder
2014-04-28 20:52 ` Dave Borowitz
2014-04-28 21:27 ` Junio C Hamano
2014-04-28 21:04 ` Junio C Hamano
2014-04-28 20:45 ` Junio C Hamano
2014-04-28 20:56 ` Dave Borowitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140428200550.GO9218@google.com \
--to=jrnieder@gmail.com \
--cc=dborowitz@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=kusmabite@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.