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

From: Jeff King <peff@peff.net>
Date: Dec 20, 2024, 07:34 UTC
Message-ID: <20241220073437.GA2389154@coredump.intra.peff.net>
In-Reply-To: <20241219-b4-pks-git-version-via-environment-v1-1-9393af058240@pks.im>

On Thu, Dec 19, 2024 at 04:53:36PM +0100, Patrick Steinhardt wrote:

Show 10 quoted lines

> But that subtle mechanism broke with 4838deab65 (Makefile: refactor
> GIT-VERSION-GEN to be reusable, 2024-12-06) and subsequent commits
> because the version information is not propagated via the Makefile
> variable anymore, but instead via the files that `GIT-VERSION-GEN`
> started to write. And as the script never knew about the `GIT_VERSION`
> environment variable in the first place it uses one of the values listed
> above instead of the overridden value.
> 
> Fix this issue by making `GIT-VERSION-GEN` handle the case where
> `GIT_VERSION` has been set via the environment.

I think this is the right direction, but there are two subtleties we might want to address. The first is that you are adjusting $VN and not $GIT_VERSION itself:

Show 16 quoted lines

> 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.

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.

-Peff

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