Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o

PATCH Superseded Philippe Blain via GitGitGadget 5 participants 8 messages Oct 7, 2024 – Oct 15, 2024 Kbuild

lore

Share & tools

lore

Philippe Blain via GitGitGadget

author

2024-10-07 17:43 UTC #0 lore raw

PATCH 1/1 Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o

From: Philippe Blain <[email protected]>

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 <[email protected]>

---
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(+)

Makefile +1 -0

index 2dde1fd2b8b..b615de74811 100644
@@ -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

Signature · 1 line

gitgitgadget

Junio C Hamano

2024-10-08 00:53 UTC #1 lore raw

Quoted 10 lines · Philippe Blain via GitGitGadget

From: Philippe Blain <[email protected]>
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 <[email protected]>

Quoted 8 lines

---
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.

Quoted 25 lines

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(+)
Makefile +1 -0
index 2dde1fd2b8b..b615de74811 100644
@@ -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 UTC #2 lore raw

Quoted 34 lines · Junio C Hamano

Quoted 10 lines · Philippe Blain via GitGitGadget
From: Philippe Blain <[email protected]>
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 <[email protected]>
Quoted 8 lines
---
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

Quoted 3 lines

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 UTC #3 lore raw

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.

Quoted 44 lines · Patrick Steinhardt

Quoted 34 lines · Junio C Hamano
Quoted 10 lines · Philippe Blain via GitGitGadget
From: Philippe Blain <[email protected]>
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 <[email protected]>
Quoted 8 lines
---
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
Quoted 3 lines
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 UTC #4 lore raw

Quoted 3 lines · Philippe Blain

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.

Quoted 2 lines

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

author

2024-10-11 17:29 UTC #5 lore raw

PATCH v2 1/1 Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o

In reply to Philippe Blain via GitGitGadget

From: Philippe Blain <[email protected]>

Correctly specify the dependency.

Signed-off-by Philippe Blain <[email protected]>

---
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/[email protected]/

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.

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(+)

Makefile +1 -0

index 2dde1fd2b8b..76926537ccd 100644
@@ -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

Signature · 1 line

gitgitgadget

Patrick Steinhardt

2024-10-14 11:59 UTC #6 lore raw

Quoted 10 lines · Philippe Blain via GitGitGadget

From: Philippe Blain <[email protected]>
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 <[email protected]>

Thanks, this version looks good to me.

Patrick

Taylor Blau

2024-10-15 00:34 UTC #7 lore raw

Quoted 8 lines · Patrick Steinhardt

Quoted 7 lines · Philippe Blain via GitGitGadget
From: Philippe Blain <[email protected]>
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.

Quoted 5 lines

Quoted 3 lines
Correctly specify the dependency.
Signed-off-by Philippe Blain <[email protected]>
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