Re: [Bug] Compat objects not added to CLAR_TEST_PROG

From: Osipov, Michael (IN IT IN) <michael.osipov@innomotics.com>
Date: Sep 5, 2025, 22:12 UTC
Message-ID: <41515d85-7afd-4c4a-b0b3-4cd902ca5677@innomotics.com>
In-Reply-To: <20250905213708.GB612697@coredump.intra.peff.net>

Let's have a look:

On 2025-09-05 23:37, Jeff King wrote:

Show 32 quoted lines

> [+cc pks for clar portability]
> 
> On Fri, Sep 05, 2025 at 03:19:50PM +0200, Osipov, Michael (IN IT IN) wrote:
> 
>> I am building Git 2.51.0 on HP-UX 11.31, previous releases went smoothly.
> 
> Neat, today I learned HP-UX is still alive and kicking. :)
> 
> Half of your patch makes sense to me, but I'm puzzled by the other half.
> This:
> 
>> diff -u -ur t/unit-tests/clar/clar/sandbox.h git-2.51.0.patched/t/unit-tests/clar/clar/sandbox.h
>> --- t/unit-tests/clar/clar/sandbox.h	2025-08-18 02:35:38 +0200
>> +++ t/unit-tests/clar/clar/sandbox.h	2025-09-05 14:10:52 +0200
>> @@ -2,6 +2,8 @@
>>   #include <sys/syslimits.h>
>>   #endif
>>   
>> +#include "../../../../compat/posix.h"
>> +
>>   static char _clar_path[4096 + 1];
>>   
>>   static int
> 
> ...seems like an obvious improvement. If we are compiling any C code,
> we'd want our compatibility macros, etc. Although it does get a little
> funny, as the contents of clar/ are imported from elsewhere, and now
> we're modifying that.
> 
> It looks like clar tries to handle portability on its own, so I guess
> another route is for it to add its own mkdtemp wrapper, and we'd import
> that fixed version. But it really feels like we're duplicating effort.

I am open to improvements here to get in the compat prototypes...

Show 20 quoted lines

> The other half of your patch is the linking side:
> 
>> diff -u -ur Makefile Makefile
>> --- Makefile	2025-08-18 02:35:38 +0200
>> +++ Makefile	2025-09-05 14:34:43 +0200
>> @@ -3933,7 +3933,7 @@
>>   $(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
>> +$(CLAR_TEST_PROG): $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS) $(COMPAT_OBJS) $(GITLIBS) GIT-LDFLAGS
>>   	$(call mkdir_p_parent_template)
>>   	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
>>   
> 
> but I'm not sure that should be necessary. The compat objects are
> included in libgit.a, and we should be linking against that when we
> build the unit-test executable. At any rate, building with NO_MKDTEMP=1
> for me on Linux does successfully find gitmkdtemp().  Are you sure this
> half of the patch was needed?

libgit.a does indeed contain the symbol:

Show 8 quoted lines

> root@deblndw002x:/var/tmp/ports/work/git-2.51.0.patched
> # nm ./libgit.a  | grep mkdtemp
> [595]    |            0|       0|FUNC |GLOB |0|   UNDEF|gitmkdtemp
> [204]    |            0|       0|FUNC |GLOB |0|   UNDEF|gitmkdtemp
> [278]    |            0|       0|FUNC |GLOB |0|   UNDEF|gitmkdtemp
> Symbols from ./libgit.a[mkdtemp.o]:
> [1]      |            0|       0|FILE |LOCAL|0|     ABS|compat/mkdtemp.c
> [110]    |            0|     272|FUNC |GLOB |0|   .text|gitmkdtemp

Let's try to revert the second half and see: ...and you are right. Since the include does properly replace the missing symbol at compiliation time, linking now works expecte:

> root@deblndw002x:/var/tmp/ports/work/git-2.51.0.patched
> # nm t/unit-tests/bin/unit-tests | grep mkdtemp
> [7332]   |            0|       0|FILE |LOCAL|0|     ABS|compat/mkdtemp.c
> [16404]  |     73286896|     272|FUNC |GLOB |0|   .text|gitmkdtemp

We can drop one hunk from the patch, great!

Michael