Re: [PATCH 2/7] perf build: Add install-build-deps framework to install devel packages

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] The `--distro` option's error message suggests `rhel` and `debian` as valid choices, but the script then rejects them as unsupported.
- [Low] The local variable `pkg` is declared but never used.
- [Low] Sourcing `/etc/os-release` without existence checks or stderr redirection leaks error messages on unsupported systems.
--

--- Patch [2]: [PATCH 2/7] perf build: Add install-build-deps framework to install devel packages ---
commit f7444376f9bbb9f83dcea58b5ac3f7286c4455dd
Author: Arnaldo Carvalho de Melo <[email protected]>

perf build: Add install-build-deps framework to install devel packages

This patch adds a 'make -C tools/perf install-build-deps' target and a
companion script to automatically install the development packages needed
to build perf. The script includes distro detection and handles the base
toolchain packages for Fedora and Ubuntu.

> diff --git a/tools/perf/scripts/install-build-deps.sh b/tools/perf/scripts/install-build-deps.sh
> new file mode 100755
> index 0000000000000..6b9a1142f32ac
> --- /dev/null
> +++ b/tools/perf/scripts/install-build-deps.sh

[ ... ]

> +detect_distro() {
> +	if [ -n "$DISTRO" ]; then
> +		echo "$DISTRO"
> +		return
> +	fi
> +	local id
> +	id=$( . /etc/os-release && echo "${ID:-}" )

[Severity: Low]
Is it possible for a minimal container to lack /etc/os-release? 

If so, sourcing it without an existence check or stderr redirection will
print a "No such file or directory" error to the terminal. Does this
contradict the commit message's goal of avoiding error spew in fresh
containers?

[ ... ]

> +main() {
> +	local action="install"
> +	local srcdir distro pkgs pkg cmd

[Severity: Low]
This isn't a bug, but it looks like the local variable pkg is declared
here but never used in the main() function.

> +
> +	while [ $# -gt 0 ]; do
> +		case "$1" in
> +		--list)		action="list"; shift ;;
> +		--dry-run)	action="dry-run"; shift ;;
> +		--distro)
> +			[ $# -ge 2 ] || {
> +				echo "error: --distro requires an argument (fedora, rhel, ubuntu, debian)" >&2

[Severity: Low]
This error message suggests "rhel" and "debian" as valid options, but the
distro validation block later in main() explicitly rejects them as
unsupported. 

Could this error message be updated to only list the distros that are
actually supported by the script?

> +				exit 1
> +			}
> +			DISTRO="$2"; shift 2 ;;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2