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 7B5E5A0C41; Wed, 6 Oct 2021 15:30:27 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6BA7641130; Wed, 6 Oct 2021 15:30:27 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mails.dpdk.org (Postfix) with ESMTP id 3017241100 for ; Wed, 6 Oct 2021 15:30:25 +0200 (CEST) X-IronPort-AV: E=McAfee;i="6200,9189,10128"; a="225882211" X-IronPort-AV: E=Sophos;i="5.85,350,1624345200"; d="scan'208";a="225882211" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Oct 2021 06:30:10 -0700 X-IronPort-AV: E=Sophos;i="5.85,350,1624345200"; d="scan'208";a="439116333" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.19.179]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 06 Oct 2021 06:30:09 -0700 Date: Wed, 6 Oct 2021 14:30:06 +0100 From: Bruce Richardson To: Sean Morrissey Cc: dev@dpdk.org, Conor Fogarty Message-ID: References: <20211004101058.2396458-1-sean.morrissey@intel.com> <20211006111329.152938-1-sean.morrissey@intel.com> <20211006111329.152938-2-sean.morrissey@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211006111329.152938-2-sean.morrissey@intel.com> Subject: Re: [dpdk-dev] [PATCH v2 1/5] devtools: script to remove unused headers includes 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 Sender: "dev" On Wed, Oct 06, 2021 at 11:13:25AM +0000, Sean Morrissey wrote: > This script can be used for removing headers flagged for removal by the > include-what-you-use (IWYU) tool. The script has the ability to remove > headers from specified sub-directories or dpdk as a whole and tests the > build after each removal by calling meson compile. > > example usages: > > Remove headers flagged by iwyu_tool output file > $ ./devtools/process_iwyu.py iwyu.out -b build > > Remove headers flagged by iwyu_tool output file from sub-directory > $ ./devtools/process_iwyu.py iwyu.out -b build -d lib/kvargs > > Remove headers directly piped from the iwyu_tool > $ iwyu_tool -p build | ./devtools/process_iwyu.py - -b build > > Signed-off-by: Sean Morrissey > Signed-off-by: Conor Fogarty > --- Some more comments inline. When those are addresses you can add my reviewed-by. Thanks, /Bruce > devtools/process_iwyu.py | 114 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 114 insertions(+) > create mode 100755 devtools/process_iwyu.py > > diff --git a/devtools/process_iwyu.py b/devtools/process_iwyu.py > new file mode 100755 > index 0000000000..c5b93c5ef3 > --- /dev/null > +++ b/devtools/process_iwyu.py > @@ -0,0 +1,114 @@ > +#!/usr/bin/env python3 > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright(c) 2021 Intel Corporation > +# > + > +import argparse > +import fileinput > +import sys > +from os.path import abspath, relpath, join > +from pathlib import Path > +from mesonbuild import mesonmain > + > + > +def args_parse(): > + "parse arguments and return the argument object back to main" > + parser = argparse.ArgumentParser( > + description="""This script can be used to remove > + includes which are not in use\n""") While in general lines should be kept under 80 characters to satisfy the flake8 script checks, literal strings are always the exception and should not be split across lines without good reason, or an explicit break in them. Most checkpatch and flake8 tools are aware of this and should not omit a warning if a literal string crosses the line-length boundary. Also, the "\n" should be unnecessary at the end of the description string. > + parser.add_argument('-b', '--build_dir', type=str, > + help="""The path to the build directory in which > + the IWYU tool was used in.""", default="build") For these items, again don't split the string, but I suggest putting all parameters bar the help text on one line, and the help text on its own on the second line i.e. move the "default" option up a line, and put "help" value all alone on second line. > + parser.add_argument('-d', '--sub_dir', type=str, > + help='The sub-directory to remove headers from.', > + default="") > + parser.add_argument('file', type=Path, > + help="""The path to the IWYU log file or output > + from stdin.""") > + > + return parser.parse_args() > + > + > +def run_meson(args): > + "Runs a meson command logging output to process.log" > + with open('process_iwyu.log', 'a') as sys.stdout: > + ret = mesonmain.run(args, abspath('meson')) > + sys.stdout = sys.__stdout__ > + return ret > + > + > +def remove_includes(filename, include, build_dir): > + "Attempts to remove include, if it fails then revert to original state" > + filepath = filename > + > + with open(filepath, 'r+') as f: Not sure you need a mode here at all, since you are just reading the file. "open(filepath) as f:" should work fine. > + lines = f.readlines() # Read lines when file is opened > + > + with open(filepath, 'w') as f: > + for ln in lines: # Removes the include passed in > + if ln.strip() != include: I'd just point out that this will fail to remove any includes which have a comment after them. Probably "startswith" is best used here instead of a straight comparison: "if not ln.startswith(include)" > + f.write(ln) > + > + # run test build -> call meson on the build folder, meson compile -C build > + ret = run_meson(['compile', '-C', build_dir]) > + if (ret == 0): # Include is not needed -> build is successful > + print('SUCCESS') > + else: > + # failed, catch the error > + # return file to original state > + with open(filepath, 'w') as f: > + f.writelines(lines) > + print('FAILED') Minor nit, I'd suggest dedenting (or de-indenting) the print one level, so that it's not part of the "with" clause, but part of the "else". [If you want to remove the else entirely, you can put a "return" after the print success.] > + > + > +def get_build_config(builddir, condition): > + "returns contents of rte_build_config.h" > + with open(join(builddir, 'rte_build_config.h')) as f: > + return [ln for ln in f.readlines() if condition(ln)] > + > + > +def uses_libbsd(builddir): > + "return whether the build uses libbsd or not" > + return bool(get_build_config(builddir, lambda ln: 'RTE_USE_LIBBSD' in ln)) > + > + > +def process(args): > + "process the iwyu output on a set of files" > + filename = None > + build_dir = abspath(args.build_dir) > + directory = args.sub_dir > + > + keep_str_fns = uses_libbsd(build_dir) # check for libbsd > + if keep_str_fns: > + print("""Warning: libbsd is present, build will fail to detect > + incorrect removal of rte_string_fns.h""", file=sys.stderr) > + # turn on werror > + run_meson(['configure', build_dir, '-Dwerror=true']) > + # Use stdin if no iwyu_tool out file given > + for line in fileinput.input(args.file): > + if 'should remove' in line: > + # If the file path in the iwyu_tool output is > + # an absolute path it means the file is outside > + # of the dpdk directory, therefore ignore it. > + # Also check to see if the file is within the > + # specified sub directory. You don't need to word-wrap the comment above quite so aggressively. Having lines a little longer would make it visually shorter and no harder to read. > + if (line.split()[0] != abspath(line.split()[0]) and > + directory in line.split()[0]): > + filename = relpath(join(build_dir, line.split()[0])) The python interpreter is going to split that line a lot of times! Use of a temporary variable would be justified I think. > + elif line.startswith('-') and filename: > + include = '#include ' + line.split()[2] > + print(f"Remove {include} from {filename} ... ", end='', flush=True) > + if keep_str_fns and '' in include: > + print('skipped') > + continue > + remove_includes(filename, include, build_dir) > + else: > + filename = None > + > + > +def main(): > + process(args_parse()) > + > + > +if __name__ == '__main__': > + main() > -- > 2.25.1 >