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: <aAdF4DzFCZ3uOJCx@pks.im>
In-Reply-To: <83d9fda5-8399-47fb-87b2-a8b376cf1625@gentoo.org>

On Mon, Apr 21, 2025 at 04:04:30PM -0400, Eli Schwartz wrote:

Show 33 quoted lines

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

Show 18 quoted lines

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

Show 22 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?

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