Re: [PATCH 6/6] meson: only check for missing networking syms on non-Windows; add compat impls

From: Eli Schwartz <eschwartz@gentoo.org>
Date: Apr 22, 2025, 15:27 UTC
Message-ID: <aadfbd6b-ea1c-475e-b6a9-f1552afa06c8@gentoo.org>
In-Reply-To: <aAdF3eu1heCycaLJ@pks.im>

On 4/22/25 3:31 AM, Patrick Steinhardt wrote:

Show 12 quoted lines

> On Mon, Apr 21, 2025 at 01:51:50PM -0400, Eli Schwartz wrote:
>> These are added in the Makefile, but not in meson. They probably won't
>> work well on systems without them.
>>
>> CMake adds them, but only on non-Windows. Actually, it only performs
>> compiler checks for hstrerror, but excludes that check on Windows with
>> the note that it is "incompatible with the Windows build". This seems to
>> be misleading -- it is not incompatible, it simply doesn't exist. Still,
>> the compat version should not be used.
> 
> CMake only checks for `hstrerror()` though -- it doesn't check for the
> other functions at all.

Right, that's what I meant.

"cmake, like this patch, adds the compat/*.c when checking function availability. Actually, it only performs compiler checks for hstrerror, but does add the compat impl in that case".

Show 8 quoted lines

>> I interpret this cmake logic to mean we shouldn't even be checking for
>> symbol availability on Windows. In addition to making it simple to add
>> compat definitions, this also probably shaves off a second or two of
>> configure time on Windows as no compiler check needs to be performed.
> 
> I dunno. In this case I'd lean towards just using the check on Windows,
> too. The less platform-specific configuration we do the easier the build
> system is to reason about.

Maybe, but the issue is that it appears on Windows it is not correct to add the compat impl for hstrerror, which would still be a platform-specific configuration. Do I check for hstrerror everywhere but configure Windows to not add the compat impl, but do add it for inet_ntop and inet_pton? That is more configuration than skipping the checks on Windows...

Show 31 quoted lines

>> Signed-off-by: Eli Schwartz <eschwartz@gentoo.org>
>> ---
>>  meson.build | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/meson.build b/meson.build
>> index 1b7e55756b..24b304fb57 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -1088,11 +1088,14 @@ else
>>  endif
>>  libgit_dependencies += networking_dependencies
>>  
>> -foreach symbol : ['inet_ntop', 'inet_pton', 'hstrerror']
>> -  if not compiler.has_function(symbol, dependencies: networking_dependencies)
>> -    libgit_c_args += '-DNO_' + symbol.to_upper()
>> -  endif
>> -endforeach
>> +if host_machine.system() != 'windows'
>> +  foreach symbol : ['inet_ntop', 'inet_pton', 'hstrerror']
>> +    if not compiler.has_function(symbol, dependencies: networking_dependencies)
>> +      libgit_c_args += '-DNO_' + symbol.to_upper()
>> +      libgit_sources += 'compat/' + symbol + '.c'
>> +    endif
>> +  endforeach
>> +endif
> 
> We do have compat sources for `inet_ntop()` and `inet_pton()` indeed, so
> adding those makes sense. But we don't have a replacement for
> `hstrerror()`, so if that function wasn't found we would error out
> because "compat/hstrerror.c" wasn't found.

I don't really understand what you mean by this. Of course the file will be found.

$ grep hstrerror compat/hstrerror.c const char *githstrerror(int err)

File is there. (The function name is then #defined by compat/posix.h, no comment.)

-- 
Eli Schwartz