| Message ID | 20200404145829.GB679473@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: avoid running curl-config unnecessarily | expand |
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.
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
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/
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 > >
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
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.
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
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)