Re: [PATCH 1/2] GIT-VERSION-GEN: fix overriding version via environment

[Patrick Steinhardt <ps@pks.im>]

On Fri, Dec 20, 2024 at 02:34:37AM -0500, Jeff King wrote:
> On Thu, Dec 19, 2024 at 04:53:36PM +0100, Patrick Steinhardt wrote:
> > diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> > index de0e63bdfbac263884e2ea328cc2ef11ace7a238..787c6cfd04f0a43d0c1c8a6690185d26ccf2fc2f 100755
> > --- a/GIT-VERSION-GEN
> > +++ b/GIT-VERSION-GEN
> > @@ -29,7 +29,10 @@ export GIT_CEILING_DIRECTORIES
> >  
> >  # First see if there is a version file (included in release tarballs),
> >  # then try git-describe, then default.
> > -if test -f "$SOURCE_DIR"/version
> > +if test -n "$GIT_VERSION"
> > +then
> > +    VN="$GIT_VERSION"
> > +elif test -f "$SOURCE_DIR"/version
> >  then
> >  	VN=$(cat "$SOURCE_DIR"/version) || VN="$DEF_VER"
> >  elif {
> 
> Later we process $VN into $GIT_VERSION, but after removing the leading
> "v" (which would usually be there from the tag name):
> 
>   GIT_VERSION=$(expr "$VN" : v*'\(.*\)')
> 
> So if I do:
> 
>   make GIT_VERSION=very-special
> 
> with v2.47 I'd end up with the version "very-special". But after your
> patch, it is "ery-special".
> 
> I'd guess it's unlikely to come up in practice, but if we are trying to
> restore the old behavior, that's one difference.

Ah, indeed, will fix.

> The second is that the value is read from the environment, but make will
> not always put its variables into the environment. Ones given on the
> command line are, so:
> 
>   make GIT_VERSION=foo
> 
> works as before. But:
> 
>   echo 'GIT_VERSION = foo' >config.mak
>   make
> 
> will not, as the variable isn't set in the environment. The invocation
> of GIT-VERSION-GEN already passes along GIT_USER_AGENT explicitly:
> 
>   version-def.h: version-def.h.in GIT-VERSION-GEN GIT-VERSION-FILE GIT-USER-AGENT
>           $(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@+
>           @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> 
> Should we do the same thing for GIT_VERSION? And GIT_DATE, etc? If we're
> going to do many of these, it might also be easier to just add "export
> GIT_VERSION", etc, in the Makefile.

I guess. It'll become quite painful to do this at every callsite, so
I'll add another commit on top to introduce a call template that does
all of this for us.

> PS I don't know if meson.build would need something similar. It does not
>    even pass through GIT_USER_AGENT now.

It's easy enough to do, so why not?

Patrick