On 07/07/2025 16:40, Junio C Hamano wrote: > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > >>> need to link a separate library (-lsysinfo). (This would require >>> a similar change to meson.build). >>> >>> - change the order of the preprocessor conditionals in the total_ram() >>> function in 'builtin/gc.c', so that the *BSD sysctl() function >>> (in the HAVE_BSD_SYSCTL block) takes priority over the sysinfo() >>> function (in the HAVE_SYSINFO block). >>> >>> - suppress the setting of HAVE_SYSINFO when HAVE_BSD_SYSCTL has been >>> defined (in both configure.ac and meson.build). >>> ... >>> The second solution would only be required by the autoconf and meson >>> build systems, the Makefile already sets the build variables to the >>> required values (since they are not 'auto-detected'). >> ... >> The final solution is almost certainly good enough (and is definitely >> simple), although the second solution has the benefit that it "fixes" >> the problem once and for all even if someone defines both >> HAVE_BSD_SYSCTL and HAVE_SYSINFO (say, in config.mak), assuming I'm >> understanding correctly. > > Yeah, I think I agree with this assessment. [Sorry for the late reply - real life keeps getting in the way!] Yep, I thought about including this fix *in addition to* the solution implemented in this patch, but decided that the chances that anyone would set both in a Makefile build was practically zero. (famous last words ;) ). Of course, practically zero is not zero, so we could do this in a follow-up patch if we wanted to take a more conservative approach. (Carlos has a series in progress which would conflict with such a patch - but the conflict resolution would be simple). >>> In order to fix the FreeBSD build, move the sysinfo() check after the >>> determination of the HAVE_BSD_SYSCTL build variable, suppressing the >>> setting of HAVE_SYSINFO if HAVE_BSD_SYSCTL is defined. Apply this logic >>> to both the configure.ac and meson.build file. >> >> Nicely described. I wasn't really following along with the discussion, >> but this commit message summarizes the situation well, so I can >> understand the reason for the change and (I hope) the implications. > > Agreed. Thanks, all. Thanks! Let me know if you would like that follow-up patch. ATB, Ramsay Jones
