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 7CA4D45AAC; Fri, 4 Oct 2024 10:40:47 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 644CC402DD; Fri, 4 Oct 2024 10:40:47 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 3915D4029D for ; Fri, 4 Oct 2024 10:40:45 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 920DC41E0 for ; Fri, 4 Oct 2024 10:40:44 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 6A62B417D; Fri, 4 Oct 2024 10:40:44 +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.2 required=5.0 tests=ALL_TRUSTED,AWL, T_SCC_BODY_TEXT_LINE autolearn=disabled version=4.0.0 X-Spam-Score: -1.2 Received: from [192.168.1.86] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (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 BA0A6417B; Fri, 4 Oct 2024 10:40:41 +0200 (CEST) Message-ID: <74e86a66-4341-465d-bcba-d76db66035cd@lysator.liu.se> Date: Fri, 4 Oct 2024 10:40:41 +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 References: <20240920062437.738706-2-mattias.ronnblom@ericsson.com> <20240920104754.739033-1-mattias.ronnblom@ericsson.com> <20240920104754.739033-2-mattias.ronnblom@ericsson.com> 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-04 10:05, Robin Jarry wrote: > Mattias Rönnblom, Oct 04, 2024 at 09:31: >> On 2024-10-03 18:47, Robin Jarry wrote: >>>> +def chk_missing(filename): >>>> +    header = open(filename).read() >>> >>> with open(filename) as f: >>>     header = f.read() >>> >> >> What benefit would that construct be to this program? > > open(filename).read() leaks open file descriptors. This construct > ensures the file is closed after reading is complete. > I know what it does. I was asking why it matters in this context. But sure, I guess you could hit the per-process fd limit before the script exists. > I understand that this script isn't mission critical, but let's avoid > leaving bad examples in the code that others may follow by accident :) > >>> I know it is a simple python script but it would be better to add a >>> proper main() function and use argparse to handle errors. E.g.: >>> >>> def main(): >>>     parser = argparse.ArgumentParser(description=__doc__) >>>     parser.add_argument("op", choices=("missing", "redundant")) >>>     parser.add_argument("headers", nargs="+") >>>     args = parser.parse_args() >>> >>>     for h in args.headers: >>>         if op == "missing": >>>             chk_missing(h) >>>         elif op == "redundant": >>>             chk_redundant(h) >>> >>> if __name__ == "__main__": >>>     main() >>> >>> >> >> I don't think you need to bring in the usual Python boiler plate. This >> script is not supposed to be invoked directly by a user - it's just an >> extension of the meson build scripts. > > Same as above, I know it is not a user facing script. But I think it > would be much better to write proper python code to have good examples > for newcomers to follow. > Making small scripts needlessly complicated is not good example, it's a bad one. >>>> diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/ >>>> meson.build >>>> index f2dadcae18..762f85efe5 100644 >>>> --- a/buildtools/chkincs/meson.build >>>> +++ b/buildtools/chkincs/meson.build >>>> @@ -38,13 +38,13 @@ 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 >>>> +chkextern = find_program('chkextern.py') >>>> + >>>> +missing_extern_headers = run_command(chkextern, 'missing', >>>> dpdk_chkinc_headers, >>>> +      capture: true, check: true).stdout().split() >>>> + >>>> +if missing_extern_headers != [] >>>> +    error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n- >>>> '.join(missing_extern_headers)) >>> >>> Instead of just relying on this script output, it would be better if >>> it exited with a non-zero status when something is wrong. That way >>> you would not have to capture stdout at all and you could leave meson >>> do the work. >>> >> >> Sure, but it would be required to invoke the script for every header >> file in the tree. Not sure I think that would be a net gain. > > You can store a global exit status in the script and process all headers > before exiting with an error if any. > You will need to give the user a list of offending header files. >> Thanks for the review. >> >>>>  endif >>>> >>>>  gen_cpp_files = generator(gen_c_file_for_header, >>>> -- >>>> 2.43.0 >>> >