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

From: Eli Schwartz <eschwartz@gentoo.org>
Date: Apr 21, 2025, 20:04 UTC
Message-ID: <83d9fda5-8399-47fb-87b2-a8b376cf1625@gentoo.org>
In-Reply-To: <20250421175247.240971-1-eschwartz@gentoo.org>

On 4/21/25 1:51 PM, 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.

By the way, when reviewing this I was having a slightly hard time figuring out which stuff belonged here... specifically, because of the differences in macro prefixes lead me to believe it's not always so simple as "does it exist".

Show 80 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
> +  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
> +  endif
> +endforeach
>  
> -if not compiler.has_function('strlcpy')
> -  libgit_c_args += '-DNO_STRLCPY'
> -  libgit_sources += 'compat/strlcpy.c'
> +if compiler.has_function('sync_file_range')
> +  libgit_c_args += '-DHAVE_SYNC_FILE_RANGE'
>  endif
>  
>  if not compiler.has_function('strdup')
> @@ -1322,45 +1339,15 @@ if not compiler.has_function('strtoumax')
>    ]
>  endif
>  
> -if not compiler.has_function('strtoull')
> -  libgit_c_args += '-DNO_STRTOULL'
> -endif
> -
> -if not compiler.has_function('setenv')
> -  libgit_c_args += '-DNO_SETENV'
> -  libgit_sources += 'compat/setenv.c'
> -endif
> -
>  if not compiler.has_function('qsort')
>    libgit_c_args += '-DINTERNAL_QSORT'
>  endif
>  libgit_sources += 'compat/qsort_s.c'

... for example, the Makefile says here:

# Define INTERNAL_QSORT to use Git's implementation of qsort(), which # is a simplified version of the merge sort used in glibc. This is # recommended if Git triggers O(n^2) behavior in your platform's # qsort().

cmake unconditionally defines it (???)

config.mak.uname says:

- AIX:
  INTERNAL_QSORT = UnfortunatelyYes

  Seems to date back to commit 377d9c409ffe0f0d994b929aeb94716139207b9d.
  "Unfortunate" indeed.

- MinGW:
  INTERNAL_QSORT = YesPlease

  Windows claims to have a qsort but perhaps it is very slow and bes
  avoided?

We should probably stop *checking* for qsort and simply encode the platforms we know are slow and automatically skip it there. Can I get confirmation regarding Windows? :)

Show 18 quoted lines

> -# unsetenv is provided by compat/mingw.c.
> -if host_machine.system() != 'windows' and not compiler.has_function('unsetenv')
> -  libgit_c_args += '-DNO_UNSETENV'
> -  libgit_sources += 'compat/unsetenv.c'
> -endif
> -
> -if not compiler.has_function('mkdtemp')
> -  libgit_c_args += '-DNO_MKDTEMP'
> -  libgit_sources += 'compat/mkdtemp.c'
> -endif
> -
> -if not compiler.has_function('initgroups')
> -  libgit_c_args += '-DNO_INITGROUPS'
> -endif
> -
>  if compiler.has_function('getdelim')
>    libgit_c_args += '-DHAVE_GETDELIM'
>  endif

But stuff like this, why isn't it consistent with the other functions? What's the difference between HAVE_XXX and NO_XXX?

Show 9 quoted lines

> -if host_machine.system() == 'windows'
> -  libgit_c_args += '-DUSE_WIN32_MMAP'
> -elif not compiler.has_function('mmap')
> -  libgit_c_args += '-DNO_MMAP'
> -  libgit_sources += 'compat/mmap.c'
> -endif
>  
>  if compiler.has_function('clock_gettime')
>    libgit_c_args += '-DHAVE_CLOCK_GETTIME'

-- 
Eli Schwartz