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 46454A0C4D; Mon, 4 Oct 2021 17:26:57 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A254941236; Mon, 4 Oct 2021 17:26:56 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mails.dpdk.org (Postfix) with ESMTP id A779F40E3C for ; Mon, 4 Oct 2021 17:26:54 +0200 (CEST) X-IronPort-AV: E=McAfee;i="6200,9189,10127"; a="248729629" X-IronPort-AV: E=Sophos;i="5.85,346,1624345200"; d="scan'208";a="248729629" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Oct 2021 08:07:57 -0700 X-IronPort-AV: E=Sophos;i="5.85,346,1624345200"; d="scan'208";a="713446915" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.10.232]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 04 Oct 2021 08:07:56 -0700 Date: Mon, 4 Oct 2021 16:07:51 +0100 From: Bruce Richardson To: Sean Morrissey Cc: dev@dpdk.org, Conor Fogarty Message-ID: References: <20211004101058.2396458-1-sean.morrissey@intel.com> <20211004101058.2396458-2-sean.morrissey@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211004101058.2396458-2-sean.morrissey@intel.com> Subject: Re: [dpdk-dev] [PATCH v1 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 Mon, Oct 04, 2021 at 10:10:54AM +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. > Since it also is importing meson and calling "meson compile" it appears to be testing the build after each removal too. I think this should be called out, to make it clear it's not a "blind" removal of headers. Further review comments inline below. /Bruce > 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 > --- > devtools/process_iwyu.py | 109 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 109 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..ddc4ceafa4 > --- /dev/null > +++ b/devtools/process_iwyu.py > @@ -0,0 +1,109 @@ > +#!/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(): > + parser = argparse.ArgumentParser(description='This script can be used to remove includes which are not in use\n') > + parser.add_argument('-b', '--build_dir', type=str, help='Name of the build directory in which the IWYU tool was used in.', default="build") > + 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.') > + These lines are all very long. While the text strings shouldn't be split across lines, you can break the line across multiple ones between parameters. I suggest checking this whole script using "flake8" to check for style errors. > + args = parser.parse_args() > + > + return args > + "args" is unneeded here. "return parse.parse_args()" is shorter. :-) > + > +def run_meson(args): > + "Runs a meson command logging output to process.log" > + with open('process.log', 'a') as sys.stdout: > + ret = mesonmain.run(args, abspath('meson')) > + sys.stdout = sys.__stdout__ > + return ret > + I think process.log should be renamed to "process_iwyu.log" to match the script name. Also, it's nice to see a few functions like this with a docstring at the start. It would be good to have a one-line summary at the start of every fn in this file. > + > +def remove_includes(filename, include, dpdk_dir, build_dir): > + # Load in file - readlines() > + # loop through list once in mem -> make cpy of list with line removed > + # write cpy -> stored in memory so write cpy to file then check > + # run test build -> call ninja on the build folder, ninja -C build, subprocess You actually call "meson compile" rather than ninja -C build. Please make sure the comments match the code as the code is reworked. If you take the approach of adding a one-line string at the start of each function, I'd suggest splitting up this comment into smaller comments spread throughout the code, explaining each short block as it appears. [Though I see it's reasonably well commented below as-is] > + # if fails -> write original back to file otherwise continue on > + # newlist = [ln for ln in lines if not ln.startswith(...)] filters out one element > + filepath = filename > + > + with open(filepath, 'r+') as f: > + 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("\n") != include: Strip without any parameters removes all whitespace, which is probably ok here, so drop the explicit "\n". > + f.write(ln) > + > + ret = run_meson(['compile', '-C', join(dpdk_dir, 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') > + > + > +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): > + filename = None > + build_dir = args.build_dir > + dpdk_dir = abspath(__file__).split('/devtools')[0] This assumes that we are using the script from the devtools dir of the copy of DPDK we want to edit. That's probably a fair enough assumption and good enough, though I'd like more input on it. The other alternative is to work of the build dir, which will have the paths to the DPDK source code directory in it. Running "meson configure" in the build dir will print out the source directory location at the start: $ meson configure | head Core properties: Source dir /home/bruce/dpdk.org Build dir /home/bruce/dpdk.org/build > + directory = args.sub_dir > + # Use stdin if no iwyu_tool out file given > + logfile = abspath(args.file) if str(args.file) != '-' else args.file Not sure this line is necessary. Assuming that we don't chdir anywhere, using args.file directly below in call to "fileinput.input" should work perfectly. > + > + keep_str_fns = uses_libbsd(join(dpdk_dir, 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) > + run_meson(['configure', dpdk_dir + "/" + build_dir, '-Dwerror=true']) # turn on werror > + > + for line in fileinput.input(logfile): > + 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 > + if line.split()[0] != abspath(line.split()[0]) and directory in line.split()[0]: > + filename = relpath(join(build_dir, line.split()[0])) > + 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, dpdk_dir, build_dir) > + else: > + filename = None > + > + > +def main(): > + args = args_parse() > + process(args) > + Nit: again, args variable is unnecessary, and you can shorten to single line if you like "process(args_parse())" > + > +if __name__ == '__main__': > + main() > -- > 2.25.1 >