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

[Eli Schwartz <eschwartz@gentoo.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3260 bytes --]

On 4/22/25 3:31 AM, Patrick Steinhardt wrote:
> 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".


>> 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...



>> 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]