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@xxxxxxxxxx> >> --- >> 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:
OpenPGP_signature.asc
Description: OpenPGP digital signature
