| Message ID | 20241220-b4-pks-git-version-via-environment-v2-4-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:48PM +0100, Patrick Steinhardt wrote: > In preceding commits we fixed that build info set via e.g. `make > GIT_VERSION=foo` didn't get propagated to GIT-VERSION-GEN. Similarly > though, setting build info via "config.mak" does not work anymore either > because the variables are only declared as Makefile variables and thus > aren't accessible by the script. > > Fix the issue by exporting those variables via "shared.mak". This also > allows us to deduplicate the export of GIT_USER_AGENT. This looks good. It fixes the issue, and I am happy that: > asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE > - $(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@ > + $(QUIET_GEN)$(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@ ...these spots get even simpler. -Peff
On Fri, Dec 20, 2024 at 10:54:33AM -0500, Jeff King wrote: > On Fri, Dec 20, 2024 at 01:22:48PM +0100, Patrick Steinhardt wrote: > > > In preceding commits we fixed that build info set via e.g. `make > > GIT_VERSION=foo` didn't get propagated to GIT-VERSION-GEN. Similarly > > though, setting build info via "config.mak" does not work anymore either > > because the variables are only declared as Makefile variables and thus > > aren't accessible by the script. > > > > Fix the issue by exporting those variables via "shared.mak". This also > > allows us to deduplicate the export of GIT_USER_AGENT. > > This looks good. It fixes the issue, and I am happy that: > > > asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE > > - $(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@ > > + $(QUIET_GEN)$(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@ > > ...these spots get even simpler. Meh. I just noticed that this doesn't work: we include GIT-VERSION-FILE and export its value, and consequently any subsequent invocation of GIT-VERSION-GEN will continue to use the value that we have in GIT-VERSION-FILE. So it's effectively only computed the first time. This would all be much simpler if we didn't include the file in the first place. In Documentation/Makefile we don't indeed use it anymore. But in the top-level Makefile we do use it to generate the name of a couple of archives. I'll have a look there. Patrick
On Fri, Dec 20, 2024 at 05:47:14PM +0100, Patrick Steinhardt wrote: > > This looks good. It fixes the issue, and I am happy that: > > > > > asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE > > > - $(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@ > > > + $(QUIET_GEN)$(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@ > > > > ...these spots get even simpler. > > Meh. I just noticed that this doesn't work: we include GIT-VERSION-FILE > and export its value, and consequently any subsequent invocation of > GIT-VERSION-GEN will continue to use the value that we have in > GIT-VERSION-FILE. So it's effectively only computed the first time. I'm not sure what you mean. I wondered earlier if we might into a chicken-and-egg problem like that, but I tested and it seemed to work fine. The rule for GIT-VERSION-FILE means we'll build it before make reads it, so that first run of it will get the updated value. And: make GIT_VERSION=foo && bin-wrappers/git version make GIT_VERSION=bar && bin-wrappers/git version does what you'd expect. And the docs work the same way: cd Documentation make GIT_VERSION=foo git.1 && man -l git.1 make GIT_VERSION=bar git.1 && man -l git.1 Is there a case you found that doesn't work? -Peff
On Fri, Dec 20, 2024 at 12:51:36PM -0500, Jeff King wrote: > On Fri, Dec 20, 2024 at 05:47:14PM +0100, Patrick Steinhardt wrote: > > > > This looks good. It fixes the issue, and I am happy that: > > > > > > > asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE > > > > - $(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@ > > > > + $(QUIET_GEN)$(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@ > > > > > > ...these spots get even simpler. > > > > Meh. I just noticed that this doesn't work: we include GIT-VERSION-FILE > > and export its value, and consequently any subsequent invocation of > > GIT-VERSION-GEN will continue to use the value that we have in > > GIT-VERSION-FILE. So it's effectively only computed the first time. > > I'm not sure what you mean. > > I wondered earlier if we might into a chicken-and-egg problem like that, > but I tested and it seemed to work fine. The rule for GIT-VERSION-FILE > means we'll build it before make reads it, so that first run of it will > get the updated value. And: > > make GIT_VERSION=foo && bin-wrappers/git version > make GIT_VERSION=bar && bin-wrappers/git version > > does what you'd expect. And the docs work the same way: > > cd Documentation > make GIT_VERSION=foo git.1 && man -l git.1 > make GIT_VERSION=bar git.1 && man -l git.1 > > 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. I've got a version now that works for all cases, but I had to use Makefile templates and some shuffling to make it work. Patrick
On Fri, Dec 20, 2024 at 07:02:10PM +0100, Patrick Steinhardt wrote: > On Fri, Dec 20, 2024 at 12:51:36PM -0500, Jeff King wrote: > > On Fri, Dec 20, 2024 at 05:47:14PM +0100, Patrick Steinhardt wrote: > > > > > > This looks good. It fixes the issue, and I am happy that: > > > > > > > > > asciidoctor-extensions.rb: asciidoctor-extensions.rb.in FORCE > > > > > - $(QUIET_GEN)GIT_USER_AGENT="$(GIT_USER_AGENT)" $(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@ > > > > > + $(QUIET_GEN)$(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@ > > > > > > > > ...these spots get even simpler. > > > > > > Meh. I just noticed that this doesn't work: we include GIT-VERSION-FILE > > > and export its value, and consequently any subsequent invocation of > > > GIT-VERSION-GEN will continue to use the value that we have in > > > GIT-VERSION-FILE. So it's effectively only computed the first time. > > > > I'm not sure what you mean. > > > > I wondered earlier if we might into a chicken-and-egg problem like that, > > but I tested and it seemed to work fine. The rule for GIT-VERSION-FILE > > means we'll build it before make reads it, so that first run of it will > > get the updated value. And: > > > > make GIT_VERSION=foo && bin-wrappers/git version > > make GIT_VERSION=bar && bin-wrappers/git version > > > > does what you'd expect. And the docs work the same way: > > > > cd Documentation > > make GIT_VERSION=foo git.1 && man -l git.1 > > make GIT_VERSION=bar git.1 && man -l git.1 > > > > 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. > > I've got a version now that works for all cases, but I had to use > Makefile templates and some shuffling to make it work. The root of the problem is this [1]: To this end, after reading in all makefiles make will consider each as a goal target, in the order in which they were processed, and attempt to update it. If parallel builds (see Parallel Execution) are enabled then makefiles will be rebuilt in parallel as well. So the Makefile will _first_ read in all includes before deciding whether or not it needs to regenerate any of them. So we already export the current GIT_VERSION in case "GIT-VERSION-FILE" exists at the time where we run "GIT-VERSION-GEN". That behaviour is quite surprising to me, but it seems to work as designed. Patrick [1]: https://www.gnu.org/software/make/manual/html_node/Remaking-Makefiles.html
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. # 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). If we can stop including it as a Makefile snippet entirely, I think that is easier to reason about. -Peff
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. Patrick
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
diff --git a/Documentation/Makefile b/Documentation/Makefile index cee88dbda66265141b87d5e5c16bf86df22fa4ef..4c652dfa14f6af2c1374e2f83d3311b36dd297c4 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -211,10 +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)/.." $< $@ + $(QUIET_GEN)$(SHELL_PATH) ../GIT-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)/.." $< $@ + $(QUIET_GEN)$(SHELL_PATH) ../GIT-VERSION-GEN "$(shell pwd)/.." $< $@ endif ASCIIDOC_DEPS += docinfo.html diff --git a/Makefile b/Makefile index 695a9d9765daf864605002d572129bae7a8c4e40..788f6ee1721850e75f19fe2181f578ad2e783043 100644 --- a/Makefile +++ b/Makefile @@ -2512,7 +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)" $< $@ + $(QUIET_GEN)$(SHELL_PATH) ./GIT-VERSION-GEN "$(shell pwd)" $< $@ version.sp version.s version.o: version-def.h diff --git a/shared.mak b/shared.mak index 29bebd30d8acbce9f50661cef48ecdbae1e41f5a..8cd132b5a03b85d4eabc3819eb3d1f64d39afa47 100644 --- a/shared.mak +++ b/shared.mak @@ -116,3 +116,10 @@ endef define libpath_template -L$(1) $(if $(filter-out -L,$(CC_LD_DYNPATH)),$(CC_LD_DYNPATH)$(1)) endef + +# Export build information so that variables defined in config.mak can be read +# by GIT-VERSION-GEN. +export GIT_BUILT_FROM_COMMIT +export GIT_DATE +export GIT_USER_AGENT +export GIT_VERSION
In preceding commits we fixed that build info set via e.g. `make GIT_VERSION=foo` didn't get propagated to GIT-VERSION-GEN. Similarly though, setting build info via "config.mak" does not work anymore either because the variables are only declared as Makefile variables and thus aren't accessible by the script. Fix the issue by exporting those variables via "shared.mak". This also allows us to deduplicate the export of GIT_USER_AGENT. Suggested-by: Jeff King <peff@peff.net> Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/Makefile | 4 ++-- Makefile | 2 +- shared.mak | 7 +++++++ 3 files changed, 10 insertions(+), 3 deletions(-)