Re: [PATCH v2 4/5] Makefile: respect build info declared in "config.mak"

[Patrick Steinhardt <ps@pks.im>]

On Fri, Dec 20, 2024 at 01:24:27PM -0500, Jeff King wrote:
> On Fri, Dec 20, 2024 at 07:02:09PM +0100, Patrick Steinhardt wrote:
> 
> > > Is there a case you found that doesn't work?
> > 
> > Yes:
> > 
> >     $ make GIT-VERSION-FILE GIT_VERSION=foo
> >     GIT_VERSION=foo
> >     make: 'GIT-VERSION-FILE' is up to date.
> >     $ cat GIT-VERSION-FILE
> >     GIT_VERSION=foo
> > 
> >     # And now run without GIT_VERSION set.
> >     make: 'GIT-VERSION-FILE' is up to date.
> >     GIT_VERSION=foo
> > 
> > So the value remains "sticky" in this case. And that is true whenever
> > you don't set GIT_VERSION at all, we always stick with what is currently
> > in that file.
> 
> Ah, right. Even though we have a recipe to build it, and make knows it
> must be built (because it depends on FORCE), make will read it (and all
> includes) first before executing any rules.
> 
> Something like this seems to work:
> 
> diff --git a/Makefile b/Makefile
> index 788f6ee172..0eb08d98f4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -596,7 +596,12 @@ GIT-VERSION-FILE: FORCE
>  	$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" GIT-VERSION-FILE.in $@ && \
>  	NEW=$$(cat $@ 2>/dev/null || :) && \
>  	if test "$$OLD" != "$$NEW"; then echo "$$NEW" >&2; fi
> +# Never include it on the first read-through, only after make has tried to
> +# refresh includes. We do not want the old values to pollute our new run of the
> +# rule above.
> +ifdef MAKE_RESTARTS
>  -include GIT-VERSION-FILE
> +endif
>  
>  # Set our default configuration.
>  #

Oh, nifty! Playing around with it indeed seems to make things work, and
it's simpler than what I have.

> But I don't know if there are any gotchas (I did not even know about
> MAKE_RESTARTS until digging in the docs looking for a solution here).

Good question indeed. I was wondering whether Make restarts at all in
case where none of the included Makefiles change. But it very much seems
like it does.

The next question is since when the option has been available, as it's
quite, and the answer is that it has been introduced via 978819e1 (Add a
new variable: MAKE_RESTARTS, to count how many times make has re-exec'd.
When rebuilding makefiles, unset -B if MAKE_RESTARTS is >0.,
2005-06-25), which is Make v3.81. Even macOS has that to the best of my
knowledge.

It still does feel somewhat hacky in the end.

> If we can stop including it as a Makefile snippet entirely, I think that
> is easier to reason about.

I very much agree, but it's a non-trivial change. I'll leave that for a
future iteration.

I'm a bit torn now. I have a solution locally that feels less hacky, but
it requires a bit more shuffling. If the eventual goal would be to get
rid of the include in the first place it feels somewhat pointless to do
these changes.

Patrick