| Message ID | 20210408212915.3060286-1-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 56550ea718091e5868fdb241b5dfa882923db70c |
| Headers | show |
| Series | Makefile: add missing dependencies of 'config-list.h' | expand |
On Thu, Apr 08 2021, SZEDER Gábor wrote: > We auto-generate the list of supported configuration variables from > 'Documentation/config/*.txt', and that list used to be created by the > 'generate-cmdlist.sh' helper script and stored in the 'command-list.h' > header. Commit 709df95b78 (help: move list_config_help to > builtin/help, 2020-04-16) extracted this into a dedicated > 'generate-configlist.sh' script and 'config-list.h' header, and added > a new target in the 'Makefile' as well, but while doing so it forgot > to extract the dependencies of the latter. Consequently, since then > 'config-list.h' is not re-generated when 'Documentation/config/*.txt' > is updated, while 'command-list.h' is re-generated unnecessarily: > > $ touch Documentation/config/log.txt > $ make -j4 > GEN command-list.h > CC help.o > AR libgit.a > > Fix this and list all config-related documentation files as > dependencies of 'config-list.h' and remove them from the dependencies > of 'command-list.h'. > > $ touch Documentation/config/log.txt > $ make > GEN config-list.h > CC builtin/help.o > LINK git > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- > Makefile | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index 5a022367d4..2c41f125e0 100644 > --- a/Makefile > +++ b/Makefile > @@ -2151,13 +2151,13 @@ $(BUILT_INS): git$X > > config-list.h: generate-configlist.sh > > -config-list.h: > +config-list.h: Documentation/*config.txt Documentation/config/*.txt > $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \ > >$@+ && mv $@+ $@ > > command-list.h: generate-cmdlist.sh command-list.txt > > -command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt > +command-list.h: $(wildcard Documentation/git*.txt) > $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \ > $(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \ > command-list.txt >$@+ && mv $@+ $@ This change makes sense. I have a not-yet-submitted patch series where I added some more config/*/*.txt that wouldn't be caught by this rule, I'd updated the Documentation/Makefile, but missed this part in the top-level Makefile. So a relation question: Does anyone actually prefer this state of affairs of having a Makefile, Documentation/Makefile, t/Makefile t/perf/Makefile and template/Makefile? It seems to me with ever-closer coupling between them that it's getting to be more of a hassle to manage state between them than it would be to just move them all into one big Makefile.
On Fri, Apr 09, 2021 at 12:08:23AM +0200, Ævar Arnfjörð Bjarmason wrote: > > -config-list.h: > > +config-list.h: Documentation/*config.txt Documentation/config/*.txt > > $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \ > > >$@+ && mv $@+ $@ > > > > command-list.h: generate-cmdlist.sh command-list.txt > > > > -command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt > > +command-list.h: $(wildcard Documentation/git*.txt) > > $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \ > > $(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \ > > command-list.txt >$@+ && mv $@+ $@ > > This change makes sense. I agree it looks like it's moving in the right direction, but I am slightly puzzled by the existing code. Why do we need to use $(wildcard) for git*.txt, but not for the others? > I have a not-yet-submitted patch series where I added some more > config/*/*.txt that wouldn't be caught by this rule, I'd updated the > Documentation/Makefile, but missed this part in the top-level Makefile. > > So a relation question: Does anyone actually prefer this state of > affairs of having a Makefile, Documentation/Makefile, t/Makefile > t/perf/Makefile and template/Makefile? > > It seems to me with ever-closer coupling between them that it's getting > to be more of a hassle to manage state between them than it would be to > just move them all into one big Makefile. Yes, I'm generally a fan of avoiding recursive make when we can. I think the caveats are: - it would be nice to continue to have stub Makefiles in sub-directories that trigger the main one (so "cd t && make" continues to work, for example). - we may need some cleanup of parts of the top-level Makefile which are triggered without dependencies (e.g., I think we unconditionally run some scripts to compute GIT_VERSION in the top-level; this is already a bit wasteful, but may get even more so as we add more rules from sub-directories). Mostly my argument against it (and why I haven't purused it) would be: it sounds like a lot of work and risk of regression, and the current system seems pretty fine in practice. -Peff
On Thu, Apr 08, 2021 at 07:40:41PM -0400, Jeff King wrote: > On Fri, Apr 09, 2021 at 12:08:23AM +0200, Ævar Arnfjörð Bjarmason wrote: > > > > -config-list.h: > > > +config-list.h: Documentation/*config.txt Documentation/config/*.txt > > > $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \ > > > >$@+ && mv $@+ $@ > > > > > > command-list.h: generate-cmdlist.sh command-list.txt > > > > > > -command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt > > > +command-list.h: $(wildcard Documentation/git*.txt) > > > $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \ > > > $(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \ > > > command-list.txt >$@+ && mv $@+ $@ > > > > This change makes sense. > > I agree it looks like it's moving in the right direction, but I am > slightly puzzled by the existing code. Why do we need to use $(wildcard) > for git*.txt, but not for the others? We don't need $(wildcard) for git*.txt either, because 'make' expands wildcards in prerequisites, see e.g.: https://www.gnu.org/software/make/manual/html_node/Wildcard-Examples.html On a related note: all config variables are now listed in Documentation/config/*.txt; Documentation/*config.txt doesn't contain any, so that could be removed.
On Fri, Apr 09 2021, Jeff King wrote: > On Fri, Apr 09, 2021 at 12:08:23AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> > -config-list.h: >> > +config-list.h: Documentation/*config.txt Documentation/config/*.txt >> > $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \ >> > >$@+ && mv $@+ $@ >> > >> > command-list.h: generate-cmdlist.sh command-list.txt >> > >> > -command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt >> > +command-list.h: $(wildcard Documentation/git*.txt) >> > $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \ >> > $(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \ >> > command-list.txt >$@+ && mv $@+ $@ >> >> This change makes sense. > > I agree it looks like it's moving in the right direction, but I am > slightly puzzled by the existing code. Why do we need to use $(wildcard) > for git*.txt, but not for the others? > >> I have a not-yet-submitted patch series where I added some more >> config/*/*.txt that wouldn't be caught by this rule, I'd updated the >> Documentation/Makefile, but missed this part in the top-level Makefile. >> >> So a relation question: Does anyone actually prefer this state of >> affairs of having a Makefile, Documentation/Makefile, t/Makefile >> t/perf/Makefile and template/Makefile? >> >> It seems to me with ever-closer coupling between them that it's getting >> to be more of a hassle to manage state between them than it would be to >> just move them all into one big Makefile. > > Yes, I'm generally a fan of avoiding recursive make when we can. I think > the caveats are: > > - it would be nice to continue to have stub Makefiles in > sub-directories that trigger the main one (so "cd t && make" > continues to work, for example). Yeah, we should definitely keep those in place. I also wonder if various rules for local wildcards will be more complex when they need to reach into subdirectories. > - we may need some cleanup of parts of the top-level Makefile which > are triggered without dependencies (e.g., I think we unconditionally > run some scripts to compute GIT_VERSION in the top-level; this is > already a bit wasteful, but may get even more so as we add more > rules from sub-directories). > > Mostly my argument against it (and why I haven't purused it) would be: > it sounds like a lot of work and risk of regression, and the current > system seems pretty fine in practice. One edge case I discovered the other day but didn't bother debugging much was make at the top-level failing because "doc.dep" in Documentation/Makefile uses this pattern: rm x && script >x Which would normally work in one Makefile, but in this case two rules in the top-level called unrelated "make -C Documentation [...]", so both of those sub-processes end up needing to generate the doc.dep, and they race each other. Another thing fixed (or, worked around) with a wider application of [1]. 1. https://lore.kernel.org/git/patch-3.6-96e2338ed8e-20210329T161723Z-avarab@gmail.com/
SZEDER Gábor <szeder.dev@gmail.com> writes: > On Thu, Apr 08, 2021 at 07:40:41PM -0400, Jeff King wrote: >> On Fri, Apr 09, 2021 at 12:08:23AM +0200, Ævar Arnfjörð Bjarmason wrote: >> >> > > -config-list.h: >> > > +config-list.h: Documentation/*config.txt Documentation/config/*.txt >> > > $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \ >> > > >$@+ && mv $@+ $@ >> > > >> > > command-list.h: generate-cmdlist.sh command-list.txt >> > > >> > > -command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt >> > > +command-list.h: $(wildcard Documentation/git*.txt) >> > > $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \ >> > > $(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \ >> > > command-list.txt >$@+ && mv $@+ $@ >> > >> > This change makes sense. >> >> I agree it looks like it's moving in the right direction, but I am >> slightly puzzled by the existing code. Why do we need to use $(wildcard) >> for git*.txt, but not for the others? > > We don't need $(wildcard) for git*.txt either, because 'make' expands > wildcards in prerequisites, see e.g.: > > https://www.gnu.org/software/make/manual/html_node/Wildcard-Examples.html > > > On a related note: all config variables are now listed in > Documentation/config/*.txt; Documentation/*config.txt doesn't contain > any, so that could be removed. Is it OK for me to keep expecting an update to the patch happen soon? Thanks.
On Fri, Apr 16, 2021 at 12:03:54PM -0700, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > > > On Thu, Apr 08, 2021 at 07:40:41PM -0400, Jeff King wrote: > >> On Fri, Apr 09, 2021 at 12:08:23AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> > >> > > -config-list.h: > >> > > +config-list.h: Documentation/*config.txt Documentation/config/*.txt > >> > > $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \ > >> > > >$@+ && mv $@+ $@ > >> > > > >> > > command-list.h: generate-cmdlist.sh command-list.txt > >> > > > >> > > -command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt > >> > > +command-list.h: $(wildcard Documentation/git*.txt) > >> > > $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \ > >> > > $(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \ > >> > > command-list.txt >$@+ && mv $@+ $@ > >> > > >> > This change makes sense. > >> > >> I agree it looks like it's moving in the right direction, but I am > >> slightly puzzled by the existing code. Why do we need to use $(wildcard) > >> for git*.txt, but not for the others? > > > > We don't need $(wildcard) for git*.txt either, because 'make' expands > > wildcards in prerequisites, see e.g.: > > > > https://www.gnu.org/software/make/manual/html_node/Wildcard-Examples.html > > > > > > On a related note: all config variables are now listed in > > Documentation/config/*.txt; Documentation/*config.txt doesn't contain > > any, so that could be removed. > > Is it OK for me to keep expecting an update to the patch happen soon? No, I think this is a good bugfix patch that stands on its own, and further cleanups could be done independently on top and should not block this patch from being merged to master. I'm inclined to think that this should be merged and then 'so/log-diff-merge' should be queued on top because of the subtle interaction between this bug, the new config variable and the completion tests.
SZEDER Gábor <szeder.dev@gmail.com> writes: > On Fri, Apr 16, 2021 at 12:03:54PM -0700, Junio C Hamano wrote: >> SZEDER Gábor <szeder.dev@gmail.com> writes: >> >> > On Thu, Apr 08, 2021 at 07:40:41PM -0400, Jeff King wrote: >> ... >> >> I agree it looks like it's moving in the right direction, but I am >> >> slightly puzzled by the existing code. Why do we need to use $(wildcard) >> >> for git*.txt, but not for the others? >> > >> > We don't need $(wildcard) for git*.txt either, because 'make' expands >> > wildcards in prerequisites, see e.g.: >> > >> > https://www.gnu.org/software/make/manual/html_node/Wildcard-Examples.html >> > >> > >> > On a related note: all config variables are now listed in >> > Documentation/config/*.txt; Documentation/*config.txt doesn't contain >> > any, so that could be removed. >> >> Is it OK for me to keep expecting an update to the patch happen soon? > > No, I think this is a good bugfix patch that stands on its own, and > further cleanups could be done independently on top and should not > block this patch from being merged to master. That's fair. Thanks.
diff --git a/Makefile b/Makefile index 5a022367d4..2c41f125e0 100644 --- a/Makefile +++ b/Makefile @@ -2151,13 +2151,13 @@ $(BUILT_INS): git$X config-list.h: generate-configlist.sh -config-list.h: +config-list.h: Documentation/*config.txt Documentation/config/*.txt $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \ >$@+ && mv $@+ $@ command-list.h: generate-cmdlist.sh command-list.txt -command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt +command-list.h: $(wildcard Documentation/git*.txt) $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \ $(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \ command-list.txt >$@+ && mv $@+ $@
We auto-generate the list of supported configuration variables from 'Documentation/config/*.txt', and that list used to be created by the 'generate-cmdlist.sh' helper script and stored in the 'command-list.h' header. Commit 709df95b78 (help: move list_config_help to builtin/help, 2020-04-16) extracted this into a dedicated 'generate-configlist.sh' script and 'config-list.h' header, and added a new target in the 'Makefile' as well, but while doing so it forgot to extract the dependencies of the latter. Consequently, since then 'config-list.h' is not re-generated when 'Documentation/config/*.txt' is updated, while 'command-list.h' is re-generated unnecessarily: $ touch Documentation/config/log.txt $ make -j4 GEN command-list.h CC help.o AR libgit.a Fix this and list all config-related documentation files as dependencies of 'config-list.h' and remove them from the dependencies of 'command-list.h'. $ touch Documentation/config/log.txt $ make GEN config-list.h CC builtin/help.o LINK git Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)