# [PATCH] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o 8 messages from 2024-10-07 to 2024-10-15. Participants: Philippe Blain via GitGitGadget, Junio C Hamano, Patrick Steinhardt, Philippe Blain, Taylor Blau. Thread: https://gitlist.dev/t/62272 ## Philippe Blain via GitGitGadget, 2024-10-07 17:43 Subject: [PATCH] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o Message-ID: URL: https://gitlist.dev/e/pull.1810.git.1728323033680.gitgitgadget%40gmail.com ``` From: Philippe Blain The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the generated 'clar.suite', but this dependency is not taken into account by our Makefile, so that it is possible for a parallel build to fail if Make tries to build 'clar.o' before 'clar.suite' is generated. Correctly specify the dependency. Signed-off-by: Philippe Blain --- Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o Hi Patrick, I tried building v2.47.0 and stumbled onto this small issue. It reproduces for me from a fresh clone on my old 2009 Mac with make -j -l 2.5, it's a little curious that no one ran into this yet. I found it best to declare the dependency directly on clar.o, instead of adjusting the dependencies of CLAR_TEST_OBJS on the line above, since it is really only this clar.c that includes clar.suite Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1810%2Fphil-blain%2Fclar-build-dep-fix-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1810/phil-blain/clar-build-dep-fix-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1810 Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index 2dde1fd2b8b..b615de74811 100644 --- a/Makefile +++ b/Makefile @@ -3911,6 +3911,7 @@ $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUI $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h $(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h +$(UNIT_TEST_DIR)/clar/clar.o: $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR) $(CLAR_TEST_PROG): $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS) $(GITLIBS) GIT-LDFLAGS $(call mkdir_p_parent_template) base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2 -- gitgitgadget ``` ## Junio C Hamano, 2024-10-08 00:53 Subject: Re: [PATCH] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o Message-ID: URL: https://gitlist.dev/e/xmqqldyz76ui.fsf%40gitster.g In-Reply-To: ``` "Philippe Blain via GitGitGadget" writes: > From: Philippe Blain > > The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the > generated 'clar.suite', but this dependency is not taken into account by > our Makefile, so that it is possible for a parallel build to fail if > Make tries to build 'clar.o' before 'clar.suite' is generated. > > Correctly specify the dependency. > > Signed-off-by: Philippe Blain > --- > Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o > > Hi Patrick, > > I tried building v2.47.0 and stumbled onto this small issue. It > reproduces for me from a fresh clone on my old 2009 Mac with make -j -l > 2.5, it's a little curious that no one ran into this yet. I suspect that nobody tells make to build clar.o (and nothing else). Instead, the t/unit-tests/bin/unit-tests target is what is typically built, which is part of $(CLAR_TEST_PROG) that has clar.suite as one of its dependencies. $ make $ rm -f t/unit-tests/clar.suite t/unit-tests/clar/clar.o $ make -j1 t/unit-tests/bin/unit-tests GEN t/unit-tests/clar.suite CC t/unit-tests/clar/clar.o LINK t/unit-tests/bin/unit-tests What is possible to happen from the broken dependencies is when I did not remove clar.o in the above experiment. We may rebuild clar.suite and then link clar.o that is outdated without realizing. > I found it best to declare the dependency directly on clar.o, instead of > adjusting the dependencies of CLAR_TEST_OBJS on the line above, since it > is really only this clar.c that includes clar.suite > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1810%2Fphil-blain%2Fclar-build-dep-fix-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1810/phil-blain/clar-build-dep-fix-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1810 > > Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Makefile b/Makefile > index 2dde1fd2b8b..b615de74811 100644 > --- a/Makefile > +++ b/Makefile > @@ -3911,6 +3911,7 @@ $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUI > $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h > $(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite > $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h > +$(UNIT_TEST_DIR)/clar/clar.o: $(UNIT_TEST_DIR)/clar.suite > $(CLAR_TEST_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR) > $(CLAR_TEST_PROG): $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS) $(GITLIBS) GIT-LDFLAGS > $(call mkdir_p_parent_template) > > base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2 ``` ## Patrick Steinhardt, 2024-10-08 03:58 Subject: Re: [PATCH] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o Message-ID: URL: https://gitlist.dev/e/ZwSt0ZrlDaYCzfRX%40pks.im In-Reply-To: ``` On Mon, Oct 07, 2024 at 05:53:41PM -0700, Junio C Hamano wrote: > "Philippe Blain via GitGitGadget" writes: > > > From: Philippe Blain > > > > The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the > > generated 'clar.suite', but this dependency is not taken into account by > > our Makefile, so that it is possible for a parallel build to fail if > > Make tries to build 'clar.o' before 'clar.suite' is generated. > > > > Correctly specify the dependency. > > > > Signed-off-by: Philippe Blain > > --- > > Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o > > > > Hi Patrick, > > > > I tried building v2.47.0 and stumbled onto this small issue. It > > reproduces for me from a fresh clone on my old 2009 Mac with make -j -l > > 2.5, it's a little curious that no one ran into this yet. > > I suspect that nobody tells make to build clar.o (and nothing else). > > Instead, the t/unit-tests/bin/unit-tests target is what is typically > built, which is part of $(CLAR_TEST_PROG) that has clar.suite as one > of its dependencies. > > $ make > $ rm -f t/unit-tests/clar.suite t/unit-tests/clar/clar.o > $ make -j1 t/unit-tests/bin/unit-tests > GEN t/unit-tests/clar.suite > CC t/unit-tests/clar/clar.o > LINK t/unit-tests/bin/unit-tests > > What is possible to happen from the broken dependencies is when I > did not remove clar.o in the above experiment. We may rebuild > clar.suite and then link clar.o that is outdated without realizing. Makes sense. In any case, the fix looks good to me, thanks! Patrick ``` ## Philippe Blain, 2024-10-10 12:04 Subject: Re: [PATCH] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o Message-ID: URL: https://gitlist.dev/e/C05B01E0-5007-4FB9-94CD-CBE74E79F9B7%40gmail.com In-Reply-To: ``` Hi, I had a closer look at how the header dependencies are handled in the Makefile and I think a cleaner and more idiomatic way to fix the problem would be to add clar.suite to GENERATED_H. I’ll try to send a v2 tomorrow so maybe hold on to merging it to next for the moment, Junio. Thanks Philippe. > Le 7 oct. 2024 à 23:58, Patrick Steinhardt a écrit : > > On Mon, Oct 07, 2024 at 05:53:41PM -0700, Junio C Hamano wrote: >> "Philippe Blain via GitGitGadget" writes: >> >>> From: Philippe Blain >>> >>> The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the >>> generated 'clar.suite', but this dependency is not taken into account by >>> our Makefile, so that it is possible for a parallel build to fail if >>> Make tries to build 'clar.o' before 'clar.suite' is generated. >>> >>> Correctly specify the dependency. >>> >>> Signed-off-by: Philippe Blain >>> --- >>> Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o >>> >>> Hi Patrick, >>> >>> I tried building v2.47.0 and stumbled onto this small issue. It >>> reproduces for me from a fresh clone on my old 2009 Mac with make -j -l >>> 2.5, it's a little curious that no one ran into this yet. >> >> I suspect that nobody tells make to build clar.o (and nothing else). >> >> Instead, the t/unit-tests/bin/unit-tests target is what is typically >> built, which is part of $(CLAR_TEST_PROG) that has clar.suite as one >> of its dependencies. >> >> $ make >> $ rm -f t/unit-tests/clar.suite t/unit-tests/clar/clar.o >> $ make -j1 t/unit-tests/bin/unit-tests >> GEN t/unit-tests/clar.suite >> CC t/unit-tests/clar/clar.o >> LINK t/unit-tests/bin/unit-tests >> >> What is possible to happen from the broken dependencies is when I >> did not remove clar.o in the above experiment. We may rebuild >> clar.suite and then link clar.o that is outdated without realizing. > > Makes sense. In any case, the fix looks good to me, thanks! > > Patrick ``` ## Junio C Hamano, 2024-10-10 18:50 Subject: Re: [PATCH] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o Message-ID: URL: https://gitlist.dev/e/xmqq34l3lrlx.fsf%40gitster.g In-Reply-To: ``` Philippe Blain writes: > I had a closer look at how the header dependencies are handled in > the Makefile and I think a cleaner and more idiomatic way to fix > the problem would be to add clar.suite to GENERATED_H. If it is a header file (e.g. we could run "make hdr-check" on it to make sure it is self-contained, if we wanted to check it), that sounds like a very appropriate thing to do. > I’ll try to send a v2 tomorrow so maybe hold on to merging it to > next for the moment, Junio. Thanks. ``` ## Philippe Blain via GitGitGadget, 2024-10-11 17:29 Subject: [PATCH v2] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o Message-ID: URL: https://gitlist.dev/e/pull.1810.v2.git.1728667787227.gitgitgadget%40gmail.com In-Reply-To: ``` From: Philippe Blain The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the generated 'clar.suite', but this dependency is not taken into account by our Makefile, so that it is possible for a parallel build to fail if Make tries to build 'clar.o' before 'clar.suite' is generated. Correctly specify the dependency. Signed-off-by: Philippe Blain --- Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o v2: Compared to v1, I just moved the new line one line up to keep the dependencies of $(CLAR_TEST_OBJS) together. Regarding what I wrote in [1], I took a closer look and actually clar.suite is already added to GENERATED_H. The issue really is that for generated headers, the dependency must be specified manually. This is the case for other generated headers (command-list.h, etc). So my initial approach is correct. [1] https://lore.kernel.org/git/C05B01E0-5007-4FB9-94CD-CBE74E79F9B7@gmail.com/ v1: Hi Patrick, I tried building v2.47.0 and stumbled onto this small issue. It reproduces for me from a fresh clone on my old 2009 Mac with make -j -l 2.5, it's a little curious that no one ran into this yet. I found it best to declare the dependency directly on clar.o, instead of adjusting the dependencies of CLAR_TEST_OBJS on the line above, since it is really only this clar.c that includes clar.suite Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1810%2Fphil-blain%2Fclar-build-dep-fix-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1810/phil-blain/clar-build-dep-fix-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1810 Range-diff vs v1: 1: 42ae65c5f82 ! 1: f20c8a0f57b Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o @@ Commit message ## Makefile ## @@ Makefile: $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUI + done >$@ $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h $(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite - $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h +$(UNIT_TEST_DIR)/clar/clar.o: $(UNIT_TEST_DIR)/clar.suite + $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h $(CLAR_TEST_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR) $(CLAR_TEST_PROG): $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS) $(GITLIBS) GIT-LDFLAGS - $(call mkdir_p_parent_template) Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index 2dde1fd2b8b..76926537ccd 100644 --- a/Makefile +++ b/Makefile @@ -3910,6 +3910,7 @@ $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUI done >$@ $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h $(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite +$(UNIT_TEST_DIR)/clar/clar.o: $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h $(CLAR_TEST_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR) $(CLAR_TEST_PROG): $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS) $(GITLIBS) GIT-LDFLAGS base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2 -- gitgitgadget ``` ## Patrick Steinhardt, 2024-10-14 11:59 Subject: Re: [PATCH v2] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o Message-ID: URL: https://gitlist.dev/e/Zw0HmISykehEiC-p%40pks.im In-Reply-To: ``` On Fri, Oct 11, 2024 at 05:29:47PM +0000, Philippe Blain via GitGitGadget wrote: > From: Philippe Blain > > The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the > generated 'clar.suite', but this dependency is not taken into account by > our Makefile, so that it is possible for a parallel build to fail if > Make tries to build 'clar.o' before 'clar.suite' is generated. > > Correctly specify the dependency. > > Signed-off-by: Philippe Blain Thanks, this version looks good to me. Patrick ``` ## Taylor Blau, 2024-10-15 00:34 Subject: Re: [PATCH v2] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o Message-ID: URL: https://gitlist.dev/e/Zw24fMNMTK14EqfL%40nand.local In-Reply-To: ``` On Mon, Oct 14, 2024 at 01:59:25PM +0200, Patrick Steinhardt wrote: > On Fri, Oct 11, 2024 at 05:29:47PM +0000, Philippe Blain via GitGitGadget wrote: > > From: Philippe Blain > > > > The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the > > generated 'clar.suite', but this dependency is not taken into account by > > our Makefile, so that it is possible for a parallel build to fail if > > Make tries to build 'clar.o' before 'clar.suite' is generated. > > > > Correctly specify the dependency. > > > > Signed-off-by: Philippe Blain > > Thanks, this version looks good to me. Thanks, both. Junio had already marked this one to merge to 'next', so I will go ahead and merge it down in the next integration cycle (which should be tomorrow, assuming I remember how to do it correctly). Thanks, Taylor ```