Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] use `curl-config --cflags`
@ 2020-03-26  8:05 Jeff King
  2020-03-26  8:06 ` [PATCH 1/2] Makefile: avoid running curl-config multiple times Jeff King
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Jeff King @ 2020-03-26  8:05 UTC (permalink / raw)
  To: git

I was recently testing Git's behavior with respect to various versions
of libcurl. So I built a one-off libcurl and installed it in /tmp/foo,
but was surprised that:

  make CURL_CONFIG=/tmp/foo/bin/curl-config

didn't work, since we do run "$(CURL_CONFIG) --libs". This fixes it,
along with a minor optimization to the existing "--libs" call.

  [1/2]: Makefile: avoid running curl-config multiple times
  [2/2]: Makefile: use curl-config --cflags

 Makefile | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

-Peff

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/2] Makefile: avoid running curl-config multiple times
  2020-03-26  8:05 [PATCH 0/2] use `curl-config --cflags` Jeff King
@ 2020-03-26  8:06 ` Jeff King
  2020-03-26  8:08 ` [PATCH 2/2] Makefile: use curl-config --cflags Jeff King
  2020-03-27 22:12 ` [PATCH 0/2] use `curl-config --cflags` Junio C Hamano
  2 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2020-03-26  8:06 UTC (permalink / raw)
  To: git

If the user hasn't set the CURL_LDFLAGS Makefile variable, we invoke
curl-config like this:

  CURL_LIBCURL += $(shell $(CURL_CONFIG) --libs)

Because the shell function is run when the value is expanded, we invoke
curl-config each time we need to link something (which generally ends up
being four times for a full build).

Instead, let's use an immediate Makefile variable, which only needs
expanding once. We can't combine that with the existing "+=", but since
we only do this when CURL_LDFLAGS is undefined, we can just set that
variable.

That also allows us to simplify our conditional a bit, since both sides
will then put the result into CURL_LIBCURL. While we're touching it,
let's fix the indentation to match the nearby code (we're inside an
outer conditional, so everything else is indented one level).

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index a5961113d8..93a8ef3a72 100644
--- a/Makefile
+++ b/Makefile
@@ -1365,11 +1365,10 @@ else
 		CURL_LIBCURL =
 	endif
 
-ifdef CURL_LDFLAGS
+	ifndef CURL_LDFLAGS
+		CURL_LDFLAGS := $(shell $(CURL_CONFIG) --libs)
+	endif
 	CURL_LIBCURL += $(CURL_LDFLAGS)
-else
-	CURL_LIBCURL += $(shell $(CURL_CONFIG) --libs)
-endif
 
 	REMOTE_CURL_PRIMARY = git-remote-http$X
 	REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$X
-- 
2.26.0.576.gb87790c3c1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 2/2] Makefile: use curl-config --cflags
  2020-03-26  8:05 [PATCH 0/2] use `curl-config --cflags` Jeff King
  2020-03-26  8:06 ` [PATCH 1/2] Makefile: avoid running curl-config multiple times Jeff King
@ 2020-03-26  8:08 ` Jeff King
  2020-03-27 22:12 ` [PATCH 0/2] use `curl-config --cflags` Junio C Hamano
  2 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2020-03-26  8:08 UTC (permalink / raw)
  To: git

We add the result of "curl-config --libs" when linking curl programs,
but we never bother calling "curl-config --cflags". Presumably nobody
noticed because:

  - a system libcurl installed into /usr/include/curl wouldn't need any
    flags ("/usr/include" is already in the search path, and the
    #include lines all look <curl/curl.h>, etc).

  - using CURLDIR sets up both the includes and the library path

However, if you prefer CURL_CONFIG to CURLDIR, something simple like:

  make CURL_CONFIG=/path/to/curl-config

doesn't work. We'd link against the libcurl specified by that program,
but not find its header files when compiling.

Let's invoke "curl-config --cflags" similar to the way we do for
"--libs". Note that we'll feed the result into BASIC_CFLAGS. The rest of
the Makefile doesn't distinguish which files need curl support during
compilation and which do not. That should be OK, though. At most this
should be adding a "-I" directive, and this is how CURLDIR already
behaves. And since we follow the immediate-variable pattern from
CURL_LDFLAGS, we won't accidentally invoke curl-config once per
compilation.

Signed-off-by: Jeff King <peff@peff.net>
---
I didn't bother touching configure.ac here, because I don't think it
would do anything useful. The existing configure script will run
"curl-config --libs" to set CURL_LDFLAGS, suppressing the call to
curl-config inside the Makefile. But with the caveat that you can
convince it to pass something besides --libs (though what would be
useful there, I have no idea, and the commit introducing it didn't shed
any light). So following that pattern, at most we'd just be trading a
call during configure time for one inside the Makefile.

What _could_ be useful is if the configure script used curl-config as
part of it's check of whether we have libcurl at all. But it doesn't. We
don't even look for curl-config until we've been able to link against
-lcurl. I have a feeling this could be fixed by reversing the order of
the blocks, but I'm not sure of all of the subtle interactions with
CURLDIR. And I don't care enough about autoconf to puzzle it out. We
certainly aren't making anything worse with this patch.

 Makefile | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 93a8ef3a72..4550bf4a42 100644
--- a/Makefile
+++ b/Makefile
@@ -1359,9 +1359,10 @@ ifdef NO_CURL
 else
 	ifdef CURLDIR
 		# Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
-		BASIC_CFLAGS += -I$(CURLDIR)/include
+		CURL_CFLAGS = -I$(CURLDIR)/include
 		CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib)
 	else
+		CURL_CFLAGS =
 		CURL_LIBCURL =
 	endif
 
@@ -1370,6 +1371,11 @@ else
 	endif
 	CURL_LIBCURL += $(CURL_LDFLAGS)
 
+	ifndef CURL_CFLAGS
+		CURL_CFLAGS := $(shell $(CURL_CONFIG) --cflags)
+	endif
+	BASIC_CFLAGS += $(CURL_CFLAGS)
+
 	REMOTE_CURL_PRIMARY = git-remote-http$X
 	REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$X
 	REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
-- 
2.26.0.576.gb87790c3c1

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 0/2] use `curl-config --cflags`
  2020-03-26  8:05 [PATCH 0/2] use `curl-config --cflags` Jeff King
  2020-03-26  8:06 ` [PATCH 1/2] Makefile: avoid running curl-config multiple times Jeff King
  2020-03-26  8:08 ` [PATCH 2/2] Makefile: use curl-config --cflags Jeff King
@ 2020-03-27 22:12 ` Junio C Hamano
  2 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2020-03-27 22:12 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Jeff King <peff@peff.net> writes:

> I was recently testing Git's behavior with respect to various versions
> of libcurl. So I built a one-off libcurl and installed it in /tmp/foo,
> but was surprised that:
>
>   make CURL_CONFIG=/tmp/foo/bin/curl-config
>
> didn't work, since we do run "$(CURL_CONFIG) --libs". This fixes it,
> along with a minor optimization to the existing "--libs" call.
>
>   [1/2]: Makefile: avoid running curl-config multiple times
>   [2/2]: Makefile: use curl-config --cflags
>
>  Makefile | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> -Peff

Both patches made sense.  Thanks.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26  8:05 [PATCH 0/2] use `curl-config --cflags` Jeff King
2020-03-26  8:06 ` [PATCH 1/2] Makefile: avoid running curl-config multiple times Jeff King
2020-03-26  8:08 ` [PATCH 2/2] Makefile: use curl-config --cflags Jeff King
2020-03-27 22:12 ` [PATCH 0/2] use `curl-config --cflags` Junio C Hamano

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git