Re: [dpdk-dev] [PATCH v2 1/5] devtools: script to remove unused headers includes

[Bruce Richardson <bruce.richardson@intel.com>]

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 <sean.morrissey@intel.com>
> Signed-off-by: Conor Fogarty <conor.fogarty@intel.com>
> ---

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 '<rte_string_fns.h>' 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
>