{"thread":{"id":"62272","subject":"[PATCH] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o","startedAt":"2024-10-07T17:43:57Z","lastAt":"2024-10-15T00:34:06Z","messageCount":8,"participants":["Philippe Blain via GitGitGadget","Junio C Hamano","Patrick Steinhardt","Philippe Blain","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"504348","messageId":"pull.1810.git.1728323033680.gitgitgadget@gmail.com","threadId":"62272","inReplyTo":null,"subject":"[PATCH] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-07T17:43:53Z","receivedAt":"2024-10-07T17:43:57Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nThe clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the\ngenerated 'clar.suite', but this dependency is not taken into account by\nour Makefile, so that it is possible for a parallel build to fail if\nMake tries to build 'clar.o' before 'clar.suite' is generated.\n\nCorrectly specify the dependency.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n    Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o\n    \n    Hi Patrick,\n    \n    I tried building v2.47.0 and stumbled onto this small issue. It\n    reproduces for me from a fresh clone on my old 2009 Mac with make -j -l\n    2.5, it's a little curious that no one ran into this yet.\n    \n    I found it best to declare the dependency directly on clar.o, instead of\n    adjusting the dependencies of CLAR_TEST_OBJS on the line above, since it\n    is really only this clar.c that includes clar.suite\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1810%2Fphil-blain%2Fclar-build-dep-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1810/phil-blain/clar-build-dep-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1810\n\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex 2dde1fd2b8b..b615de74811 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3911,6 +3911,7 @@ $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUI\n $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h\n \t$(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite\n $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h\n+$(UNIT_TEST_DIR)/clar/clar.o: $(UNIT_TEST_DIR)/clar.suite\n $(CLAR_TEST_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR)\n $(CLAR_TEST_PROG): $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS) $(GITLIBS) GIT-LDFLAGS\n \t$(call mkdir_p_parent_template)\n\nbase-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n-- \ngitgitgadget\n"},{"id":"504370","messageId":"xmqqldyz76ui.fsf@gitster.g","threadId":"62272","inReplyTo":"pull.1810.git.1728323033680.gitgitgadget@gmail.com","subject":"Re: [PATCH] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-08T00:53:41Z","receivedAt":"2024-10-08T00:53:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>\n> The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the\n> generated 'clar.suite', but this dependency is not taken into account by\n> our Makefile, so that it is possible for a parallel build to fail if\n> Make tries to build 'clar.o' before 'clar.suite' is generated.\n>\n> Correctly specify the dependency.\n>\n> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> ---\n>     Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o\n>     \n>     Hi Patrick,\n>     \n>     I tried building v2.47.0 and stumbled onto this small issue. It\n>     reproduces for me from a fresh clone on my old 2009 Mac with make -j -l\n>     2.5, it's a little curious that no one ran into this yet.\n\nI suspect that nobody tells make to build clar.o (and nothing else).\n\nInstead, the t/unit-tests/bin/unit-tests target is what is typically\nbuilt, which is part of $(CLAR_TEST_PROG) that has clar.suite as one\nof its dependencies.\n\n    $ make\n    $ rm -f t/unit-tests/clar.suite t/unit-tests/clar/clar.o\n    $ make -j1 t/unit-tests/bin/unit-tests\n    GEN t/unit-tests/clar.suite\n    CC t/unit-tests/clar/clar.o\n    LINK t/unit-tests/bin/unit-tests\n\nWhat is possible to happen from the broken dependencies is when I\ndid not remove clar.o in the above experiment.  We may rebuild\nclar.suite and then link clar.o that is outdated without realizing.\n\n>     I found it best to declare the dependency directly on clar.o, instead of\n>     adjusting the dependencies of CLAR_TEST_OBJS on the line above, since it\n>     is really only this clar.c that includes clar.suite\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1810%2Fphil-blain%2Fclar-build-dep-fix-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1810/phil-blain/clar-build-dep-fix-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1810\n>\n>  Makefile | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/Makefile b/Makefile\n> index 2dde1fd2b8b..b615de74811 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3911,6 +3911,7 @@ $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUI\n>  $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h\n>  \t$(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite\n>  $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h\n> +$(UNIT_TEST_DIR)/clar/clar.o: $(UNIT_TEST_DIR)/clar.suite\n>  $(CLAR_TEST_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR)\n>  $(CLAR_TEST_PROG): $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS) $(GITLIBS) GIT-LDFLAGS\n>  \t$(call mkdir_p_parent_template)\n>\n> base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n"},{"id":"504386","messageId":"ZwSt0ZrlDaYCzfRX@pks.im","threadId":"62272","inReplyTo":"xmqqldyz76ui.fsf@gitster.g","subject":"Re: [PATCH] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-08T03:58:09Z","receivedAt":"2024-10-08T03:58:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 07, 2024 at 05:53:41PM -0700, Junio C Hamano wrote:\n> \"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n> > From: Philippe Blain <levraiphilippeblain@gmail.com>\n> >\n> > The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the\n> > generated 'clar.suite', but this dependency is not taken into account by\n> > our Makefile, so that it is possible for a parallel build to fail if\n> > Make tries to build 'clar.o' before 'clar.suite' is generated.\n> >\n> > Correctly specify the dependency.\n> >\n> > Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> > ---\n> >     Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o\n> >     \n> >     Hi Patrick,\n> >     \n> >     I tried building v2.47.0 and stumbled onto this small issue. It\n> >     reproduces for me from a fresh clone on my old 2009 Mac with make -j -l\n> >     2.5, it's a little curious that no one ran into this yet.\n> \n> I suspect that nobody tells make to build clar.o (and nothing else).\n> \n> Instead, the t/unit-tests/bin/unit-tests target is what is typically\n> built, which is part of $(CLAR_TEST_PROG) that has clar.suite as one\n> of its dependencies.\n> \n>     $ make\n>     $ rm -f t/unit-tests/clar.suite t/unit-tests/clar/clar.o\n>     $ make -j1 t/unit-tests/bin/unit-tests\n>     GEN t/unit-tests/clar.suite\n>     CC t/unit-tests/clar/clar.o\n>     LINK t/unit-tests/bin/unit-tests\n> \n> What is possible to happen from the broken dependencies is when I\n> did not remove clar.o in the above experiment.  We may rebuild\n> clar.suite and then link clar.o that is outdated without realizing.\n\nMakes sense. In any case, the fix looks good to me, thanks!\n\nPatrick\n"},{"id":"504688","messageId":"C05B01E0-5007-4FB9-94CD-CBE74E79F9B7@gmail.com","threadId":"62272","inReplyTo":"ZwSt0ZrlDaYCzfRX@pks.im","subject":"Re: [PATCH] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2024-10-10T12:04:10Z","receivedAt":"2024-10-10T12:04:17Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi,\n\nI 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. \n\nI’ll try to send a v2 tomorrow so maybe hold on to merging it to next for the moment, Junio. \n\nThanks\nPhilippe. \n\n> Le 7 oct. 2024 à 23:58, Patrick Steinhardt <ps@pks.im> a écrit :\n> \n> On Mon, Oct 07, 2024 at 05:53:41PM -0700, Junio C Hamano wrote:\n>> \"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>> \n>>> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>>> \n>>> The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the\n>>> generated 'clar.suite', but this dependency is not taken into account by\n>>> our Makefile, so that it is possible for a parallel build to fail if\n>>> Make tries to build 'clar.o' before 'clar.suite' is generated.\n>>> \n>>> Correctly specify the dependency.\n>>> \n>>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n>>> ---\n>>>    Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o\n>>> \n>>>    Hi Patrick,\n>>> \n>>>    I tried building v2.47.0 and stumbled onto this small issue. It\n>>>    reproduces for me from a fresh clone on my old 2009 Mac with make -j -l\n>>>    2.5, it's a little curious that no one ran into this yet.\n>> \n>> I suspect that nobody tells make to build clar.o (and nothing else).\n>> \n>> Instead, the t/unit-tests/bin/unit-tests target is what is typically\n>> built, which is part of $(CLAR_TEST_PROG) that has clar.suite as one\n>> of its dependencies.\n>> \n>>    $ make\n>>    $ rm -f t/unit-tests/clar.suite t/unit-tests/clar/clar.o\n>>    $ make -j1 t/unit-tests/bin/unit-tests\n>>    GEN t/unit-tests/clar.suite\n>>    CC t/unit-tests/clar/clar.o\n>>    LINK t/unit-tests/bin/unit-tests\n>> \n>> What is possible to happen from the broken dependencies is when I\n>> did not remove clar.o in the above experiment.  We may rebuild\n>> clar.suite and then link clar.o that is outdated without realizing.\n> \n> Makes sense. In any case, the fix looks good to me, thanks!\n> \n> Patrick\n"},{"id":"504732","messageId":"xmqq34l3lrlx.fsf@gitster.g","threadId":"62272","inReplyTo":"C05B01E0-5007-4FB9-94CD-CBE74E79F9B7@gmail.com","subject":"Re: [PATCH] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-10T18:50:34Z","receivedAt":"2024-10-10T18:50:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n> I had a closer look at how the header dependencies are handled in\n> the Makefile and I think a cleaner and more idiomatic way to fix\n> the problem would be to add clar.suite to GENERATED_H.\n\nIf it is a header file (e.g. we could run \"make hdr-check\" on it to\nmake sure it is self-contained, if we wanted to check it), that\nsounds like a very appropriate thing to do.\n\n> I’ll try to send a v2 tomorrow so maybe hold on to merging it to\n> next for the moment, Junio.\n\nThanks.\n"},{"id":"504853","messageId":"pull.1810.v2.git.1728667787227.gitgitgadget@gmail.com","threadId":"62272","inReplyTo":"pull.1810.git.1728323033680.gitgitgadget@gmail.com","subject":"[PATCH v2] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-11T17:29:47Z","receivedAt":"2024-10-11T17:29:50Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nThe clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the\ngenerated 'clar.suite', but this dependency is not taken into account by\nour Makefile, so that it is possible for a parallel build to fail if\nMake tries to build 'clar.o' before 'clar.suite' is generated.\n\nCorrectly specify the dependency.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n    Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o\n    \n    v2: Compared to v1, I just moved the new line one line up to keep the\n    dependencies of $(CLAR_TEST_OBJS) together.\n    \n    Regarding what I wrote in [1], I took a closer look and actually\n    clar.suite is already added to GENERATED_H. The issue really is that for\n    generated headers, the dependency must be specified manually. This is\n    the case for other generated headers (command-list.h, etc). So my\n    initial approach is correct.\n    \n    [1]\n    https://lore.kernel.org/git/C05B01E0-5007-4FB9-94CD-CBE74E79F9B7@gmail.com/\n    \n    v1: Hi Patrick,\n    \n    I tried building v2.47.0 and stumbled onto this small issue. It\n    reproduces for me from a fresh clone on my old 2009 Mac with make -j -l\n    2.5, it's a little curious that no one ran into this yet.\n    \n    I found it best to declare the dependency directly on clar.o, instead of\n    adjusting the dependencies of CLAR_TEST_OBJS on the line above, since it\n    is really only this clar.c that includes clar.suite\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1810%2Fphil-blain%2Fclar-build-dep-fix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1810/phil-blain/clar-build-dep-fix-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1810\n\nRange-diff vs v1:\n\n 1:  42ae65c5f82 ! 1:  f20c8a0f57b Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o\n     @@ Commit message\n      \n       ## Makefile ##\n      @@ Makefile: $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUI\n     + \tdone >$@\n       $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h\n       \t$(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite\n     - $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h\n      +$(UNIT_TEST_DIR)/clar/clar.o: $(UNIT_TEST_DIR)/clar.suite\n     + $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h\n       $(CLAR_TEST_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR)\n       $(CLAR_TEST_PROG): $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS) $(GITLIBS) GIT-LDFLAGS\n     - \t$(call mkdir_p_parent_template)\n\n\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex 2dde1fd2b8b..76926537ccd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3910,6 +3910,7 @@ $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUI\n \tdone >$@\n $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h\n \t$(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite\n+$(UNIT_TEST_DIR)/clar/clar.o: $(UNIT_TEST_DIR)/clar.suite\n $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h\n $(CLAR_TEST_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR)\n $(CLAR_TEST_PROG): $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS) $(GITLIBS) GIT-LDFLAGS\n\nbase-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n-- \ngitgitgadget\n"},{"id":"504985","messageId":"Zw0HmISykehEiC-p@pks.im","threadId":"62272","inReplyTo":"pull.1810.v2.git.1728667787227.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-14T11:59:25Z","receivedAt":"2024-10-14T11:59:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 11, 2024 at 05:29:47PM +0000, Philippe Blain via GitGitGadget wrote:\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n> \n> The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the\n> generated 'clar.suite', but this dependency is not taken into account by\n> our Makefile, so that it is possible for a parallel build to fail if\n> Make tries to build 'clar.o' before 'clar.suite' is generated.\n> \n> Correctly specify the dependency.\n> \n> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n\nThanks, this version looks good to me.\n\nPatrick\n"},{"id":"505103","messageId":"Zw24fMNMTK14EqfL@nand.local","threadId":"62272","inReplyTo":"Zw0HmISykehEiC-p@pks.im","subject":"Re: [PATCH v2] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-15T00:34:04Z","receivedAt":"2024-10-15T00:34:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 14, 2024 at 01:59:25PM +0200, Patrick Steinhardt wrote:\n> On Fri, Oct 11, 2024 at 05:29:47PM +0000, Philippe Blain via GitGitGadget wrote:\n> > From: Philippe Blain <levraiphilippeblain@gmail.com>\n> >\n> > The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the\n> > generated 'clar.suite', but this dependency is not taken into account by\n> > our Makefile, so that it is possible for a parallel build to fail if\n> > Make tries to build 'clar.o' before 'clar.suite' is generated.\n> >\n> > Correctly specify the dependency.\n> >\n> > Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n>\n> Thanks, this version looks good to me.\n\nThanks, both. Junio had already marked this one to merge to 'next', so I\nwill go ahead and merge it down in the next integration cycle (which\nshould be tomorrow, assuming I remember how to do it correctly).\n\nThanks,\nTaylor\n"}]}