From: Osipov, Michael (IN IT IN) <michael.osipov@innomotics.com>
Date: Tue, 09 Sep 2025 08:00:54 GMT
Subject: Re: [Bug] Compat objects not added to CLAR_TEST_PROG
Message-ID: <50da35ac-71f8-49dd-bcd8-83726f1954a9@innomotics.com>
In-Reply-To: <aL_bAxZubXMOGWsu@pks.im>



On 2025-09-09 09:45, Patrick Steinhardt wrote:
> On Fri, Sep 05, 2025 at 05:37:08PM -0400, Jeff King wrote:
>> On Fri, Sep 05, 2025 at 03:19:50PM +0200, Osipov, Michael (IN IT IN) wrote:
>>> 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.
> 
> We're duplicating effort indeed, but that effort benefits other
> projects that use clar.
> 
> In any case, we already have logic to detect whether or not the platform
> should have `mkdtemp()`:
> 
>      #if defined(__MINGW32__)
>          if (_mktemp(_clar_tempdir) == NULL)
>              return -1;
> 
>          if (mkdir(_clar_tempdir, 0700) != 0)
>              return -1;
>      #elif defined(_WIN32)
>          if (_mktemp_s(_clar_tempdir, sizeof(_clar_tempdir)) != 0)
>              return -1;
> 
>          if (mkdir(_clar_tempdir, 0700) != 0)
>              return -1;
>      #elif defined(__sun) || defined(__TANDEM)
>          if (mktemp(_clar_tempdir) == NULL)
>              return -1;
> 
>          if (mkdir(_clar_tempdir, 0700) != 0)
>              return -1;
>      #else
>          if (mkdtemp(_clar_tempdir) == NULL)
>              return -1;
>      #endif
> 
> So that raises the question whether HP-UX has mktemp(3p) -- if so, we
> can probably fix the issue like this:
> 
> diff --git a/clar/sandbox.h b/clar/sandbox.h
> index ff43159..5af36f3 100644
> --- a/clar/sandbox.h
> +++ b/clar/sandbox.h
> @@ -164,7 +164,7 @@ static int build_tempdir_path(void)
>   
>   	if (mkdir(_clar_tempdir, 0700) != 0)
>   		return -1;
> -#elif defined(__sun) || defined(__TANDEM)
> +#elif defined(__sun) || defined(__TANDEM) || defined(__HPUX)
>   	if (mktemp(_clar_tempdir) == NULL)
>   		return -1;
>   
> The `__HPUX` define is pulled out of thin air, I have no idea what
> preprocessor macro that system sets. But something in that spirit may
> fix that issue. If so, I'm happy to fix this upstream and then pull
> the latest version into Git.

I can confirm that your idea works and much better than my idea:
root@deblndw002x:/var/tmp/ports/work
# diff -ur git-2.51.0 git-2.51.0.patched/ | grep -v "Only in"
diff -u -ur git-2.51.0/t/unit-tests/clar/clar/sandbox.h 
git-2.51.0.patched/t/unit-tests/clar/clar/sandbox.h
--- git-2.51.0/t/unit-tests/clar/clar/sandbox.h 2025-08-18 02:35:38 +0200
+++ git-2.51.0.patched/t/unit-tests/clar/clar/sandbox.h 2025-09-09 
09:50:07 +0200
@@ -128,7 +128,7 @@

         if (mkdir(_clar_path, 0700) != 0)
                 return -1;
-#elif defined(__sun) || defined(__TANDEM)
+#elif defined(__sun) || defined(__TANDEM) || defined(__hpux)
         if (mktemp(_clar_path) == NULL)
                 return -1;

Can you make that happen upstream?

Thanks,

Michael