[PATCH 0/2] use `curl-config --cflags`

[PATCH 0/2] use `curl-config --cflags`

[Jeff King]

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

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

[Jeff King]

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

[PATCH 2/2] Makefile: use curl-config --cflags

[Jeff King]

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

Re: [PATCH 0/2] use `curl-config --cflags`

[Junio C Hamano]

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.

Re: [PATCH 0/2] use `curl-config --cflags`

[Johannes Schindelin]

Hi Peff,

On Thu, 26 Mar 2020, Jeff King wrote:

> 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

I _suspect_ that this is responsible for the build failure

	make: curl-config: Command not found

at https://github.com/git/git/runs/556459415#step:4:674

Do we need this to fix this?

-- snip --
diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh
index de41888430a..325b4cc6185 100755
--- a/ci/test-documentation.sh
+++ b/ci/test-documentation.sh
@@ -11,6 +11,7 @@ filter_log () {
 	    -e '/^    \* new asciidoc flags$/d' \
 	    -e '/stripped namespace before processing/d' \
 	    -e '/Attributed.*IDs for element/d' \
+	    -e '/curl-config: Command not found/d' \
 	    "$1"
 }

-- snap --

Ciao,
Dscho

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

[PATCH] Makefile: avoid running curl-config unnecessarily

[Jeff King]

On Sat, Apr 04, 2020 at 03:38:00PM +0200, Johannes Schindelin wrote:

> >   [1/2]: Makefile: avoid running curl-config multiple times
> >   [2/2]: Makefile: use curl-config --cflags
> 
> I _suspect_ that this is responsible for the build failure
> 
> 	make: curl-config: Command not found
> 
> at https://github.com/git/git/runs/556459415#step:4:674
> 
> Do we need this to fix this?

Hmm, yeah. It's an unfortunate side effect of the ":=" assignment to
stop repeatedly invoking curl-config. Now it's only invoked once, but
it's _always_ once, even if we're not building anything that needs it.

I wonder if there's a way to expand a Makefile variable lazily, but only
once...aha, with some help from the Internet, I came up with the patch
below.

> -- snip --
> diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh
> index de41888430a..325b4cc6185 100755
> --- a/ci/test-documentation.sh
> +++ b/ci/test-documentation.sh
> @@ -11,6 +11,7 @@ filter_log () {
>  	    -e '/^    \* new asciidoc flags$/d' \
>  	    -e '/stripped namespace before processing/d' \
>  	    -e '/Attributed.*IDs for element/d' \
> +	    -e '/curl-config: Command not found/d' \
>  	    "$1"
>  }

