Re: [PATCH RFC v6 18/19] Introduce support for the Meson build system

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Nov 13, 2024, 16:30 UTC
Message-ID: <cf51bd62-0573-42ba-8784-643bde347d10@gmail.com>
In-Reply-To: <20241112-pks-meson-v6-18-648b30996827@pks.im>

Hi Patrick

On 12/11/2024 17:03, Patrick Steinhardt wrote:

I had a read through to see what the meson syntax was like and it seems to be quite nice. I've left a couple of comments below. It's an impressive amount of work to get to this stage, it's a bit daunting that replicating everything in our current Makefile is even more work!

> +#      # Set up a build directory with 'address' and 'undefined' sanitizers
> +#      # using Clang.
> +#      $ CC=clang meson setup -Db_sanitize=address,undefined

Unfortunately when building with this the tests fail with

==217115==ASan runtime does not come first in initial library list; you should either link runtime to your application or manually preload it with LD_PRELOAD. ./test-lib.sh: line 1080: 217115 Aborted (core dumped) git switch -C primary error: last command exited with $?=134

because we don't wire up -Db_sanitize to SANITIZE_ADDRESS in GIT-BUILD-OPTIONS. The Makefile sets a few other build options as well with SANATIZE=address which we should replicate here. I was suprised meson wont allow -Db_sanitize=address,leak.

I assume that it would be possible to set some default CFLAGS to match the Makefile and have something like DEVELOPER and DEVOPTS if we adopt this? As a contributor it's really convenient to be able to set DEVELOPER=1 and have a sane set of compiler flags. It's really good for the project as it makes it easy for everyone to compile their code with a standard set of warnings enabled.

Show 10 quoted lines

> +if compiler.compiles('''
> +  #include <inttypes.h>
> +
> +  void func(void)
> +  {
> +    uintmax_t x = 0;
> +  }
> +''', name: 'uintmax_t')
> +  libgit_c_args += '-DNO_UINTMAX_T'
> +endif

I think the logic is backwards here - shouldn't we be defining NO_UINTMAX_T if it does not compile? uintptr_t is optional so we should be checking for that and defining NO_UINTPTR_T if it's missing as well I think.

Show 12 quoted lines

> +if compiler.run('''
> +  #include <stdio.h>
> +
> +  int main(int argc, const char **argv)
> +  {
> +    FILE *f = fopen(".", "r");
> +    return f ? 0 : 1;
> +  }
> +''', name: 'fread reads directories').returncode() == 0
> +  libgit_c_args += '-DFREAD_READS_DIRECTORIES'
> +  libgit_sources += 'compat/fopen.c'
> +endif

This checks a property of the build host so should it be guarded with if not meson.is_cross_build() as below?

Show 5 quoted lines

> +# Build a separate library for "version.c" so that we do not have to rebuild
> +# everything when the current Git commit changes. TODO: this only gets set up
> +# at configuration time, so we do not notice version changes unless the build
> +# instructions get regenerated. We should refactor the source file such that we
> +# can substitute tags in the file via `vcs_tag()`.

There are three version dependent strings GIT_VERSION, GIT_BUILT_FROM_COMMIT and GIT_USER_AGENT - can vcs_tag() handle all three or do we want a script that writes a header?

Best Wishes

Phillip