| Message ID | 20241219-b4-pks-git-version-via-environment-v1-1-9393af058240@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | GIT-VERSION-GEN: fix overriding values | expand |
Patrick Steinhardt <ps@pks.im> writes: > worked somewhat by chance, only: ... > > But that subtle mechanism broke with 4838deab65 (Makefile: refactor > GIT-VERSION-GEN to be reusable, 2024-12-06) and subsequent commits With such a nice analysis, it does not look like it was "by chance" working, though ;-) And ... > 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. ... the "fix" sounds very much the logical and only correct solution. Thanks, queued. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > GIT-VERSION-GEN | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > 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 {
On Thu, Dec 19, 2024 at 04:53:36PM +0100, Patrick Steinhardt wrote: > 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: > 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.
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
On Fri, Dec 20, 2024 at 09:45:36AM +0100, Patrick Steinhardt wrote: > > 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. Is there any reason not to just do: export GIT_VERSION export GIT_DATE export GIT_BUILT_FROM_COMMIT export GIT_USER_AGENT in shared.mak? Then you only have to do it once, and no need for templates. -Peff
On Fri, Dec 20, 2024 at 03:56:26AM -0500, Jeff King wrote: > On Fri, Dec 20, 2024 at 09:45:36AM +0100, Patrick Steinhardt wrote: > > > > 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. > > Is there any reason not to just do: > > export GIT_VERSION > export GIT_DATE > export GIT_BUILT_FROM_COMMIT > export GIT_USER_AGENT > > in shared.mak? Then you only have to do it once, and no need for > templates. You could do that, yeah, but the user needs to be aware that they can. I'm happy to not go down that path and live with the above solution. Alternatively, this would be what the call template would look like. Patrick diff --git a/Documentation/Makefile b/Documentation/Makefile index 3392e1ce7e..a7cb885b67 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -211,12 +211,10 @@ XMLTO_EXTRA += --skip-validation XMLTO_EXTRA += -x manpage.xsl asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE - $(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 + $(QUIET_GEN)$(call version_gen,$(shell pwd)/..,$<,$@) else asciidoc.conf: asciidoc.conf.in FORCE - $(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 + $(QUIET_GEN)$(call version_gen,$(shell pwd)/..,$<,$@) endif ASCIIDOC_DEPS += docinfo.html diff --git a/Makefile b/Makefile index 79739a13d2..9cfe3d0aa9 100644 --- a/Makefile +++ b/Makefile @@ -593,7 +593,7 @@ include shared.mak GIT-VERSION-FILE: FORCE @OLD=$$(cat $@ 2>/dev/null || :) && \ - $(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" GIT-VERSION-FILE.in $@ && \ + $(call version_gen,"$(shell pwd)",GIT-VERSION-FILE.in,$@) && \ NEW=$$(cat $@ 2>/dev/null || :) && \ if test "$$OLD" != "$$NEW"; then echo "$$NEW" >&2; fi -include GIT-VERSION-FILE @@ -2512,8 +2512,7 @@ pager.sp pager.s pager.o: EXTRA_CPPFLAGS = \ -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)' 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 + $(QUIET_GEN)$(call version_gen,"$(shell pwd)",$<,$@) version.sp version.s version.o: version-def.h @@ -2554,8 +2553,7 @@ $(SCRIPT_SH_GEN) $(SCRIPT_LIB) : % : %.sh generate-script.sh GIT-BUILD-OPTIONS G mv $@+ $@ git.rc: git.rc.in GIT-VERSION-GEN GIT-VERSION-FILE - $(QUIET_GEN)$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@+ - @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi + $(QUIET_GEN)$(call version_gen,"$(shell pwd)",$<,$@) git.res: git.rc GIT-PREFIX $(QUIET_RC)$(RC) -i $< -o $@ diff --git a/shared.mak b/shared.mak index 29bebd30d8..8e0a19691f 100644 --- a/shared.mak +++ b/shared.mak @@ -116,3 +116,14 @@ endef define libpath_template -L$(1) $(if $(filter-out -L,$(CC_LD_DYNPATH)),$(CC_LD_DYNPATH)$(1)) endef + +# Populate build information into a file via GIT-VERSION-GEN. Requires the +# absolute path to the root source directory as well as input and output files +# as arguments, in that order. +define version_gen +GIT_BUILT_FROM_COMMIT="$(GIT_BUILT_FROM_COMMIT)" \ +GIT_DATE="$(GIT_DATE)" \ +GIT_USER_AGENT="$(GIT_USER_AGENT)" \ +GIT_VERSION="$(GIT_VERSION)" \ +$(SHELL_PATH) "$(1)/GIT-VERSION-GEN" "$(1)" "$(2)" "$(3)" +endef
On Fri, Dec 20, 2024 at 10:31:30AM +0100, Patrick Steinhardt wrote: > > > 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. > > > > Is there any reason not to just do: > > > > export GIT_VERSION > > export GIT_DATE > > export GIT_BUILT_FROM_COMMIT > > export GIT_USER_AGENT > > > > in shared.mak? Then you only have to do it once, and no need for > > templates. > > You could do that, yeah, but the user needs to be aware that they can. > I'm happy to not go down that path and live with the above solution. > Alternatively, this would be what the call template would look like. I meant that _we_ would mark those variables for export ourselves (in shared.mak, which is used by all of our Makefiles, not the user's config.mak). I.e., this: diff --git a/shared.mak b/shared.mak index 29bebd30d8..4aa7dbf5e0 100644 --- a/shared.mak +++ b/shared.mak @@ -116,3 +116,5 @@ endef define libpath_template -L$(1) $(if $(filter-out -L,$(CC_LD_DYNPATH)),$(CC_LD_DYNPATH)$(1)) endef + +export GIT_VERSION which makes: echo 'GIT_VERSION = foo' >config.mak make behave as it used to (when coupled with the fix you already sent to respect the variable within the version-gen script). -Peff
On Fri, Dec 20, 2024 at 06:17:16AM -0500, Jeff King wrote: > On Fri, Dec 20, 2024 at 10:31:30AM +0100, Patrick Steinhardt wrote: > > > > > 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. > > > > > > Is there any reason not to just do: > > > > > > export GIT_VERSION > > > export GIT_DATE > > > export GIT_BUILT_FROM_COMMIT > > > export GIT_USER_AGENT > > > > > > in shared.mak? Then you only have to do it once, and no need for > > > templates. > > > > You could do that, yeah, but the user needs to be aware that they can. > > I'm happy to not go down that path and live with the above solution. > > Alternatively, this would be what the call template would look like. > > I meant that _we_ would mark those variables for export ourselves (in > shared.mak, which is used by all of our Makefiles, not the user's > config.mak). > > I.e., this: > > diff --git a/shared.mak b/shared.mak > index 29bebd30d8..4aa7dbf5e0 100644 > --- a/shared.mak > +++ b/shared.mak > @@ -116,3 +116,5 @@ endef > define libpath_template > -L$(1) $(if $(filter-out -L,$(CC_LD_DYNPATH)),$(CC_LD_DYNPATH)$(1)) > endef > + > +export GIT_VERSION > > which makes: > > echo 'GIT_VERSION = foo' >config.mak > make > > behave as it used to (when coupled with the fix you already sent to > respect the variable within the version-gen script). Ah, I misread "shared.mak" and thought you meant "config.mak". That makes more sense then, thanks! Patrick
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 {
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 | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)