From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id AEEDB45B01; Thu, 10 Oct 2024 12:24:34 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 78C6640298; Thu, 10 Oct 2024 12:24:34 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 133EA4025E for ; Thu, 10 Oct 2024 12:24:33 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id A17EE1E7EB for ; Thu, 10 Oct 2024 12:24:32 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 7EA561E84B; Thu, 10 Oct 2024 12:24:32 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,AWL, T_SCC_BODY_TEXT_LINE autolearn=disabled version=4.0.0 X-Spam-Score: -1.0 Received: from [192.168.30.130] (host-217-213-113-219.mobileonline.telia.com [217.213.113.219]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 28E2B1E7E9; Thu, 10 Oct 2024 12:24:28 +0200 (CEST) Message-ID: <0f81b9f3-8724-4bfb-bcd4-8d5c7c035d15@lysator.liu.se> Date: Thu, 10 Oct 2024 12:24:25 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v12 1/7] buildtools/chkincs: relax C linkage requirement To: Robin Jarry , =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , dev@dpdk.org Cc: Heng Wang , Stephen Hemminger , Tyler Retzlaff , =?UTF-8?Q?Morten_Br=C3=B8rup?= , Jack Bond-Preston , David Marchand , Chengwen Feng , "Richardson, Bruce" References: <20240920062437.738706-2-mattias.ronnblom@ericsson.com> <20240920104754.739033-1-mattias.ronnblom@ericsson.com> <20240920104754.739033-2-mattias.ronnblom@ericsson.com> <74e86a66-4341-465d-bcba-d76db66035cd@lysator.liu.se> <722ddf9d-29ed-4d3b-bc54-ac2aafefd2bb@lysator.liu.se> <6fd9040d-d923-42e2-8eab-b7ad5e48e28c@lysator.liu.se> <2ee279b5-910f-4428-84e0-6a14dd4dfd5c@lysator.liu.se> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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".