Re: [PATCH 1/6] meson: simplify and parameterize various standard function checks

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 22, 2025, 07:31 UTC
Message-ID: <aAdFysi-n_5Aa4Au@pks.im>
In-Reply-To: <20250421175247.240971-1-eschwartz@gentoo.org>

On Mon, Apr 21, 2025 at 01:51:45PM -0400, Eli Schwartz wrote:

Show 10 quoted lines

> This is repetitive logic. We either want to use some -lc function, or if
> it is not available we define it as -DNO_XXX and usually (but not
> always) provide some custom compatibility impl instead.
> 
> Checking the intent of each block when reading through the file is slow
> and not very DRY. Switch to taking an array of checkable functions
> instead.
> 
> Not all functions are straightforward to move, since different macro
> prefixes are used.

Yeah, this is somewhat unfortunate indeed. I think in the long term we might want to unify our approach so that we consistently use e.g. `HAVE_SOME_FUNCTION` or `NO_SOME_FUNCTION`.

Show 46 quoted lines

> Signed-off-by: Eli Schwartz <eschwartz@gentoo.org>
> ---
>  meson.build | 73 ++++++++++++++++++++++-------------------------------
>  1 file changed, 30 insertions(+), 43 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index c47cb79af0..6c147c22a4 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1290,23 +1290,40 @@ if not compiler.has_member('struct passwd', 'pw_gecos', prefix: '#include <pwd.h
>    libgit_c_args += '-DNO_GECOS_IN_PWENT'
>  endif
>  
> -if compiler.has_function('sync_file_range')
> -  libgit_c_args += '-DHAVE_SYNC_FILE_RANGE'
> -endif
> +checkfuncs = [
> +  'strcasestr',
> +  'memmem',
> +  'strlcpy',
> +  # no compat
> +  'strtoull',
> +  'setenv',
> +  'mkdtemp',
> +  # no compat
> +  'initgroups',
> +]
>  
> -if not compiler.has_function('strcasestr')
> -  libgit_c_args += '-DNO_STRCASESTR'
> -  libgit_sources += 'compat/strcasestr.c'
> +if host_machine.system() == 'windows'
> +  libgit_c_args += '-DUSE_WIN32_MMAP'
> +else
> +  checkfuncs += [
> +    'mmap',
> +    # unsetenv is provided by compat/mingw.c.
> +    'unsetenv',
> +  ]
>  endif
>  
> -if not compiler.has_function('memmem')
> -  libgit_c_args += '-DNO_MEMMEM'
> -  libgit_sources += 'compat/memmem.c'
> -endif
> +foreach func: checkfuncs

Our current code style puts a space both before and after the colon.

Show 6 quoted lines

> +  if not compiler.has_function(func)
> +    libgit_c_args += '-DNO_' + func.to_upper()
> +    impl = 'compat/' + func + '.c'
> +    if fs.exists(impl)
> +      libgit_sources += impl
> +    endif

I think this is a bit too magic-y. An alternative might be to have `checkfuncs` be a dictionary where the value of each function is the compat sources that we fall back to.

Other than that I really like the simplifications introduced by this patch, thanks!

Patrick