git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: [PATCH] Makefile: avoid running curl-config unnecessarily
Date: Sat, 4 Apr 2020 10:58:29 -0400	[thread overview]
Message-ID: <20200404145829.GB679473@coredump.intra.peff.net> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2004041535360.46@tvgsbejvaqbjf.bet>

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


  reply	other threads:[~2020-04-04 14:58 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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
2020-04-04 13:38 ` Johannes Schindelin
2020-04-04 14:58   ` Jeff King [this message]
2020-04-05  1:33     ` [PATCH] Makefile: avoid running curl-config unnecessarily Eric Sunshine
2020-04-05 18:44       ` Jeff King
2020-04-05 19:18         ` Eric Sunshine
2020-04-05 22:12       ` Johannes Schindelin
2020-04-05 22:12     ` Johannes Schindelin
2020-04-15 21:47     ` Junio C Hamano
2020-04-15 21:57       ` Jeff King

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200404145829.GB679473@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).