| Message ID | 20241220-b4-pks-git-version-via-environment-v2-1-f1457a5e8c38@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | GIT-VERSION-GEN: fix overriding values | expand |
On Fri, Dec 20, 2024 at 01:22:45PM +0100, Patrick Steinhardt wrote: > diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN > index de0e63bdfbac263884e2ea328cc2ef11ace7a238..27f9d6a81f77248c652649ae21d0ec51b8f2d247 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 Hmm. If $GIT_VERSION is set, then we set $VN here... > -GIT_VERSION=$(expr "$VN" : v*'\(.*\)') > +# Only strip leading `v` in case we have derived VN manually. Otherwise we > +# retain whatever the user has set in their environment. > +if test -z "$GIT_VERSION" > +then > + GIT_VERSION=$(expr "$VN" : v*'\(.*\)') > +fi ...but later we ignore $VN completely. So it would work equally well with the first hunk dropped completely. However, having an entry in the cascading if/else does mean that we short-circuit the effort to run git-describe, etc. I don't think the old code ever did that (we'd generate the Makefile snippet in GIT-VERSION-FILE, read it back, and then make would still override the value from the snippet). So I dunno. I like keeping things simple, but I also like skipping unnecessary code, too. Maybe if the top hunk were: if test -n "$GIT_VERSION" then : do nothing, we will use this value verbatim elif ... that would make the intended flow more obvious. There are probably other ways to structure it, too. The whole $VN thing could be inside the: if test -z "$GIT_VERSION" block. Or alternatively, if each block of the if/else just ran expr and set $GIT_VERSION itself (perhaps with a one-liner helper function) then we wouldn't need $VN at all. I don't know how much trouble it's worth to refactor all this. Mostly I was just surprised to see the first hunk at all in this version. -Peff
On Fri, Dec 20, 2024 at 10:52:23AM -0500, Jeff King wrote: > On Fri, Dec 20, 2024 at 01:22:45PM +0100, Patrick Steinhardt wrote: > > > diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN > > index de0e63bdfbac263884e2ea328cc2ef11ace7a238..27f9d6a81f77248c652649ae21d0ec51b8f2d247 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 > > Hmm. If $GIT_VERSION is set, then we set $VN here... > > > -GIT_VERSION=$(expr "$VN" : v*'\(.*\)') > > +# Only strip leading `v` in case we have derived VN manually. Otherwise we > > +# retain whatever the user has set in their environment. > > +if test -z "$GIT_VERSION" > > +then > > + GIT_VERSION=$(expr "$VN" : v*'\(.*\)') > > +fi > > ...but later we ignore $VN completely. > > So it would work equally well with the first hunk dropped completely. > However, having an entry in the cascading if/else does mean that we > short-circuit the effort to run git-describe, etc. > > I don't think the old code ever did that (we'd generate the Makefile > snippet in GIT-VERSION-FILE, read it back, and then make would still > override the value from the snippet). > > So I dunno. I like keeping things simple, but I also like skipping > unnecessary code, too. Maybe if the top hunk were: > > if test -n "$GIT_VERSION" > then > : do nothing, we will use this value verbatim > elif ... > > that would make the intended flow more obvious. > > There are probably other ways to structure it, too. The whole $VN thing > could be inside the: > > if test -z "$GIT_VERSION" > > block. Or alternatively, if each block of the if/else just ran expr and > set $GIT_VERSION itself (perhaps with a one-liner helper function) then > we wouldn't need $VN at all. > > I don't know how much trouble it's worth to refactor all this. Mostly I > was just surprised to see the first hunk at all in this version. I think wrapping it all in `if test -z "$GIT_VERSION"` would be best. I was thinking about whether to do that refactoring, but I shied away from it due to the required reindentation. But I agree that the end result is a bit on the awkward side. You know, let me send another iteration where I just do it, now that we're two having the same thought. Patrick
Jeff King <peff@peff.net> writes: > So I dunno. I like keeping things simple, but I also like skipping > unnecessary code, too. Maybe if the top hunk were: > > if test -n "$GIT_VERSION" > then > : do nothing, we will use this value verbatim > elif ... > > that would make the intended flow more obvious. True. > There are probably other ways to structure it, too. The whole $VN thing > could be inside the: > > if test -z "$GIT_VERSION" > > block. Or alternatively, if each block of the if/else just ran expr and > set $GIT_VERSION itself (perhaps with a one-liner helper function) then > we wouldn't need $VN at all. True again. It has been quite a while since I wrote the original before the meson topic came up and the script hasn't changed for a long time (other than DEF_VER line for obvious reasons), but I think in that ancient version, $VN _was_ the variable to be looked at and GIT_VERSION did not even exist as a shell variable at all. If $GIT_VERSION is serving the same role as old $VN in the mesonified version, perhaps we should get rid of the $VN variable to clarify the new world order. > I don't know how much trouble it's worth to refactor all this. Mostly I > was just surprised to see the first hunk at all in this version. > > -Peff
On Fri, Dec 20, 2024 at 08:17:14AM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > So I dunno. I like keeping things simple, but I also like skipping > > unnecessary code, too. Maybe if the top hunk were: > > > > if test -n "$GIT_VERSION" > > then > > : do nothing, we will use this value verbatim > > elif ... > > > > that would make the intended flow more obvious. > > True. > > > There are probably other ways to structure it, too. The whole $VN thing > > could be inside the: > > > > if test -z "$GIT_VERSION" > > > > block. Or alternatively, if each block of the if/else just ran expr and > > set $GIT_VERSION itself (perhaps with a one-liner helper function) then > > we wouldn't need $VN at all. > > True again. It has been quite a while since I wrote the original > before the meson topic came up and the script hasn't changed for a > long time (other than DEF_VER line for obvious reasons), but I think > in that ancient version, $VN _was_ the variable to be looked at and > GIT_VERSION did not even exist as a shell variable at all. > > If $GIT_VERSION is serving the same role as old $VN in the > mesonified version, perhaps we should get rid of the $VN variable to > clarify the new world order. I think for now I'll keep $VN, if even just to discern the intermediate value without the leading "v" stripped from the final version that we have in "$GIT_VERSION". But I'll send a revised version where I make the control flow a bit more obvious. Thanks! Patrick
diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index de0e63bdfbac263884e2ea328cc2ef11ace7a238..27f9d6a81f77248c652649ae21d0ec51b8f2d247 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 { @@ -51,7 +54,13 @@ else VN="$DEF_VER" fi -GIT_VERSION=$(expr "$VN" : v*'\(.*\)') +# Only strip leading `v` in case we have derived VN manually. Otherwise we +# retain whatever the user has set in their environment. +if test -z "$GIT_VERSION" +then + GIT_VERSION=$(expr "$VN" : v*'\(.*\)') +fi + GIT_BUILT_FROM_COMMIT=$(git -C "$SOURCE_DIR" rev-parse -q --verify HEAD 2>/dev/null) GIT_DATE=$(git -C "$SOURCE_DIR" show --quiet --format='%as' 2>/dev/null) if test -z "$GIT_USER_AGENT"
GIT-VERSION-GEN tries to derive the version that Git is being built from via multiple different sources in the following order: 1. A file called "version" in the source tree's root directory, if it exists. 2. The current commit in case Git is built from a Git repository. 3. Otherwise, we use a fallback version stored in a variable which is bumped whenever a new Git version is getting tagged. It used to be possible to override the version by overriding the `GIT_VERSION` Makefile variable (e.g. `make GIT_VERSION=foo`). This worked somewhat by chance, only: `GIT-VERSION-GEN` would write the actual Git version into `GIT-VERSION-FILE`, not the overridden value, but when including the file into our Makefile we would not override the `GIT_VERSION` variable because it has already been set by the user. And because our Makefile used the variable to propagate the version to our build tools instead of using `GIT-VERSION-FILE` the resulting build artifacts used the overridden version. 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. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- GIT-VERSION-GEN | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)