Yes, this works, but it's rather unfortunate that we're invoking
curl-config when it's not needed. Perhaps using NO_CURL in the
documentation job would be better. But if the patch below isn't too
disgusting, I think I prefer that approach (because it helps _any_
top-level make invocation that isn't actually building http binaries).

Junio, you may want to hold off on moving jk/build-with-right-curl to
next until we resolve this one way or the other.

-- >8 --
Subject: [PATCH] Makefile: avoid running curl-config unnecessarily

Commit 94a88e2524 (Makefile: avoid running curl-config multiple times,
2020-03-26) put the call to $(CURL_CONFIG) into a "simple" variable
which is expanded immediately, rather than expanding it each time it's
needed. However, that also means that we expand it whenever the Makefile
is parsed, whether we need it or not.

This is wasteful, but also breaks the ci/test-documentation.sh job, as
it does not have curl at all and complains about the extra messages to
stderr. An easy way to see it is just:

  $ make CURL_CONFIG=does-not-work check-builtins
  make: does-not-work: Command not found
  make: does-not-work: Command not found
  GIT_VERSION = 2.26.0.108.gb3f3f45f29
  make: does-not-work: Command not found
  make: does-not-work: Command not found
  ./check-builtins.sh

We can get the best of both worlds if we're willing to accept a little
Makefile hackery. Courtesy of the article at:

  http://make.mad-scientist.net/deferred-simple-variable-expansion/

this patch uses a lazily-evaluated recursive variable which replaces its
contents with an immediately assigned simple one on first use.

Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Jeff King <peff@peff.net>
---
This is our first use of eval in the Makefile, but that goes back to GNU
make v3.80, which is from 2002. I think that should be OK.

If this inlining is too gross, we could probably contain it in a
function where callers would do something like:

  $(eval lazy-shell-var, CURL_LDFLAGS, $(CURL_CONFIG) --libs)

That's better in the sense that there's less gobbledygook at each
callsite, but worse in that it obscures the fact that it's a variable
assignment. I'd probably consider going that direction if we started
growing more use-cases than these two.

We could probably also stuff this into a sh
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 5099f6a8f8..97dbdcd201 100644
--- a/Makefile
+++ b/Makefile
@@ -1366,12 +1366,12 @@ else
 	endif
 
 	ifndef CURL_LDFLAGS
-		CURL_LDFLAGS := $(shell $(CURL_CONFIG) --libs)
+		CURL_LDFLAGS = $(eval CURL_LDFLAGS := $$(shell $$(CURL_CONFIG) --libs))$(CURL_LDFLAGS)
 	endif
 	CURL_LIBCURL += $(CURL_LDFLAGS)
 
 	ifndef CURL_CFLAGS
-		CURL_CFLAGS := $(shell $(CURL_CONFIG) --cflags)
+		CURL_CFLAGS = $(eval CURL_CFLAGS := $$(shell $$(CURL_CONFIG) --cflags))$(CURL_CFLAGS)
 	endif
 	BASIC_CFLAGS += $(CURL_CFLAGS)
 
-- 
2.26.0.410.gc279fb3cbd

Re: [PATCH] Makefile: avoid running curl-config unnecessarily

[Eric Sunshine]

On Sat, Apr 4, 2020 at 10:58 AM Jeff King <peff@peff.net> wrote:
> This is our first use of eval in the Makefile, but that goes back to GNU
> make v3.80, which is from 2002. I think that should be OK.

Upon reading this, I was worried that it might trip up Mac OS which
has (often very) old versions of tools, but it's alright in this case,
as Apple ships GNU 'make' 3.81 from 2006.

Re: [PATCH] Makefile: avoid running curl-config unnecessarily

[Jeff King]

On Sat, Apr 04, 2020 at 09:33:51PM -0400, Eric Sunshine wrote:

> On Sat, Apr 4, 2020 at 10:58 AM Jeff King <peff@peff.net> wrote:
> > This is our first use of eval in the Makefile, but that goes back to GNU
> > make v3.80, which is from 2002. I think that should be OK.
> 
> Upon reading this, I was worried that it might trip up Mac OS which
> has (often very) old versions of tools, but it's alright in this case,
> as Apple ships GNU 'make' 3.81 from 2006.

They're certainly good at keeping old versions of things around. :) I
wonder if it's due to a switch from GPLv2 to v3. My understanding is
that's why they'll keep bash 3 around forever, and the GPLv3 was
published in 2007, between GNU make 3.81 and 3.82.

Anyway, I think we're good, but thanks for double-checking.

-Peff

Re: [PATCH] Makefile: avoid running curl-config unnecessarily

[Eric Sunshine]

On Sun, Apr 5, 2020 at 2:44 PM Jeff King <peff@peff.net> wrote:
> On Sat, Apr 04, 2020 at 09:33:51PM -0400, Eric Sunshine wrote:
> > On Sat, Apr 4, 2020 at 10:58 AM Jeff King <peff@peff.net> wrote:
> > > This is our first use of eval in the Makefile, but that goes back to GNU
> > > make v3.80, which is from 2002. I think that should be OK.
> >
> > Upon reading this, I was worried that it might trip up Mac OS which
> > has (often very) old versions of tools, but it's alright in this case,
> > as Apple ships GNU 'make' 3.81 from 2006.
>
> They're certainly good at keeping old versions of things around. :) I
> wonder if it's due to a switch from GPLv2 to v3. My understanding is
> that's why they'll keep bash 3 around forever, and the GPLv3 was
> published in 2007, between GNU make 3.81 and 3.82.

That is my understanding, as well.

> Anyway, I think we're good, but thanks for double-checking.

History repeats itself[1].

[1]: https://lore.kernel.org/git/20180802212930.GB32538@sigill.intra.peff.net/

Re: [PATCH] Makefile: avoid running curl-config unnecessarily

[Johannes Schindelin]

Hi,

On Sat, 4 Apr 2020, Eric Sunshine wrote:

> On Sat, Apr 4, 2020 at 10:58 AM Jeff King <peff@peff.net> wrote:
> > This is our first use of eval in the Makefile, but that goes back to GNU
> > make v3.80, which is from 2002. I think that should be OK.
>
> Upon reading this, I was worried that it might trip up Mac OS which
> has (often very) old versions of tools, but it's alright in this case,
> as Apple ships GNU 'make' 3.81 from 2006.

I share this concern. It may be more robust, after all, to squelch the
`curl-config` warning specifically in the `Documentation` job of our CI/PR
runs.

Ciao,
Dscho

Re: [PATCH] Makefile: avoid running curl-config unnecessarily

[Johannes Schindelin]

Hi Peff,

On Sat, 4 Apr 2020, Jeff King wrote:

> On Sat, Apr 04, 2020 at 03:38:00PM +0200, Johannes Schindelin wrote:
>
> > >   [1/2]: Makefile: avoid running curl-config multiple times
> > >   [2/2]: Makefile: use curl-config --cflags
> >
> > I _suspect_ that this is responsible for the build failure
> >
> > 	make: curl-config: Command not found
> >
> > at https://github.com/git/git/runs/556459415#step:4:674
> >
> > Do we need this to fix this?
>
> Hmm, yeah. It's an unfortunate side effect of the ":=" assignment to
> stop repeatedly invoking curl-config. Now it's only invoked once, but
> it's _always_ once, even if we're not building anything that needs it.
>
> I wonder if there's a way to expand a Makefile variable lazily, but only
> once...aha, with some help from the Internet, I came up with the patch
> below.

Thanks, that would work.

> > -- snip --
> > diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh
> > index de41888430a..325b4cc6185 100755
> > --- a/ci/test-documentation.sh
> > +++ b/ci/test-documentation.sh
> > @@ -11,6 +11,7 @@ filter_log () {
> >  	    -e '/^    \* new asciidoc flags$/d' \
> >  	    -e '/stripped namespace before processing/d' \
> >  	    -e '/Attributed.*IDs for element/d' \
> > +	    -e '/curl-config: Command not found/d' \
> >  	    "$1"
> >  }
>
> Yes, this works, but it's rather unfortunate that we're invoking
> curl-config when it's not needed. Perhaps using NO_CURL in the
> documentation job would be better. But if the patch below isn't too
> disgusting, I think I prefer that approach (because it helps _any_
> top-level make invocation that isn't actually building http binaries).

I believe that we do need to avoid `NO_CURL` lest we miss out on
documentation mismatches in the `check-docs` target.

Ciao,
Dscho

> Junio, you may want to hold off on moving jk/build-with-right-curl to
> next until we resolve this one way or the other.
>
> -- >8 --
> Subject: [PATCH] Makefile: avoid running curl-config unnecessarily
>
> Commit 94a88e2524 (Makefile: avoid running curl-config multiple times,
> 2020-03-26) put the call to $(CURL_CONFIG) into a "simple" variable
> which is expanded immediately, rather than expanding it each time it's
> needed. However, that also means that we expand it whenever the Makefile
> is parsed, whether we need it or not.
>
> This is wasteful, but also breaks the ci/test-documentation.sh job, as
> it does not have curl at all and complains about the extra messages to
> stderr. An easy way to see it is just:
>
>   $ make CURL_CONFIG=does-not-work check-builtins
>   make: does-not-work: Command not found
>   make: does-not-work: Command not found
>   GIT_VERSION = 2.26.0.108.gb3f3f45f29
>   make: does-not-work: Command not found
>   make: does-not-work: Command not found
>   ./check-builtins.sh
>
> We can get the best of both worlds if we're willing to accept a little
> Makefile hackery. Courtesy of the article at:
>
>   http://make.mad-scientist.net/deferred-simple-variable-expansion/
>
> this patch uses a lazily-evaluated recursive variable which replaces its
> contents with an immediately assigned simple one on first use.
>
> Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This is our first use of eval in the Makefile, but that goes back to GNU
> make v3.80, which is from 2002. I think that should be OK.
>
> If this inlining is too gross, we could probably contain it in a
> function where callers would do something like:
>
>   $(eval lazy-shell-var, CURL_LDFLAGS, $(CURL_CONFIG) --libs)
>
> That's better in the sense that there's less gobbledygook at each
> callsite, but worse in that it obscures the fact that it's a variable
> assignment. I'd probably consider going that direction if we started
> growing more use-cases than these two.
>
> We could probably also stuff this into a sh
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 5099f6a8f8..97dbdcd201 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1366,12 +1366,12 @@ else
>  	endif
>
>  	ifndef CURL_LDFLAGS
> -		CURL_LDFLAGS := $(shell $(CURL_CONFIG) --libs)
> +		CURL_LDFLAGS = $(eval CURL_LDFLAGS := $$(shell $$(CURL_CONFIG) --libs))$(CURL_LDFLAGS)
>  	endif
>  	CURL_LIBCURL += $(CURL_LDFLAGS)
>
>  	ifndef CURL_CFLAGS
> -		CURL_CFLAGS := $(shell $(CURL_CONFIG) --cflags)
> +		CURL_CFLAGS = $(eval CURL_CFLAGS := $$(shell $$(CURL_CONFIG) --cflags))$(CURL_CFLAGS)
>  	endif
>  	BASIC_CFLAGS += $(CURL_CFLAGS)
>
> --
> 2.26.0.410.gc279fb3cbd
>
>

Re: [PATCH] Makefile: avoid running curl-config unnecessarily

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Junio, you may want to hold off on moving jk/build-with-right-curl to
> next until we resolve this one way or the other.
>
> -- >8 --
> Subject: [PATCH] Makefile: avoid running curl-config unnecessarily

I think this has been accepted favourably after all?  I am inclined
to mark these three as ready for 'next'.

Thanks.

Re: [PATCH] Makefile: avoid running curl-config unnecessarily

[Jeff King]

On Wed, Apr 15, 2020 at 02:47:47PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Junio, you may want to hold off on moving jk/build-with-right-curl to
> > next until we resolve this one way or the other.
> >
> > -- >8 --
> > Subject: [PATCH] Makefile: avoid running curl-config unnecessarily
> 
> I think this has been accepted favourably after all?  I am inclined
> to mark these three as ready for 'next'.

Yeah, my "until we resolve" was basically "do people find this Makefile
eval trick too gross". And I think the answer is that it's fine.

-Peff