Re: [PATCH] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o

From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Oct 10, 2024, 12:04 UTC
Message-ID: <C05B01E0-5007-4FB9-94CD-CBE74E79F9B7@gmail.com>
In-Reply-To: <ZwSt0ZrlDaYCzfRX@pks.im>

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.

Show 44 quoted lines

> Le 7 oct. 2024 à 23:58, Patrick Steinhardt <ps@pks.im> a écrit :
> 
> On Mon, Oct 07, 2024 at 05:53:41PM -0700, Junio C Hamano wrote:
>> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> 
>>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>> 
>>> 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 <levraiphilippeblain@gmail.com>
>>> ---
>>>    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