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

[Eli Schwartz <eschwartz@gentoo.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4850 bytes --]

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



> 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? :)


> -# 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?


> -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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]