Re: [PATCH v5 55/58] perf Makefile: Update Python script installation path

sashiko-bot
2026-04-24T18:21:58Z

Sashiko AI review found 3 potential issue(s):
- [High] The interactive UI script browser is completely broken for Python scripts due to obsolete macros and paths.
- [Medium] Test suite silently loses coverage for the 'disable python module' configuration.
- [Medium] The `python-module` feature test incorrectly requires the Python embedding library, unnecessarily failing on some systems.
--

commit 00f5f415c2dee014c40385386d7e52b97edea230
Author: Ian Rogers <irogers@google.com>

perf Makefile: Update Python script installation path

Remove libpython feature test that is now a python-module feature test.
Update references from libpython to python-module accordingly.
Remove references to legacy 'scripts/python' directory and install
scripts directly to 'python' directory under libexec.

> Remove references to legacy 'scripts/python' directory and install
> scripts directly to 'python' directory under libexec. This aligns with
> the removal of embedded interpreter and move to standalone
> scripts.

Was tools/perf/ui/browsers/scripts.c missed in this update? The
find_scripts() function still has an #ifndef HAVE_LIBPYTHON_SUPPORT check
that unconditionally skips the Python directory because
HAVE_LIBPYTHON_SUPPORT was removed.

It also still searches the legacy scripts/python path instead of the new
python/ directory:

tools/perf/ui/browsers/scripts.c:find_scripts() {
...
if (!strcmp(lang_dirent->d_name, ".") || !strcmp(lang_dirent->d_name, ".."))
continue;

#ifndef HAVE_LIBPYTHON_SUPPORT
if (strstr(lang_dirent->d_name, "python"))
continue;
#endif
...
scnprintf(scripts_path_array[i], pathlen, "%s/scripts/%s/%s",
exec_path,
lang_dirent->d_name,
script_dirent->d_name);
...
}

Will this cause the UI script browser to completely break for Python scripts?

> diff --git a/tools/build/feature/test-python-module.c b/tools/build/feature/test-python-module.c
> new file mode 100644
> index 00000000000000..d670dba014b001
> --- /dev/null
> +++ b/tools/build/feature/test-python-module.c
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <Python.h>
> +
> +int main(void)
> +{
> + static struct PyModuleDef moduledef = {
> + PyModuleDef_HEAD_INIT,
> + };
> + PyObject *module = PyModule_Create(&moduledef);
> +
> + return module ? 0 : -1;
> +}

Because this test is compiled as a standalone executable, it forces the build
system to link against libpython.

Since the embedded interpreter was removed and perf now only builds a Python
extension module, does this incorrectly require the Python embedding library?

Systems that have the Python headers but lack the static/dynamic embedding
library will fail this feature test, incorrectly disabling Python support
across the build.

> diff --git a/tools/perf/tests/make b/tools/perf/tests/make
> index 31b064928cfca0..f2c5f1c254a7cc 100644
> --- a/tools/perf/tests/make
> +++ b/tools/perf/tests/make
> @@ -75,8 +75,6 @@ make_jevents_all := JEVENTS_ARCH=all
> make_no_bpf_skel := BUILD_BPF_SKEL=0
> make_gen_vmlinux_h := GEN_VMLINUX_H=1
>
> -make_no_libpython := NO_LIBPYTHON=1
> -make_no_scripts := NO_LIBPYTHON=1
> make_no_slang := NO_SLANG=1
> make_no_gtk2 := NO_GTK2=1
> make_no_ui := NO_SLANG=1 NO_GTK2=1

[ ... ]

> @@ -118,7 +116,7 @@ make_install_prefix_slash := install prefix=/tmp/krava/
> make_static := LDFLAGS=-static NO_PERF_READ_VDSO32=1 NO_PERF_READ_VDSOX32=1 NO_JVMTI=1 NO_LIBTRACEEVENT=1 NO_LIBELF=1
>
> # all the NO_* variable combined
> -make_minimal := NO_LIBPYTHON=1 NO_GTK2=1
> +make_minimal := NO_GTK2=1
> make_minimal += NO_DEMANGLE=1 NO_LIBELF=1 NO_BACKTRACE=1
> make_minimal += NO_LIBNUMA=1 NO_LIBBIONIC=1 NO_LIBDW=1

Should NO_PYTHON_MODULE=1 be added to make_minimal here since
NO_LIBPYTHON=1 was removed?

[ ... ]

> @@ -150,7 +148,6 @@ run += make_jevents_all
> run += make_no_bpf_skel
> run += make_gen_vmlinux_h
>
> -run += make_no_libpython
> run += make_no_scripts
> run += make_no_slang
> run += make_no_gtk2

Is it expected that make_no_scripts is still in the run list even though
its definition was removed?

Because the variable is now undefined, it evaluates to an empty string. This
causes the test suite to execute a redundant default build instead of
verifying the NO_PYTHON_MODULE=1 opt-out path, leading to a silent drop
in CI test coverage.

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