Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o #1810
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Philippe Blain via GitGitGadget" <[email protected]> writes:
> 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 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 |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Mon, Oct 07, 2024 at 05:53:41PM -0700, Junio C Hamano wrote:
> "Philippe Blain via GitGitGadget" <[email protected]> writes:
>
> > 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 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 |
|
This patch series was integrated into seen via git@0ba0f9d. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@9decb09. |
|
This patch series was integrated into seen via git@fac0be3. |
|
On the Git mailing list, Philippe Blain wrote (reply to this): 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 <[email protected]> a écrit :
>
> On Mon, Oct 07, 2024 at 05:53:41PM -0700, Junio C Hamano wrote:
>> "Philippe Blain via GitGitGadget" <[email protected]> writes:
>>
>>> 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 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 |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Philippe Blain <[email protected]> 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. |
|
This patch series was integrated into seen via git@4a9f863. |
|
There was a status update in the "Cooking" section about the branch Build fix. Expecting a reroll. cf. <[email protected]> source: <[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]>
phil-blain
force-pushed
the
clar-build-dep-fix
branch
from
42ae65c
to
f20c8a0
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@712f36a. |
|
This patch series was integrated into seen via git@6f8ee0e. |
|
There was a status update in the "Cooking" section about the branch Build fix. Will merge to 'next'. source: <[email protected]> |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Fri, Oct 11, 2024 at 05:29:47PM +0000, Philippe Blain via GitGitGadget wrote:
> 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 |
|
This patch series was integrated into seen via git@e4d0535. |
|
This patch series was integrated into seen via git@bba4f9d. |
|
On the Git mailing list, Taylor Blau wrote (reply to this): 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 <[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.
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 |
|
User |
|
This patch series was integrated into seen via git@9029bc8. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.I found it best to declare the dependency directly on
clar.o, instead of adjusting the dependencies ofCLAR_TEST_OBJSon the line above, since it is really only thisclar.cthat includesclar.suiteCC: Patrick Steinhardt [email protected]
cc: Philippe Blain [email protected]
cc: Taylor Blau [email protected]