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

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

On 2024-10-06 14:25, Robin Jarry wrote:
> Mattias Rönnblom, Oct 06, 2024 at 11:47:
>> OK, so it does change something, but not for the better, seemingly.
>>
>> meson.build now somehow has to distinguished between two different 
>> "errors":
>> 1) One or more offending header files were found.
>> 2) Any other kind of error (file not found, access control issues, 
>> internal script error, etc).
>>
>> For 1), it should print a list of the offending header files.
>>
>> If you just set "check: true" in run_command(), and have the 
>> chkextern.py script return non-zero on error, the offending header 
>> files will not be printed on stdout.
>>
>> My guess would be that this complicating factor is the reason for this 
>> pattern of not (in/from meson) depending on the script delegate exit 
>> code, but rather the output.
>>
>> May I should rename the script to "scanextern.py" to better reflect 
>> that a failure to find an issue is not an error (=non-zero exit code).
> 
> 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.

> 
> diff --git a/buildtools/chkincs/check_extern_c.sh b/buildtools/chkincs/ 
> check_extern_c.sh
> new file mode 100755
> index 000000000000..96ace5ffd248
> --- /dev/null
> +++ b/buildtools/chkincs/check_extern_c.sh
> @@ -0,0 +1,49 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: BSD-3-Clause
> +
> +has_symbols() {
> +    grep -Eq \
> +        -e 'rte_[a-z0-9_]+\(' \
> +        -e 'cmdline_[a-z0-9_]+\(' \
> +        -e 'vt100_[a-z0-9_]+\(' \
> +        -e 'rdline_[a-z0-9_]+\(' \
> +        -e 'cirbuf_[a-z0-9_]+\(' \
> +        -e 'pthread_[a-z0-9_]+\(' \
> +        -e 'regcomp\(' \
> +        -e 'count_cpu\(' \
> +        -e '^extern[[:space:]]+[a-z0-9_]+[[:space:]]' \
> +        "$1"
> +}
> +
> +has_extern_c() {
> +    grep -q 'extern "C"' "$1"
> +}
> +
> +mode=$1
> +shift
> +ret=0
> +
> +case "$mode" in

Here the shell script purist notes that getopt(1) is not used. :)

> +--missing)
> +    for header in "$@"; do
> +        if has_symbols "$header" && ! has_extern_c "$header"; then
> +            echo "error: missing extern \"C\": $header" >&2
> +            ret=1
> +        fi
> +    done
> +    ;;
> +--redundant)
> +    for header in "$@"; do
> +        if ! has_symbols "$header" && has_extern_c "$header"; then
> +            echo "error: missing extern \"C\": $header" >&2
> +            exit 1
> +        fi
> +    done
> +    ;;
> +*)
> +    echo "error: unsupported mode '$mode'" >&2
> +    ret=1
> +esac
> +
> +exit $ret
> diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/ 
> meson.build
> index f2dadcae18ef..b463845083eb 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.sh') + ['--missing', '@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',
>