From: "Osipov, Michael (IN IT IN)" <michael.osipov@innomotics•com>
To: Jeff King <peff@peff•net>
Cc: Patrick Steinhardt <ps@pks•im>, git@vger•kernel.org
Subject: Re: [Bug] Compat objects not added to CLAR_TEST_PROG
Date: Sat, 6 Sep 2025 00:12:54 +0200 [thread overview]
Message-ID: <41515d85-7afd-4c4a-b0b3-4cd902ca5677@innomotics.com> (raw)
In-Reply-To: <20250905213708.GB612697@coredump.intra.peff.net>
Let's have a look:
On 2025-09-05 23:37, Jeff King wrote:
> [+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...
> 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:
> 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
next prev parent reply other threads:[~2025-09-05 22:13 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-05 13:19 [Bug] Compat objects not added to CLAR_TEST_PROG Osipov, Michael (IN IT IN)
2025-09-05 21:37 ` Jeff King
2025-09-05 22:12 ` Osipov, Michael (IN IT IN) [this message]
2025-09-09 7:45 ` Patrick Steinhardt
2025-09-09 8:00 ` Osipov, Michael (IN IT IN)
2025-09-09 10:15 ` Patrick Steinhardt
2025-09-09 12:07 ` Osipov, Michael (IN IT IN)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=41515d85-7afd-4c4a-b0b3-4cd902ca5677@innomotics.com \
--to=michael.osipov@innomotics$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox