Re: [PATCH 5/5] configure.ac: upgrade to a compilation check for sysinfo

From: Eli Schwartz <eschwartz@gentoo.org>
Date: May 8, 2025, 21:07 UTC
Message-ID: <9baad29d-a5bf-443d-98a1-36d7020e5835@gentoo.org>
In-Reply-To: <20250508164443.1506440-6-ramsay@ramsayjones.plus.com>

On 5/8/25 12:44 PM, Ramsay Jones wrote:

Show 21 quoted lines

> Commit f5e3c6c57d ("meson: do a full usage-based compile check for
> sysinfo", 2025-04-25) updated the 'sysinfo()' check, as part of the
> meson build, due to the failure of the check on Solaris. Prior to
> that commit, the meson build only checked the availability of the
> '<sys/sysinfo.h>' header file. On Solaris, both the header and the
> 'sysinfo()' function exist, but are completely unrelated to the same
> function on Linux (and cygwin).
> 
> Commit 50dec7c566 ("config.mak.uname: add sysinfo() configuration for
> cygwin", 2025-04-17) added a similar 'sysinfo()' check to the autoconf
> build. This check looked for the 'sysinfo()' function itself, rather
> than just the header, but it will fail (incorrectly set HAVE_SYSINFO)
> for the same reason.
> 
> In order to correctly identify the 'sysinfo()' function we require as
> part of 'git-gc' (used in the 'total_ram() function), we also upgrade
> to a compilation check, in a similar way to the meson commit. Note that
> since commit c9a51775a3 ("builtin/gc.c: correct RAM calculation when
> using sysinfo", 2025-04-17) both the 'totalram' and 'mem_unit' fields
> of the 'struct sysinfo' are used, so the new check includes both of
> those fields in the compile check.

and

> Note that I cannot test the new autoconf check in patch #5 (I don't have
> access to a Solaris system). I _think_ it will correctly unset HAVE_SYSINFO
> on Solaris, but I cannot confirm that. (I can only test on Linux and cygwin).

Well, I can confirm this results in the detection being correctly changed on Solaris 11.3 and stop reporting sysinfo as available during ./configure, so this has my ACK on technical grounds. That being said, in the original meson thread, there was this review:

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

Show 21 quoted lines

> On Mon, Apr 21, 2025 at 01:51:46PM -0400, Eli Schwartz wrote:
>> It is deprecated and removed in SUS v3 / POSIX 2001, so various systems
>> may not include it. Solaris, in particular, carefully refrains from
>> defining it except inside of a maze of `#ifdef` to make sure you have
>> kept your nose clean and only used it in code that *targets* SUS v2 or
>> earlier.
>>
>> config.mak.uname defines this automatically, though only for QNX.
> 
> Ah, interesting. I mostly went by our autoconf infrastructure when
> converting the checks, which didn't have a check for `getpagesize()`
> either. We might want to teach autoconf to check for this function while
> at it.
> 
> In all honesty though, I rather hope that we're soon in a state where we
> can just drop autoconf altogether in favor of Meson. The only two
> blockers I'm aware of are wiring up git-gui and gitk. The former project
> has already been adapted upstream, the latter is still in review. But
> once those have landed, we should be ready to mark Meson as stable and
> then we can start deprecating autoconf unless there are good reasons not
> to do so.

So you are indeed teaching autoconf to check for this function, but should we also ask whether it's worth continued maintenance of autoconf? It was/is not clear to me who the stakeholders are for the autoconf support.

On the one hand, it exists so maybe it should be fixed when we know it has issues.

On the other hand, it sounds like this patch (and commit 50dec7c566 "config.mak.uname: add sysinfo() configuration for cygwin") only modify autoconf out of a sense of duty, rather than finding autoconf useful. What does it say about the autoconf support if the people finding bugs in it don't even use it, but only discovered the bug while working on a different build system they do use and depend on (config.mak.uname, or meson.build, both count here). Who *is* using it? Apparently not Solaris users?

-- 
Eli Schwartz