On Fri, Dec 20, 2024 at 07:39:38PM +0100, Patrick Steinhardt wrote:
> 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.
Okay, I did find an issue where it does not work:
$ git clean -dfx
$ make GIT-USER-AGENT
$ cat GIT-USER-AGENT
git/
$ cat GIT-VERSION-FILE
cat: GIT-VERSION-FILE: No such file or directory
It does not generate the version file at all anymore when it's not an
explicit dependency. While I could of course add the missing dependency
I don't know whether there are any other implicit dependencies that
would be broken, as well. My gut feeling says "probably".
I'll go with my version instead.
Patrick
