Re: [PATCH 7/7] perf build: install-build-deps: add RHEL family devel package mapping

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing candidate packages (like zlib and jvmti) are silently omitted from the missing packages list, violating the stated behavior.
- [Medium] AlmaLinux is incorrectly matched as `alma` instead of `almalinux` in the distro detection logic.
- [Low] The temporary file created by `mktemp` is not protected by a signal/exit trap and will leak if the script is interrupted.
--

--- Patch [7]: [PATCH 7/7] perf build: install-build-deps: add RHEL family devel package mapping ---
commit 09c82c9c3894b64680aa568b20137b9484ac96d7
Author: Arnaldo Carvalho de Melo <[email protected]>

perf build: install-build-deps: add RHEL family devel package mapping

With the Fedora mapping in place, this patch extends it to the RHEL
family (RHEL, CentOS Stream, Rocky Linux, AlmaLinux, Oracle Linux),
which shares most Fedora package names and runs dnf (RHEL 8 and
later).

> diff --git a/tools/perf/scripts/install-build-deps.sh b/tools/perf/scripts/install-build-deps.sh
> index 4a6f4bda49094..41053053a72a5 100755
> --- a/tools/perf/scripts/install-build-deps.sh
> +++ b/tools/perf/scripts/install-build-deps.sh

[ ... ]

> @@ -285,11 +380,12 @@ detect_distro() {
>  	id=$( . /etc/os-release && echo "${ID:-}" )
>  	case "$id" in
>  	fedora)			echo "fedora" ;;
> +	rhel|centos|rocky|alma|ol)

[Severity: Medium]
Will this correctly match AlmaLinux? The ID field in /etc/os-release for
AlmaLinux is usually "almalinux" rather than "alma". If it doesn't match,
the script might fall through and abort with an unsupported distro error.

> +		# All share the Fedora package names (with the exceptions
> +		# handled by rhel_pkg_for()) and dnf.
> +		echo "rhel" ;;
>  	ubuntu)			echo "ubuntu" ;;
>  	debian)			echo "debian" ;;
> -	# RHEL and its derivatives share most Fedora package names, but the
> -	# mapping is only validated on Fedora, so don't auto-detect them.
> -	rhel|centos|rocky|alma|ol) echo "" ;;
>  	*)			echo "" ;;
>  	esac
>  }

[ ... ]

> @@ -311,33 +407,73 @@ feature_tests() {
>  	for feat in $(feature_tests "$srcdir"); do
> -		if [ "$distro" = "fedora" ]; then
> -			pkg=$(fedora_pkg_for "$feat")
> -		else
> -			pkg=$(debian_pkg_for "$feat")
> -		fi
> +		case "$distro" in
> +		fedora)		pkg=$(fedora_pkg_for "$feat") ;;
> +		rhel)		pkg=$(rhel_pkg_for "$feat") ;;
> +		*)		pkg=$(debian_pkg_for "$feat") ;;
> +		esac
>  		[ -n "$pkg" ] || continue

[Severity: Medium]
If pick_available() in rhel_pkg_for() fails to find any of the candidate
packages (like zlib-devel or zlib-ng-compat-devel), it returns an empty
string.

Since this check skips empty strings, does this bypass the missing package
tracking entirely for those features, preventing them from being reported
to the user as missing?

>  		for pkg in $pkg; do
> +			if [ "$distro" = "rhel" ] && ! pkg_available "$pkg"; then
> +				missing_pkgs="$missing_pkgs $pkg"
> +				continue
> +			fi
>  			case " $pkgs " in
>  			*" $pkg "*) ;;
>  			*) pkgs="$pkgs $pkg" ;;
>  			esac
>  		done
>  	done

[ ... ]

> @@ -377,23 +513,31 @@ main() {
>  	srcdir=$(cd "$(dirname "$0")/../../.." && pwd)
>  	distro=$(detect_distro)
>  	case "$distro" in
> -	fedora|ubuntu|debian) ;;
> +	fedora|rhel|ubuntu|debian) ;;
>  	*)
>  		echo "error: unsupported distro (got '$distro'); the package mapping is not validated on other distros." >&2
> -		echo "Supported and validated: Fedora 44 (toolbx container), Ubuntu 26.04 and Debian 13 (distrobox containers)." >&2
> +		echo "Supported and validated: Fedora 44 (toolbx container), Ubuntu 26.04 and Debian 13 (distrobox containers), and CentOS Stream 10 (distrobox container)." >&2
>  		exit 1
>  		;;
>  	esac
>  
> -	pkgs=$(package_set "$distro" "$srcdir")
> +	missing_file=$(mktemp) || {
> +		echo "error: cannot create a temporary file (mktemp failed)" >&2
> +		exit 1
> +	}

[Severity: Low]
If the script is interrupted (for example, via Ctrl-C) while running the
dnf repoquery loop, will this temporary file leak in /tmp? Should there
be a trap, similar to trap 'rm -f "$missing_file"' EXIT INT TERM, to ensure
it gets cleaned up if the script terminates early?

> +	pkgs=$(package_set "$distro" "$srcdir" 2>"$missing_file")
> +	missing_pkgs=$(cat "$missing_file")
> +	rm -f "$missing_file"

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