Re: [PATCH v2] perf python: Clean up and restructure setup.py

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Unresolved reference to `src_perf` in `InstallLib.run()` will raise a `NameError` during the installation phase.
- [Low] Naive splitting of compiler flags (`CFLAGS` and `OPT`) using `.split()` breaks quoted arguments and redundantly duplicates user flags.
--

--- Patch [1]: [PATCH v2] perf python: Clean up and restructure setup.py ---
commit 48e3810550a434c966bd3941fa49269f6b80e25e
Author: Ian Rogers <irogers@google.com>

perf python: Clean up and restructure setup.py

This commit cleans up and restructures the python setup script to resolve
pylint warnings and improve code quality. It moves global logic into a
main() function and extracts compiler flag filtering into a helper.

> diff --git a/tools/perf/util/setup.py b/tools/perf/util/setup.py
> index cb14554454a21..1385f07424b7c 100644
> --- a/tools/perf/util/setup.py
> +++ b/tools/perf/util/setup.py
[ ... ]
> +def filter_clang_options(cc: str, cc_args: list[str], src_feature_tests: str) -> None:
> +    """Filter out unsupported clang options from sysconfig CFLAGS and OPT.
> +
> +    Args:
> +        cc: The compiler executable.
> +        cc_args: Compiler arguments from CC environment variable.
> +        src_feature_tests: Path to the feature tests directory.
> +    """
> +    config_vars = sysconfig.get_config_vars()
>      for var in ('CFLAGS', 'OPT'):
> -        vars[var] = sub("-specs=[^ ]+", "", vars[var])
> +        if var not in config_vars:
> +            continue
> +
> +        # Split into individual flags to avoid regex boundary issues
> +        flags = config_vars[var].split()

[Severity: Low]
Does using a simple string split here break quoted compiler flags?

If the system Python configuration provides CFLAGS or OPT with quoted
arguments (e.g., -D"MACRO='some string'"), it appears this will destructively
split them on spaces into invalid fragments.

Should this use a quote-aware parser like shlex.split() instead?

[ ... ]
> +class InstallLib(_install_lib):
> +    """Custom install_lib command to set output directory."""
>  
> -class install_lib(_install_lib):
> -    def finalize_options(self):
> +    def __init__(self, *args, **kwargs):
> +        self.build_dir = None
> +        super().__init__(*args, **kwargs)
> +
> +    def finalize_options(self) -> None:
>          _install_lib.finalize_options(self)
> -        self.build_dir = build_lib
> +        build_lib = os.getenv('PYTHON_EXTBUILD_LIB')
> +        if build_lib:
> +            self.build_dir = build_lib
>  
>      def run(self):
>          _install_lib.run(self)
>          shutil.copy2(f'{src_perf}/python/perf.pyi', self.install_dir)

[Severity: Low]
Will this raise a NameError exception during installation?

The src_perf variable was refactored out of the module scope and moved into
the main() local scope, making it undefined here in InstallLib.run().

[ ... ]
> +def main() -> None:
[ ... ]
> +    if cc_is_clang:
> +        filter_clang_options(cc, cc_args, src_feature_tests)
> +
> +    cflags = os.getenv('CFLAGS', '').split()

[Severity: Low]
Similarly to the sysconfig variables, does this naive split break quoted
arguments in the CFLAGS environment variable?

Also, since setuptools inherently parses and applies CFLAGS from the
environment during extension compilation, will manually appending this to
extra_compile_args duplicate the user flags?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723164421.1232688-1-irogers@google.com?part=1