From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 10 Apr 2025 14:50:39 GMT
Subject: Re: [PATCH v2 3/4] meson: add support for 'hdr-check'
Message-ID: <dc0d6acb-bcf1-4a7e-bfc8-7c2e7793c72b@gmail.com>
In-Reply-To: <20250410-505-wire-up-sparse-via-meson-v2-3-acb45cc8a2e5@gmail.com>

Hi Karthik

On 10/04/2025 12:30, Karthik Nayak wrote:
> The Makefile supports a target called 'hdr-check', which checks if
> individual header files can be independently compiled. Let's port this
> functionality to meson, our new build system too. The implementation
> resembles that of the Makefile and provides the same check.

Thanks for adding this, I've left a few comments below

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>   meson.build | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 107 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 790d178007..6fce1aa618 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -655,6 +655,12 @@ if git.found()
>     endforeach
>   endif
>   
> +headers_generated = [

To me "generated_headers" would be a more natural name and would match 
the style of "coccinelle_headers" from patch 1 as well as the equivalent 
in the Makefile.

> +  'command-list.h',
> +  'config-list.h',
> +  'hook-list.h'
> +]
> +
>   if not get_option('breaking_changes')
>     builtin_sources += 'builtin/pack-redundant.c'
>   endif
> @@ -1995,6 +2001,107 @@ endif
>   
>   subdir('contrib')
>   
> +headers_check_exclude = headers_generated
> +headers_check_exclude += [
> +  'compat/apple-common-crypto.h',
> +  'compat/bswap.h',
> +  'compat/compiler.h',
> +  'compat/disk.h',
> +  'compat/fsmonitor/fsm-darwin-gcc.h',
> +  'compat/fsmonitor/fsm-health.h',
> +  'compat/fsmonitor/fsm-listen.h',
> +  'compat/mingw.h',
> +  'compat/msvc.h',
> +  'compat/nedmalloc/malloc.c.h',
> +  'compat/nedmalloc/nedmalloc.h',
> +  'compat/nonblock.h',
> +  'compat/obstack.h',
> +  'compat/poll/poll.h',
> +  'compat/precompose_utf8.h',
> +  'compat/regex/regex.h',
> +  'compat/regex/regex_internal.h',
> +  'compat/sha1-chunked.h',
> +  'compat/terminal.h',
> +  'compat/vcbuild/include/sys/param.h',
> +  'compat/vcbuild/include/sys/time.h',
> +  'compat/vcbuild/include/sys/utime.h',
> +  'compat/vcbuild/include/unistd.h',
> +  'compat/vcbuild/include/utime.h',
> +  'compat/win32.h',
> +  'compat/win32/alloca.h',
> +  'compat/win32/dirent.h',
> +  'compat/win32/lazyload.h',
> +  'compat/win32/path-utils.h',
> +  'compat/win32/pthread.h',
> +  'compat/win32/syslog.h',
> +  'compat/zlib-compat.h',
> +  't/unit-tests/clar/clar.h',
> +  't/unit-tests/clar/clar/fixtures.h',
> +  't/unit-tests/clar/clar/fs.h',
> +  't/unit-tests/clar/clar/print.h',
> +  't/unit-tests/clar/clar/sandbox.h',
> +  't/unit-tests/clar/clar/summary.h',
> +  't/unit-tests/clar/test/clar_test.h',
> +  'unicode-width.h',
> +  'xdiff/xdiff.h',
> +  'xdiff/xdiffi.h',
> +  'xdiff/xemit.h',
> +  'xdiff/xinclude.h',
> +  'xdiff/xmacros.h',
> +  'xdiff/xprepare.h',
> +  'xdiff/xtypes.h',
> +  'xdiff/xutils.h',
> +]

Having to manually maintain this list is a bit of a shame as every time 
a new file is added to compat we need to add it to meson.build twice. 
The Makefile avoids this by filtering the list of headers used when 
building git based on their path - can we do the same here?

> +if sha1_backend != 'openssl'
> +  headers_check_exclude += 'sha1/openssl.h'
> +endif
> +if sha256_backend != 'openssl'
> +  headers_check_exclude += 'sha256/openssl.h'
> +endif
> +if sha256_backend != 'nettle'
> +  headers_check_exclude += 'sha256/nettle.h'
> +endif
> +if sha256_backend != 'gcrpyt'
> +  headers_check_exclude += 'sha256/gcrypt.h'
> +endif
> +
> +if headers.length() != 0 and compiler.get_argument_syntax() == 'gcc'
> +  hco_targets = []
> +  foreach h : headers
> +    if headers_check_exclude.contains(h)
> +      continue
> +    endif
> +
> +    hcc = custom_target(
> +      input: h,
> +      output: h.underscorify() + 'cc',
> +      command: [
> +        shell,
> +        '-c',
> +        'echo \'#include "git-compat-util.h"\' > @OUTPUT@ && echo -n \'#include "' + h + '"\' >> @OUTPUT@'

This line is rather long. Also do we really need "echo -n" here, the 
Makefile does not use it.

> +      ]
> +    )
> +
> +    hco = custom_target(
> +      input: hcc,
> +      output: h.underscorify().replace('.h', '.hco'),
> +      command: [
> +        compiler.cmd_array(),
> +        libgit_c_args,
> +        '-I', meson.project_source_root(),
> +        '-I', meson.project_source_root() / 't/unit-tests',

Do you know why we don't need to add these include paths for the 
equivalent rule in the Makefile?

Thanks

Phillip

> +        '-o', '/dev/null',
> +        '-c', '-xc',
> +        '@INPUT@'
> +      ]
> +    )
> +    hco_targets += hco
> +  endforeach
> +
> +  alias_target('hdr-check', hco_targets)
> +endif
> +
>   foreach key, value : {
>     'DIFF': diff.full_path(),
>     'GIT_SOURCE_DIR': meson.project_source_root(),
>