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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Apr 21, 2025 at 04:04:30PM -0400, Eli Schwartz wrote:
> On 4/21/25 1:51 PM, Eli Schwartz wrote:
> > diff --git a/meson.build b/meson.build
> > index c47cb79af0..6c147c22a4 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -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 (???)

Our CMake build instructions shouldn't be treated as canonical source of
truth. They're good enough for some usecases, but they are not as
feature complete as any of Makefile/autoconf/Meson.

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

I'd rather prefer to try and detect this generically instead of adding
more platform-specific configuration. It is way simpler to maintain, and
if we ever see that things don't work well on a specific platform we may
still reconsider at that point in time.

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

Inconsistencies like this are what you get in a codebase that is 20
years old. Many things have grown organically, and one hand doesn't
always know what the other hand is doing.

I agree that in the best case we'd unify these. But unfortunately, this
is not trivial because those are part of the build interface for our
Makefiles. People may have `HAVE_GETDELIM = YesPlease` in their
`config.mak` file, and we don't want to break this usecase. So if we
were to change this in the future, we'd have to also introduce shims for
backwards compatibility.

Patrick