Re: [PATCH v7 55/59] perf Makefile: Update Python script installation path

sashiko-bot
2026-04-25T23:22:23Z

Sashiko AI review found 2 potential issue(s):
- [High] The patch changes the installation directory for Python scripts but fails to update the C code that resolves them, causing `perf script -l` to unconditionally abort and breaking script resolution.
- [Medium] The patch breaks the `make_no_scripts` and `make_minimal` test targets, resulting in a silent loss of test coverage for the Python opt-out build configuration.
--

commit 7e77c00b937b8bb6b2b6ff7f1c92cb9981141b94
Author: Ian Rogers <irogers@google.com>

perf Makefile: Update Python script installation path

This patch updates the feature test from libpython to python-module, adjusts
the corresponding variables in the build system, and changes the installation
directory for Python scripts from scripts/python to python under the perf
libexec directory.

> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
> index 2020532bab9c..e50b1e8cf85d 100644
> --- a/tools/perf/Makefile.perf
> +++ b/tools/perf/Makefile.perf
[ ... ]
> @@ -1099,10 +1097,10 @@ endif
> $(call QUIET_INSTALL, perf-iostat) \
> $(INSTALL) $(OUTPUT)perf-iostat -t '$(DESTDIR_SQ)$(perfexec_instdir_SQ)'
>
> -ifndef NO_LIBPYTHON
> +ifndef NO_PYTHON_MODULE
> $(call QUIET_INSTALL, python-scripts) \
> - $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perfexec_instdir_SQ)/scripts/python'; \
> - $(INSTALL) python/*.py -m 644 -t '$(DESTDIR_SQ)$(perfexec_instdir_SQ)/scripts/python'
> + $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perfexec_instdir_SQ)/python'; \
> + $(INSTALL) python/*.py -m 644 -t '$(DESTDIR_SQ)$(perfexec_instdir_SQ)/python'
> endif

Does changing the installation directory to python without also updating
tools/perf/builtin-script.c cause a temporary regression in this commit?

The list_available_scripts() function in tools/perf/builtin-script.c
still calls opendir() on the legacy scripts directory and explicitly calls
exit(-1) if it does not exist, which causes perf script -l to abort.

Similarly, find_script() still searches the nonexistent scripts directory,
preventing execution of the Python scripts.

I noticed this is fixed later in the series in commit
28566cda6894feea81082103f438305b884d5d12 ("perf script: Refactor to support
standalone scripts and remove legacy features").

Should each commit build and function independently without introducing
temporary regressions?

> diff --git a/tools/perf/tests/make b/tools/perf/tests/make
> index 31b064928cfc..f2c5f1c254a7 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
> make_minimal += NO_LIBBPF=1

Should NO_PYTHON_MODULE=1 be added to make_minimal to replace NO_LIBPYTHON=1?

Without it, this drops the intended minimal build coverage for Python-disabled
configurations.

[ ... ]
> @@ -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

Does leaving run += make_no_scripts here without an underlying assignment
cause the test framework to silently evaluate the variable to an empty string?

This appears to cause the test target to run a standard default make build
instead of verifying that perf builds successfully when Python module support
is disabled.

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