* [PATCH] Documentation/Makefile: fix lint-docs mkdir dependency
@ 2021-10-26 7:31 Jeff King
2021-10-26 10:05 ` Ævar Arnfjörð Bjarmason
2021-10-28 0:03 ` Junio C Hamano
0 siblings, 2 replies; 9+ messages in thread
From: Jeff King @ 2021-10-26 7:31 UTC (permalink / raw)
To: git; +Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano
Since 8650c6298c (doc lint: make "lint-docs" non-.PHONY, 2021-10-15), we
put the output for gitlink linter into .build/lint-docs/gitlink. There
are order-only dependencies to create the sequence of subdirs like:
.build/lint-docs: | .build
$(QUIET)mkdir $@
.build/lint-docs/gitlink: | .build/lint-docs
$(QUIET)mkdir $@
where each level has to depend on the prior one (since the parent
directory must exist for us to create something inside it). But the
"howto" and "config" subdirectories of gitlink have the wrong
dependency; they depend on "lint-docs", not "lint-docs/gitlink".
This usually works out, because the LINT_DOCS_GITLINK targets which
depend on "gitlink/howto" also depend on just "gitlink", so the
directory gets created anyway. But since we haven't given make an
explicit ordering, things can racily happen out of order.
If you stick a "sleep 1" in the rule to build "gitlink" like this:
## Lint: gitlink
.build/lint-docs/gitlink: | .build/lint-docs
- $(QUIET)mkdir $@
+ $(QUIET)sleep 1 && mkdir $@
then "make clean; make lint-docs" will fail reliably. Or you can see it
as-is just by building the directory in isolation:
$ make clean
[...]
$ make .build/lint-docs/gitlink/howto
GEN mergetools-list.made
GEN cmd-list.made
GEN doc.dep
SUBDIR ../
make[1]: 'GIT-VERSION-FILE' is up to date.
SUBDIR ../
make[1]: 'GIT-VERSION-FILE' is up to date.
mkdir: cannot create directory ‘.build/lint-docs/gitlink/howto’: No such file or directory
make: *** [Makefile:476: .build/lint-docs/gitlink/howto] Error 1
The fix is easy: we just need to depend on the correct parent directory.
Signed-off-by: Jeff King <peff@peff.net>
---
The problem starts in ab/fix-make-lint-docs, which is in master.
I wasn't able to trigger the problem locally even with running 'make
clean; make lint-docs' in a loop, but I did see it in the wild in a CI
documentation job:
https://github.com/peff/git/runs/4005766641?check_suite_focus=true#step:4:60
It would have been a lot easier to diagnose from the CI output if the
mkdir lines weren't silent. I.e., if we had a $(QUIET_MKDIR) which
printed "MKDIR $@" rather than nothing at all.
Documentation/Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 911b6bf79c..ed656db2ae 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -472,9 +472,9 @@ print-man1:
## Lint: gitlink
.build/lint-docs/gitlink: | .build/lint-docs
$(QUIET)mkdir $@
-.build/lint-docs/gitlink/howto: | .build/lint-docs
+.build/lint-docs/gitlink/howto: | .build/lint-docs/gitlink
$(QUIET)mkdir $@
-.build/lint-docs/gitlink/config: | .build/lint-docs
+.build/lint-docs/gitlink/config: | .build/lint-docs/gitlink
$(QUIET)mkdir $@
LINT_DOCS_GITLINK = $(patsubst %.txt,.build/lint-docs/gitlink/%.ok,$(HOWTO_TXT) $(DOC_DEP_TXT))
$(LINT_DOCS_GITLINK): | .build/lint-docs/gitlink
--
2.33.1.1387.g97d4a0c3a8
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] Documentation/Makefile: fix lint-docs mkdir dependency 2021-10-26 7:31 [PATCH] Documentation/Makefile: fix lint-docs mkdir dependency Jeff King @ 2021-10-26 10:05 ` Ævar Arnfjörð Bjarmason 2021-10-26 21:18 ` Jeff King 2021-10-28 0:03 ` Junio C Hamano 1 sibling, 1 reply; 9+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-26 10:05 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano On Tue, Oct 26 2021, Jeff King wrote: > Since 8650c6298c (doc lint: make "lint-docs" non-.PHONY, 2021-10-15), we > put the output for gitlink linter into .build/lint-docs/gitlink. There > are order-only dependencies to create the sequence of subdirs like: > > .build/lint-docs: | .build > $(QUIET)mkdir $@ > .build/lint-docs/gitlink: | .build/lint-docs > $(QUIET)mkdir $@ > > where each level has to depend on the prior one (since the parent > directory must exist for us to create something inside it). But the > "howto" and "config" subdirectories of gitlink have the wrong > dependency; they depend on "lint-docs", not "lint-docs/gitlink". > > This usually works out, because the LINT_DOCS_GITLINK targets which > depend on "gitlink/howto" also depend on just "gitlink", so the > directory gets created anyway. But since we haven't given make an > explicit ordering, things can racily happen out of order. > > If you stick a "sleep 1" in the rule to build "gitlink" like this: > > ## Lint: gitlink > .build/lint-docs/gitlink: | .build/lint-docs > - $(QUIET)mkdir $@ > + $(QUIET)sleep 1 && mkdir $@ > > then "make clean; make lint-docs" will fail reliably. Or you can see it > as-is just by building the directory in isolation: > > $ make clean > [...] > $ make .build/lint-docs/gitlink/howto > GEN mergetools-list.made > GEN cmd-list.made > GEN doc.dep > SUBDIR ../ > make[1]: 'GIT-VERSION-FILE' is up to date. > SUBDIR ../ > make[1]: 'GIT-VERSION-FILE' is up to date. > mkdir: cannot create directory ‘.build/lint-docs/gitlink/howto’: No such file or directory > make: *** [Makefile:476: .build/lint-docs/gitlink/howto] Error 1 > > The fix is easy: we just need to depend on the correct parent directory. > > Signed-off-by: Jeff King <peff@peff.net> > --- > The problem starts in ab/fix-make-lint-docs, which is in master. > > I wasn't able to trigger the problem locally even with running 'make > clean; make lint-docs' in a loop, but I did see it in the wild in a CI > documentation job: > > https://github.com/peff/git/runs/4005766641?check_suite_focus=true#step:4:60 > > It would have been a lot easier to diagnose from the CI output if the > mkdir lines weren't silent. I.e., if we had a $(QUIET_MKDIR) which > printed "MKDIR $@" rather than nothing at all. > > Documentation/Makefile | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/Makefile b/Documentation/Makefile > index 911b6bf79c..ed656db2ae 100644 > --- a/Documentation/Makefile > +++ b/Documentation/Makefile > @@ -472,9 +472,9 @@ print-man1: > ## Lint: gitlink > .build/lint-docs/gitlink: | .build/lint-docs > $(QUIET)mkdir $@ > -.build/lint-docs/gitlink/howto: | .build/lint-docs > +.build/lint-docs/gitlink/howto: | .build/lint-docs/gitlink > $(QUIET)mkdir $@ > -.build/lint-docs/gitlink/config: | .build/lint-docs > +.build/lint-docs/gitlink/config: | .build/lint-docs/gitlink > $(QUIET)mkdir $@ > LINT_DOCS_GITLINK = $(patsubst %.txt,.build/lint-docs/gitlink/%.ok,$(HOWTO_TXT) $(DOC_DEP_TXT)) > $(LINT_DOCS_GITLINK): | .build/lint-docs/gitlink Thanks a lot for fixing that bug, and sorry for not spotting it. This fix LGTM and is obviously correct. I'll try to do something about the $(QUIET*) as a follow-up, I was trying to find the right balance between over-verbosity & "tracing". ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Documentation/Makefile: fix lint-docs mkdir dependency 2021-10-26 10:05 ` Ævar Arnfjörð Bjarmason @ 2021-10-26 21:18 ` Jeff King 0 siblings, 0 replies; 9+ messages in thread From: Jeff King @ 2021-10-26 21:18 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano On Tue, Oct 26, 2021 at 12:05:40PM +0200, Ævar Arnfjörð Bjarmason wrote: > I'll try to do something about the $(QUIET*) as a follow-up, I was > trying to find the right balance between over-verbosity & "tracing". Yeah, I wondered if it would end up super-verbose. But I tried it out and IMHO it looks just fine to print mkdir lines. In the initial build, "make" realizes we only need to run each rule once, and in subsequent builds it doesn't run them at all. -Peff ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Documentation/Makefile: fix lint-docs mkdir dependency 2021-10-26 7:31 [PATCH] Documentation/Makefile: fix lint-docs mkdir dependency Jeff King 2021-10-26 10:05 ` Ævar Arnfjörð Bjarmason @ 2021-10-28 0:03 ` Junio C Hamano 2021-10-28 7:48 ` Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2021-10-28 0:03 UTC (permalink / raw) To: Jeff King; +Cc: git, Ævar Arnfjörð Bjarmason Jeff King <peff@peff.net> writes: > Since 8650c6298c (doc lint: make "lint-docs" non-.PHONY, 2021-10-15), we > put the output for gitlink linter into .build/lint-docs/gitlink. There > are order-only dependencies to create the sequence of subdirs like: > > .build/lint-docs: | .build > $(QUIET)mkdir $@ > .build/lint-docs/gitlink: | .build/lint-docs > $(QUIET)mkdir $@ > > where each level has to depend on the prior one (since the parent > directory must exist for us to create something inside it). But the > "howto" and "config" subdirectories of gitlink have the wrong > dependency; they depend on "lint-docs", not "lint-docs/gitlink". Thanks. I wonder if we can somehow avoid this unwieldy chain of commands, perhaps with using "mkdir -p" somewhere, or make the lint scripts create the necessary leading paths. From the looks of the tail end of Documentation/Makefile, the latter may be the cleaner solution. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Documentation/Makefile: fix lint-docs mkdir dependency 2021-10-28 0:03 ` Junio C Hamano @ 2021-10-28 7:48 ` Ævar Arnfjörð Bjarmason 2021-10-28 14:35 ` Jeff King 0 siblings, 1 reply; 9+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-28 7:48 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git On Wed, Oct 27 2021, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > >> Since 8650c6298c (doc lint: make "lint-docs" non-.PHONY, 2021-10-15), we >> put the output for gitlink linter into .build/lint-docs/gitlink. There >> are order-only dependencies to create the sequence of subdirs like: >> >> .build/lint-docs: | .build >> $(QUIET)mkdir $@ >> .build/lint-docs/gitlink: | .build/lint-docs >> $(QUIET)mkdir $@ >> >> where each level has to depend on the prior one (since the parent >> directory must exist for us to create something inside it). But the >> "howto" and "config" subdirectories of gitlink have the wrong >> dependency; they depend on "lint-docs", not "lint-docs/gitlink". > > Thanks. > > I wonder if we can somehow avoid this unwieldy chain of commands, > perhaps with using "mkdir -p" somewhere, or make the lint scripts > create the necessary leading paths. From the looks of the tail end > of Documentation/Makefile, the latter may be the cleaner solution. Simplest would be to simply do the "mkdir -p" unconditionally, which we do in some other places. The diff below on top of next would do that. I didn't do it because it slows things down quite a bit. Here HEAD is the diff below: $ hyperfine --warmup 5 -m 30 -L s ",~" -p 'git checkout HEAD{s} -- Makefile; rm -rf .build' 'make -j8 lint-docs R={s}' -c 'git checkout HEAD -- Makefile' Benchmark #1: make -j8 lint-docs R= Time (mean ± σ): 709.7 ms ± 25.7 ms [User: 3.584 s, System: 0.696 s] Range (min … max): 691.4 ms … 833.1 ms 30 runs Benchmark #2: make -j8 lint-docs R=~ Time (mean ± σ): 647.3 ms ± 5.5 ms [User: 3.081 s, System: 0.635 s] Range (min … max): 638.4 ms … 670.6 ms 30 runs Summary 'make -j8 lint-docs R=~' ran 1.10 ± 0.04 times faster than 'make -j8 lint-docs R=' You can do this with make macros via $(eval) calling a $(foreach) loop, i.e. just generate the boilerplate we have now. For this case I thought it wasn't worth it, but figured I could eventually loop back to it if/when we use a nested structure inside a ".build directory more widely. diff --git a/Documentation/Makefile b/Documentation/Makefile index ed656db2ae9..8a185e89e55 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -463,26 +463,11 @@ quick-install-html: require-htmlrepo print-man1: @for i in $(MAN1_TXT); do echo $$i; done -## Lint: Common -.build: - $(QUIET)mkdir $@ -.build/lint-docs: | .build - $(QUIET)mkdir $@ - -## Lint: gitlink -.build/lint-docs/gitlink: | .build/lint-docs - $(QUIET)mkdir $@ -.build/lint-docs/gitlink/howto: | .build/lint-docs/gitlink - $(QUIET)mkdir $@ -.build/lint-docs/gitlink/config: | .build/lint-docs/gitlink - $(QUIET)mkdir $@ LINT_DOCS_GITLINK = $(patsubst %.txt,.build/lint-docs/gitlink/%.ok,$(HOWTO_TXT) $(DOC_DEP_TXT)) -$(LINT_DOCS_GITLINK): | .build/lint-docs/gitlink -$(LINT_DOCS_GITLINK): | .build/lint-docs/gitlink/howto -$(LINT_DOCS_GITLINK): | .build/lint-docs/gitlink/config $(LINT_DOCS_GITLINK): lint-gitlink.perl $(LINT_DOCS_GITLINK): .build/lint-docs/gitlink/%.ok: %.txt - $(QUIET_LINT_GITLINK)$(PERL_PATH) lint-gitlink.perl \ + $(QUIET_LINT_GITLINK)mkdir -p $(@D) && \ + $(PERL_PATH) lint-gitlink.perl \ $< \ $(HOWTO_TXT) $(DOC_DEP_TXT) \ --section=1 $(MAN1_TXT) \ @@ -492,24 +477,20 @@ $(LINT_DOCS_GITLINK): .build/lint-docs/gitlink/%.ok: %.txt lint-docs-gitlink: $(LINT_DOCS_GITLINK) ## Lint: man-end-blurb -.build/lint-docs/man-end-blurb: | .build/lint-docs - $(QUIET)mkdir $@ LINT_DOCS_MAN_END_BLURB = $(patsubst %.txt,.build/lint-docs/man-end-blurb/%.ok,$(MAN_TXT)) -$(LINT_DOCS_MAN_END_BLURB): | .build/lint-docs/man-end-blurb $(LINT_DOCS_MAN_END_BLURB): lint-man-end-blurb.perl $(LINT_DOCS_MAN_END_BLURB): .build/lint-docs/man-end-blurb/%.ok: %.txt - $(QUIET_LINT_MANEND)$(PERL_PATH) lint-man-end-blurb.perl $< >$@ + $(QUIET_LINT_MANEND)mkdir -p $(@D) && \ + $(PERL_PATH) lint-man-end-blurb.perl $< >$@ .PHONY: lint-docs-man-end-blurb lint-docs-man-end-blurb: $(LINT_DOCS_MAN_END_BLURB) ## Lint: man-section-order -.build/lint-docs/man-section-order: | .build/lint-docs - $(QUIET)mkdir $@ LINT_DOCS_MAN_SECTION_ORDER = $(patsubst %.txt,.build/lint-docs/man-section-order/%.ok,$(MAN_TXT)) -$(LINT_DOCS_MAN_SECTION_ORDER): | .build/lint-docs/man-section-order $(LINT_DOCS_MAN_SECTION_ORDER): lint-man-section-order.perl $(LINT_DOCS_MAN_SECTION_ORDER): .build/lint-docs/man-section-order/%.ok: %.txt - $(QUIET_LINT_MANSEC)$(PERL_PATH) lint-man-section-order.perl $< >$@ + $(QUIET_LINT_MANSEC)mkdir -p $(@D) && \ + $(PERL_PATH) lint-man-section-order.perl $< >$@ .PHONY: lint-docs-man-section-order lint-docs-man-section-order: $(LINT_DOCS_MAN_SECTION_ORDER) ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Documentation/Makefile: fix lint-docs mkdir dependency 2021-10-28 7:48 ` Ævar Arnfjörð Bjarmason @ 2021-10-28 14:35 ` Jeff King 2021-10-28 16:45 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Jeff King @ 2021-10-28 14:35 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Junio C Hamano, git On Thu, Oct 28, 2021 at 09:48:51AM +0200, Ævar Arnfjörð Bjarmason wrote: > > I wonder if we can somehow avoid this unwieldy chain of commands, > > perhaps with using "mkdir -p" somewhere, or make the lint scripts > > create the necessary leading paths. From the looks of the tail end > > of Documentation/Makefile, the latter may be the cleaner solution. > > Simplest would be to simply do the "mkdir -p" unconditionally, which we > do in some other places. The diff below on top of next would do that. > > I didn't do it because it slows things down quite a bit. Here HEAD is > the diff below: There's an in-between, I'd think, where the many "foo/bar/baz/$@" targets have an order-dependency on "foo/bar/baz", and that single rule uses "mkdir -p" to create all of the directories. It doesn't buy us much simplification in this case, though, because various rules independently depend on .build/gitlink/lint-docs/howto, .built/gitlink/lint-docs, and .build/gitlink, etc. So we still end up with roughly the same number of rules, though the directory rules don't have to depend on one another. It also means that these "mkdir -p" may race with each other, though in general I'd hope that most "mkdir" implements could handle this. Something like this works, I think: diff --git a/Documentation/Makefile b/Documentation/Makefile index 911b6bf79c..a70e418af6 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -463,19 +463,13 @@ quick-install-html: require-htmlrepo print-man1: @for i in $(MAN1_TXT); do echo $$i; done -## Lint: Common -.build: - $(QUIET)mkdir $@ -.build/lint-docs: | .build - $(QUIET)mkdir $@ - ## Lint: gitlink -.build/lint-docs/gitlink: | .build/lint-docs - $(QUIET)mkdir $@ -.build/lint-docs/gitlink/howto: | .build/lint-docs - $(QUIET)mkdir $@ -.build/lint-docs/gitlink/config: | .build/lint-docs - $(QUIET)mkdir $@ +.build/lint-docs/gitlink: + $(QUIET)mkdir -p $@ +.build/lint-docs/gitlink/howto: + $(QUIET)mkdir -p $@ +.build/lint-docs/gitlink/config: + $(QUIET)mkdir -p $@ LINT_DOCS_GITLINK = $(patsubst %.txt,.build/lint-docs/gitlink/%.ok,$(HOWTO_TXT) $(DOC_DEP_TXT)) $(LINT_DOCS_GITLINK): | .build/lint-docs/gitlink $(LINT_DOCS_GITLINK): | .build/lint-docs/gitlink/howto @@ -492,8 +486,8 @@ $(LINT_DOCS_GITLINK): .build/lint-docs/gitlink/%.ok: %.txt lint-docs-gitlink: $(LINT_DOCS_GITLINK) ## Lint: man-end-blurb -.build/lint-docs/man-end-blurb: | .build/lint-docs - $(QUIET)mkdir $@ +.build/lint-docs/man-end-blurb: + $(QUIET)mkdir -p $@ LINT_DOCS_MAN_END_BLURB = $(patsubst %.txt,.build/lint-docs/man-end-blurb/%.ok,$(MAN_TXT)) $(LINT_DOCS_MAN_END_BLURB): | .build/lint-docs/man-end-blurb $(LINT_DOCS_MAN_END_BLURB): lint-man-end-blurb.perl @@ -503,8 +497,8 @@ $(LINT_DOCS_MAN_END_BLURB): .build/lint-docs/man-end-blurb/%.ok: %.txt lint-docs-man-end-blurb: $(LINT_DOCS_MAN_END_BLURB) ## Lint: man-section-order -.build/lint-docs/man-section-order: | .build/lint-docs - $(QUIET)mkdir $@ +.build/lint-docs/man-section-order: + $(QUIET)mkdir -p $@ LINT_DOCS_MAN_SECTION_ORDER = $(patsubst %.txt,.build/lint-docs/man-section-order/%.ok,$(MAN_TXT)) $(LINT_DOCS_MAN_SECTION_ORDER): | .build/lint-docs/man-section-order $(LINT_DOCS_MAN_SECTION_ORDER): lint-man-section-order.perl We could technically even drop the .build/lint-docs/gitlink rule, because it's a parent of other directories built by the same rule. But that's a bit too clever and magical for my tastes. All that said, I'm not that unhappy with the current state, and I think with my patch it should be correct / robust. > You can do this with make macros via $(eval) calling a $(foreach) loop, > i.e. just generate the boilerplate we have now. For this case I thought > it wasn't worth it, but figured I could eventually loop back to it > if/when we use a nested structure inside a ".build directory more > widely. Yeah, I think for the scope of the problem here (remembering that "mkdir foo/bar" needs to depend on "mkdir foo") that a macro would probably just confuse things. IMHO the more subtle maintenance trap is that LINT_DOCS_GITLINK needs to remember to depend on the "config/" and "howto/" directories, because that's where we keep source files. It would be easy to add a source file to DOC_DEP_TXT that mentions a new subdirectory, but not realize it needs an extra rule. If the macro magic went as far as actually mapping all of LINT_DOCS_GITLINK into their .build/ dirname counterparts, and then automatically generated the right rules, that would make it truly turnkey. It would probably also be a pretty gross macro, but maybe worth it if nobody ever needed to touch it. :) At any rate, I don't think there's any urgency on that. -Peff ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Documentation/Makefile: fix lint-docs mkdir dependency 2021-10-28 14:35 ` Jeff King @ 2021-10-28 16:45 ` Junio C Hamano 2021-10-28 17:06 ` Jeff King 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2021-10-28 16:45 UTC (permalink / raw) To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, git Jeff King <peff@peff.net> writes: > There's an in-between, I'd think, where the many "foo/bar/baz/$@" > targets have an order-dependency on "foo/bar/baz", and that single rule > uses "mkdir -p" to create all of the directories. > > It doesn't buy us much simplification in this case, though, because > various rules independently depend on .build/gitlink/lint-docs/howto, > .built/gitlink/lint-docs, and .build/gitlink, etc. So we still end up > with roughly the same number of rules, though the directory rules don't > have to depend on one another. > > It also means that these "mkdir -p" may race with each other, though in > general I'd hope that most "mkdir" implements could handle this. > > Something like this works, I think: Hmph, what I actually meant was to make sure that the recipe to create the files to have "mkdir -p $(basename $@)" in front, instead of having "we need to prepare the containing directory in order to have a file there" in the makefile. > ... > At any rate, I don't think there's any urgency on that. Sure. I think I've picked up the one at the start of this thread already, so we should be good. Thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Documentation/Makefile: fix lint-docs mkdir dependency 2021-10-28 16:45 ` Junio C Hamano @ 2021-10-28 17:06 ` Jeff King 2021-10-28 18:30 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 9+ messages in thread From: Jeff King @ 2021-10-28 17:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git On Thu, Oct 28, 2021 at 09:45:14AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > There's an in-between, I'd think, where the many "foo/bar/baz/$@" > > targets have an order-dependency on "foo/bar/baz", and that single rule > > uses "mkdir -p" to create all of the directories. > > > > It doesn't buy us much simplification in this case, though, because > > various rules independently depend on .build/gitlink/lint-docs/howto, > > .built/gitlink/lint-docs, and .build/gitlink, etc. So we still end up > > with roughly the same number of rules, though the directory rules don't > > have to depend on one another. > > > > It also means that these "mkdir -p" may race with each other, though in > > general I'd hope that most "mkdir" implements could handle this. > > > > Something like this works, I think: > > Hmph, what I actually meant was to make sure that the recipe to > create the files to have "mkdir -p $(basename $@)" in front, instead > of having "we need to prepare the containing directory in order to > have a file there" in the makefile. Yeah, I agree that's simpler, and is what Ævar showed. But it is slower, because we run a bunch of redundant "mkdir -p" calls, one per file. -Peff ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Documentation/Makefile: fix lint-docs mkdir dependency 2021-10-28 17:06 ` Jeff King @ 2021-10-28 18:30 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 9+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-28 18:30 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git On Thu, Oct 28 2021, Jeff King wrote: > On Thu, Oct 28, 2021 at 09:45:14AM -0700, Junio C Hamano wrote: > >> Jeff King <peff@peff.net> writes: >> >> > There's an in-between, I'd think, where the many "foo/bar/baz/$@" >> > targets have an order-dependency on "foo/bar/baz", and that single rule >> > uses "mkdir -p" to create all of the directories. >> > >> > It doesn't buy us much simplification in this case, though, because >> > various rules independently depend on .build/gitlink/lint-docs/howto, >> > .built/gitlink/lint-docs, and .build/gitlink, etc. So we still end up >> > with roughly the same number of rules, though the directory rules don't >> > have to depend on one another. >> > >> > It also means that these "mkdir -p" may race with each other, though in >> > general I'd hope that most "mkdir" implements could handle this. >> > >> > Something like this works, I think: >> >> Hmph, what I actually meant was to make sure that the recipe to >> create the files to have "mkdir -p $(basename $@)" in front, instead >> of having "we need to prepare the containing directory in order to >> have a file there" in the makefile. > > Yeah, I agree that's simpler, and is what Ævar showed. But it is slower, > because we run a bunch of redundant "mkdir -p" calls, one per file. Here's a method that's both less verbose in lines & also faster, but maybe too clever a use of GNU make features, on top of "next". It relies on $(wildcard) returning an empty list on a non-existing filename, and then on $(if) to either expand to "mkdir -p $(@D)", or nothing, and (perhaps in an ugly way?) piggy-backs on an existing $@ rule, so one rule has two $(QUIET_*) uses. With: HEAD~2 = next HEAD~1 = unconditional mkdir -p, upthread <211028.861r45y3pt.gmgdl@evledraar.gmail.com> HEAD = the below patch We get these results, i.e. it's also faster: $ hyperfine --warmup 5 -m 30 -L s ",~,~2" -p 'git checkout HEAD{s} -- Makefile; rm -rf .build' 'make -j8 lint-docs R={s}' -c 'git checkout HEAD -- Makefile' Benchmark #1: make -j8 lint-docs R= Time (mean ± σ): 628.5 ms ± 43.2 ms [User: 2.385 s, System: 0.445 s] Range (min … max): 605.6 ms … 787.7 ms 30 runs Benchmark #2: make -j8 lint-docs R=~ Time (mean ± σ): 770.2 ms ± 12.7 ms [User: 3.446 s, System: 0.666 s] Range (min … max): 756.3 ms … 831.4 ms 30 runs Benchmark #3: make -j8 lint-docs R=~2 Time (mean ± σ): 696.9 ms ± 3.5 ms [User: 2.967 s, System: 0.600 s] Range (min … max): 691.2 ms … 706.2 ms 30 runs Summary 'make -j8 lint-docs R=' ran 1.11 ± 0.08 times faster than 'make -j8 lint-docs R=~2' 1.23 ± 0.09 times faster than 'make -j8 lint-docs R=~' The one negative thing is that we'll return an inconsistent set of "mkdir" lines, since it's racy, but here we're using the race to our benefit. It doesn't matter for the end result if we e.g. created a more nested directory first, or needed two "mkdir -p" calls because we did a shallower one first. Do you think it's worth submitting that as a non-throwaway? diff --git a/Documentation/Makefile b/Documentation/Makefile index ed656db2ae9..99c2f9d02cf 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -234,6 +234,7 @@ ifndef V QUIET_DBLATEX = @echo ' ' DBLATEX $@; QUIET_XSLTPROC = @echo ' ' XSLTPROC $@; QUIET_GEN = @echo ' ' GEN $@; + QUIET_MKDIR_P = @echo ' ' MKDIR -p $(@D); QUIET_STDERR = 2> /dev/null QUIET_SUBDIR0 = +@subdir= QUIET_SUBDIR1 = ;$(NO_SUBDIR) echo ' ' SUBDIR $$subdir; \ @@ -463,25 +464,10 @@ quick-install-html: require-htmlrepo print-man1: @for i in $(MAN1_TXT); do echo $$i; done -## Lint: Common -.build: - $(QUIET)mkdir $@ -.build/lint-docs: | .build - $(QUIET)mkdir $@ - -## Lint: gitlink -.build/lint-docs/gitlink: | .build/lint-docs - $(QUIET)mkdir $@ -.build/lint-docs/gitlink/howto: | .build/lint-docs/gitlink - $(QUIET)mkdir $@ -.build/lint-docs/gitlink/config: | .build/lint-docs/gitlink - $(QUIET)mkdir $@ LINT_DOCS_GITLINK = $(patsubst %.txt,.build/lint-docs/gitlink/%.ok,$(HOWTO_TXT) $(DOC_DEP_TXT)) -$(LINT_DOCS_GITLINK): | .build/lint-docs/gitlink -$(LINT_DOCS_GITLINK): | .build/lint-docs/gitlink/howto -$(LINT_DOCS_GITLINK): | .build/lint-docs/gitlink/config $(LINT_DOCS_GITLINK): lint-gitlink.perl $(LINT_DOCS_GITLINK): .build/lint-docs/gitlink/%.ok: %.txt + $(if $(wildcard $(@D)),,$(QUIET_MKDIR_P)$(shell mkdir -p $(@D))) $(QUIET_LINT_GITLINK)$(PERL_PATH) lint-gitlink.perl \ $< \ $(HOWTO_TXT) $(DOC_DEP_TXT) \ @@ -492,23 +478,19 @@ $(LINT_DOCS_GITLINK): .build/lint-docs/gitlink/%.ok: %.txt lint-docs-gitlink: $(LINT_DOCS_GITLINK) ## Lint: man-end-blurb -.build/lint-docs/man-end-blurb: | .build/lint-docs - $(QUIET)mkdir $@ LINT_DOCS_MAN_END_BLURB = $(patsubst %.txt,.build/lint-docs/man-end-blurb/%.ok,$(MAN_TXT)) -$(LINT_DOCS_MAN_END_BLURB): | .build/lint-docs/man-end-blurb $(LINT_DOCS_MAN_END_BLURB): lint-man-end-blurb.perl $(LINT_DOCS_MAN_END_BLURB): .build/lint-docs/man-end-blurb/%.ok: %.txt - $(QUIET_LINT_MANEND)$(PERL_PATH) lint-man-end-blurb.perl $< >$@ + $(if $(wildcard $(@D)),,$(QUIET_MKDIR_P)$(shell mkdir -p $(@D))) + $(QUIET_LINT_MANEND)$($(PERL_PATH) lint-man-end-blurb.perl $< >$@ .PHONY: lint-docs-man-end-blurb lint-docs-man-end-blurb: $(LINT_DOCS_MAN_END_BLURB) ## Lint: man-section-order -.build/lint-docs/man-section-order: | .build/lint-docs - $(QUIET)mkdir $@ LINT_DOCS_MAN_SECTION_ORDER = $(patsubst %.txt,.build/lint-docs/man-section-order/%.ok,$(MAN_TXT)) -$(LINT_DOCS_MAN_SECTION_ORDER): | .build/lint-docs/man-section-order $(LINT_DOCS_MAN_SECTION_ORDER): lint-man-section-order.perl $(LINT_DOCS_MAN_SECTION_ORDER): .build/lint-docs/man-section-order/%.ok: %.txt + $(if $(wildcard $(@D)),,$(QUIET_MKDIR_P)$(shell mkdir -p $(@D))) $(QUIET_LINT_MANSEC)$(PERL_PATH) lint-man-section-order.perl $< >$@ .PHONY: lint-docs-man-section-order lint-docs-man-section-order: $(LINT_DOCS_MAN_SECTION_ORDER) ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-10-28 18:38 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-26 7:31 [PATCH] Documentation/Makefile: fix lint-docs mkdir dependency Jeff King 2021-10-26 10:05 ` Ævar Arnfjörð Bjarmason 2021-10-26 21:18 ` Jeff King 2021-10-28 0:03 ` Junio C Hamano 2021-10-28 7:48 ` Ævar Arnfjörð Bjarmason 2021-10-28 14:35 ` Jeff King 2021-10-28 16:45 ` Junio C Hamano 2021-10-28 17:06 ` Jeff King 2021-10-28 18:30 ` Ævar Arnfjörð Bjarmason
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).