git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [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).