Re: [PATCH v12 1/7] buildtools/chkincs: relax C linkage requirement

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2024-10-06 17:58, Robin Jarry wrote:
> Mattias Rönnblom, Oct 06, 2024 at 16:17:
>>> I think you need to change run_command() to custom_target(). I was 
>>> thinking of this patch to be much simpler as follows:
>>
>> I started off with a shell script, but I switched to Python when I 
>> wanted to strip off comments. (Not that that can't be done in bourne 
>> shell.)
>>
>> The below script doesn't strip off comments, and provides no usage 
>> information. Earlier I got the impression you wanted to improve 
>> command-line usage.
>>
>> I think we need to decide if this thing is an integral part of the 
>> build system, custom-made for its needs, or if its something that 
>> should also be friendly to a command-line human user.
>>
>> I could go either way on that.
> 
> We don't have to choose. Being part of the build system does not mean 

If it's an integral part of the build system, and the only user is 
another program (e.g., meson.build), the script can be made simpler. For 
example, you need not bother with usage information, since a computer 
program will not have any use of it.

It would be helpful if Bruce could comment on this. How should we think 
about small tools like this? That are more-or-less just an artifact of 
the lack of expressiveness in meson.

I'm leaning toward having them as proper human-usable CLI tools, 
provided it doesn't cause too much fuzz on the meson.build side of things.

> the script cannot use the standard python tools to parse arguments. Here 
> is what I came up with. It is shorter, strips comments and deals with 
> arguments in a simpler way. We don't need to pass the "missing" or 
> "redundant" operation to the script, it can figure out what to do on its 
> own (see inline comment in main()):

Sure, you could check for both issues at the same time. The primary 
reason for doing otherwise was that the code base wouldn't survive the 
"redundant" check at the time of the script's introduction (in the patch 
set).

> 
> diff --git a/buildtools/chkincs/check_extern_c.py b/buildtools/chkincs/ 
> check_extern_c.py
> new file mode 100755
> index 000000000000..3e61719a5ea5
> --- /dev/null
> +++ b/buildtools/chkincs/check_extern_c.py
> @@ -0,0 +1,72 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: BSD-3-Clause
> +
> +"""
> +Detect missing or redundant `extern "C"` statements in headers.
> +"""
> +
> +import argparse
> +import re
> +import sys
> +
> +
> +# regular expressions
> +EXTERN_C_RE = re.compile(r'^extern\s+"C"', re.MULTILINE)
> +CPP_LINE_COMMENT_RE = re.compile(r"//.*$", re.MULTILINE)
> +C_BLOCK_COMMENT_RE = re.compile(r"/\*.+?\*/", re.DOTALL)
> +C_SYMBOL_RE = [
> +    # external variable definitions
> +    re.compile(r"^extern\s+[a-z0-9_]+\s", re.MULTILINE),
> +    # exported functions
> +    re.compile(r"\brte_[a-z0-9_]+\("),
> +    re.compile(r"\bcmdline_[a-z0-9_]+\("),
> +    re.compile(r"\bvt100_[a-z0-9_]+\("),
> +    re.compile(r"\brdline_[a-z0-9_]+\("),
> +    re.compile(r"\bcirbuf_[a-z0-9_]+\("),
> +    # windows compatibility
> +    re.compile(r"\bpthread_[a-z0-9_]+\("),
> +    re.compile(r"\bregcomp\("),
> +    re.compile(r"\bcount_cpu\("),
> +]
> +
> +
> +def strip_comments(buf: str) -> str:
> +    buf = CPP_LINE_COMMENT_RE.sub("", buf, re.MULTILINE)
> +    return C_BLOCK_COMMENT_RE.sub("", buf, re.DOTALL)
> +
> +
> +def has_c_symbols(buf: str) -> bool:
> +    for expr in C_SYMBOL_RE:
> +        if expr.search(buf, re.MULTILINE):
> +            return True
> +    return False
> +
> +
> +def main() -> int:
> +    parser = argparse.ArgumentParser(description=__doc__)
> +    parser.add_argument("headers", nargs="+")
> +    args = parser.parse_args()
> +
> +    ret = 0
> +
> +    for h in args.headers:
> +        with open(h) as f:
> +            buf = f.read()
> +
> +        buf = strip_comments(buf)
> +
> +        if has_c_symbols(buf):
> +            if not EXTERN_C_RE.search(buf):
> +                print('error: missing extern "C" in', h)
> +                ret = 1
> +        # Uncomment next block once all extraneous extern "C" have been 
> removed
> +        #else:
> +        #    if EXTERN_C_RE.search(buf):
> +        #        print('error: extraneous extern "C" in', h)
> +        #        ret = 1
> +
> +    return ret
> +
> +
> +if __name__ == "__main__":
> +    sys.exit(main())
> diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/ 
> meson.build
> index f2dadcae18ef..a551b2df9268 100644
> --- a/buildtools/chkincs/meson.build
> +++ b/buildtools/chkincs/meson.build
> @@ -38,14 +38,11 @@ if not add_languages('cpp', required: false)
> endif
> 
> # check for extern C in files, since this is not detected as an error by 
> the compiler
> -grep = find_program('grep', required: false)
> -if grep.found()
> -    errlist = run_command([grep, '--files-without-match', '^extern 
> "C"', dpdk_chkinc_headers],
> -            check: false, capture: true).stdout().split()
> -    if errlist != []
> -        error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n- 
> '.join(errlist))
> -    endif
> -endif
> +custom_target('check-extern-c',
> +        command: files('check_extern_c.py') + ['@INPUT@'],
> +        input: dpdk_chkinc_headers,
> +        output: 'check-extern-c',
> +        build_by_default: true)
> 
> gen_cpp_files = generator(gen_c_file_for_header,
>          output: '@BASENAME@.cpp',
> 

I won't review this in detail, but you can always submit a new patch 
against what's on main. What is there now is somewhat half-way between 
"proper CLI tool" and "meson.build delegate".