[gentoo-dev] [PATCH v2 0/2] rocm.eclass: new eclass for ROCm packages

[gentoo-dev] [PATCH v2 0/2] rocm.eclass: new eclass for ROCm packages

[Yiyang Wu]

From: YiyangWu <xgreenlandforwyy@gmail.com>

This update fixes problems pointed out last week on rocm.eclass.

Changelog from v1:
1. Change to the mordern EAPI check style;
2. fix typo in amdgpu_targets.desc;
3. remove trailing space in comments

Yiyang Wu (2):
  rocm.eclass: new eclass
  profiles/desc: add amdgpu_targets.desc for USE_EXPAND

 eclass/rocm.eclass                | 275 ++++++++++++++++++++++++++++++
 profiles/base/make.defaults       |   2 +-
 profiles/desc/amdgpu_targets.desc |  15 ++
 3 files changed, 291 insertions(+), 1 deletion(-)
 create mode 100644 eclass/rocm.eclass
 create mode 100644 profiles/desc/amdgpu_targets.desc

Interdiff against v1:
diff --git a/eclass/rocm.eclass b/eclass/rocm.eclass
index e4b448f7b894..8ca2c051ddce 100644
--- a/eclass/rocm.eclass
+++ b/eclass/rocm.eclass
@@ -19,7 +19,7 @@
 # Most ROCm packages use cmake as build system, so this eclass does not export
 # phase functions which overwrites the phase functions in cmake.eclass. Ebuild
 # should explicitly call rocm_src_* in src_configure and src_test.
-# 
+#
 # @EXAMPLE:
 # # Example for ROCm packages in https://github.com/ROCmSoftwarePlatform
 # inherit cmake rocm
@@ -28,25 +28,25 @@
 # IUSE="test"
 # REQUIRED_USE="${ROCM_REQUIRED_USE}"
 # RESTRICT="!test? ( test )"
-#  
+#
 # RDEPEND="
 #     dev-util/hip
 #     sci-libs/rocBLAS:${SLOT}[${ROCM_USEDEP}]
 # "
-# 
+#
 # S=${WORKDIR}/${PN}-rocm-${PV}
-#  
+#
 # src_configure() {
 #     local mycmakeargs=(
 #         -DBUILD_CLIENTS_TESTS=$(usex test ON OFF)
 #     )
 #     rocm_src_configure
 # }
-# 
+#
 # src_test() {
 #     rocm_src_test
 # }
-# 
+#
 # # Example for packages depend on ROCm libraries -- a package depend on
 # # rocBLAS, and use comma seperated ${HCC_AMDGPU_TARGET} to determine GPU
 # # architecture to compile. Requires ROCm version >5.
@@ -67,14 +67,11 @@
 
 if [[ ! ${_ROCM_ECLASS} ]]; then
 
-case ${EAPI} in
-	0|1|2|3|4|5|6)
-		die "${ECLASS}: unsupported EAPI=${EAPI:-0} (too old)"
-		;;
+case "${EAPI:-0}" in
 	7|8)
 		;;
 	*)
-		die "${ECLASS}: unsupported EAPI=${EAPI} (unknown)"
+		die "Unsupported EAPI=${EAPI} for ${ECLASS}"
 		;;
 esac
 
diff --git a/profiles/desc/amdgpu_targets.desc b/profiles/desc/amdgpu_targets.desc
index 942ab51356fb..8a3db2b56dab 100644
--- a/profiles/desc/amdgpu_targets.desc
+++ b/profiles/desc/amdgpu_targets.desc
@@ -8,7 +8,7 @@ gfx900 - Vega GPU, codename vega10, including Radeon Vega Frontier Edition, Rade
 gfx906 - Vega GPU, codename vega20, including Radeon (Pro) VII, Radeon Instinct MI50/MI60
 gfx908 - CDNA Accelerator, codename arcturus, including AMD Instinct MI100 Accelerator
 gfx90a - CDNA2 Accelerator, codename aldebaran, including AMD Instinct MI200 series Accelerators
-gfx1010 - RDNA GPU, codename navi10, including Radeon RX 5700XT/5700/5700M/5700B/5700XTB/5600XT/5600/5600M, Radeon Pro 5700Xt/5700, Radeon Pro W5700X/W5700
+gfx1010 - RDNA GPU, codename navi10, including Radeon RX 5700XT/5700/5700M/5700B/5700XTB/5600XT/5600/5600M, Radeon Pro 5700XT/5700, Radeon Pro W5700X/W5700
 gfx1011 - RDNA GPU, codename navi12, including Radeon Pro 5600M/V520
 gfx1012 - RDNA GPU, codename navi14, including Radeon RX 5500XT/5500/5500M/5500XTB/5300/5300M, Radeon Pro 5500XT/5500M/5300/5300M, Radeon Pro W5500X/W5500/W5500M/W5300M
 gfx1030 - RDNA2 GPU, codename navi21/sienna cichlid, including Radeon RX 6950XT/6900XT/6800XT/6800, Radeon Pro W6800
-- 
2.34.1

[gentoo-dev] [PATCH v2 1/2] rocm.eclass: new eclass

[Yiyang Wu]

This eclass provides utilities for ROCm libraries in
https://github.com/ROCmSoftwarePlatform, e.g. rocBLAS, rocFFT.
It contains a USE_EXPAND, amdgpu_targets_*, which handles the GPU
architecture to compile, and keep targets coherent among dependencies.
Packages that depend on ROCm libraries, like cupy, can also make use of
this eclass, mainly specify GPU architecture and it's corresponding
dependencies via USE_EXPAND.

Closes: https://bugs.gentoo.org/810619
Signed-off-by: YiyangWu <xgreenlandforwyy@gmail.com>
---
 eclass/rocm.eclass          | 275 ++++++++++++++++++++++++++++++++++++
 profiles/base/make.defaults |   2 +-
 2 files changed, 276 insertions(+), 1 deletion(-)
 create mode 100644 eclass/rocm.eclass

diff --git a/eclass/rocm.eclass b/eclass/rocm.eclass
new file mode 100644
index 000000000000..8ca2c051ddce
--- /dev/null
+++ b/eclass/rocm.eclass
@@ -0,0 +1,275 @@
+# Copyright 2022 Gentoo Authors
+# Distributed under the terms of the GNU General Public License v2
+
+# @ECLASS: rocm.eclass
+# @MAINTAINER:
+# Gentoo Science Project <sci@gentoo.org>
+# @AUTHOR:
+# Yiyang Wu <xgreenlandforwyy@gmail.com>
+# @SUPPORTED_EAPIS: 7 8
+# @BLURB: Common functions and variables for ROCm packages written in HIP
+# @DESCRIPTION:
+# ROCm packages such as sci-libs/<roc|hip>* can utilize functions in this eclass.
+# Currently, it handles the AMDGPU_TARGETS variable via USE_EXPAND, so user can
+# use USE flag to control which GPU architecture to compile, and ensure coherence
+# among dependencies. It also specify CXX=hipcc, to let hipcc compile. Another
+# important function is src_test, which checks whether a valid KFD device exists
+# for testing, and then execute the test program.
+#
+# Most ROCm packages use cmake as build system, so this eclass does not export
+# phase functions which overwrites the phase functions in cmake.eclass. Ebuild
+# should explicitly call rocm_src_* in src_configure and src_test.
+#
+# @EXAMPLE:
+# # Example for ROCm packages in https://github.com/ROCmSoftwarePlatform
+# inherit cmake rocm
+# SRC_URI="https://github.com/ROCmSoftwarePlatform/${PN}/archive/rocm-${PV}.tar.gz -> ${P}.tar.gz"
+# SLOT="0/$(ver_cut 1-2)"
+# IUSE="test"
+# REQUIRED_USE="${ROCM_REQUIRED_USE}"
+# RESTRICT="!test? ( test )"
+#
+# RDEPEND="
+#     dev-util/hip
+#     sci-libs/rocBLAS:${SLOT}[${ROCM_USEDEP}]
+# "
+#
+# S=${WORKDIR}/${PN}-rocm-${PV}
+#
+# src_configure() {
+#     local mycmakeargs=(
+#         -DBUILD_CLIENTS_TESTS=$(usex test ON OFF)
+#     )
+#     rocm_src_configure
+# }
+#
+# src_test() {
+#     rocm_src_test
+# }
+#
+# # Example for packages depend on ROCm libraries -- a package depend on
+# # rocBLAS, and use comma seperated ${HCC_AMDGPU_TARGET} to determine GPU
+# # architecture to compile. Requires ROCm version >5.
+# ROCM_VERSION=5
+# inherit rocm
+# IUSE="rocm"
+# REQUIRED_USE="rocm? ( ${ROCM_REQUIRED_USE} )"
+# DEPEND="rocm? ( >=dev-util/hip-${ROCM_VERSION}
+#     >=sci-libs/rocBLAS-${ROCM_VERSION}[${ROCM_USEDEP}] )"
+# ....
+# src_configure() {
+#     if use rocm; then
+#         local AMDGPU_FLAGS=$(get_amdgpu_flags)
+#         export HCC_AMDGPU_TARGET=${AMDGPU_FLAGS//;/,}
+#     fi
+#     default
+# }
+
+if [[ ! ${_ROCM_ECLASS} ]]; then
+
+case "${EAPI:-0}" in
+	7|8)
+		;;
+	*)
+		die "Unsupported EAPI=${EAPI} for ${ECLASS}"
+		;;
+esac
+
+inherit edo
+
+
+# @ECLASS_VARIABLE: ROCM_VERSION
+# @DEFAULT_UNSET
+# @PRE_INHERIT
+# @DESCRIPTION:
+# The ROCm version of current package. Default is ${PV}, but for other packages
+# that depend on ROCm libraries, this can be set to match the version of
+# required ROCm libraries.
+
+# @ECLASS_VARIABLE: ALL_AMDGPU_TARGETS
+# @INTERNAL
+# @DESCRIPTION:
+# The list of USE flags corresponding to all AMDGPU targets in this ROCm
+# version. The value depends on ${PV}. Architectures and devices map:
+# https://www.coelacanth-dream.com/posts/2019/12/30/did-rid-product-matome-p2
+
+# @ECLASS_VARIABLE: OFFICIAL_AMDGPU_TARGETS
+# @INTERNAL
+# @DESCRIPTION:
+# The list of USE flags corresponding to all officlially supported AMDGPU
+# targets in this ROCm version, documented at
+# https://docs.amd.com/bundle/ROCm-Installation-Guide-v${PV}/page/Prerequisite_Actions.html.
+# USE flag of these architectures will be default on. Depends on ${PV}.
+
+# @ECLASS_VARIABLE: ROCM_REQUIRED_USE
+# @OUTPUT_VARIABLE
+# @DESCRIPTION:
+# Requires at least one AMDGPU target to be compiled.
+# Example use for ROCm libraries:
+# REQUIRED_USE="${ROCM_REQUIRED_USE}"
+# Example use for packages that depend on ROCm libraries
+# IUSE="rocm"
+# REQUIRED_USE="rocm? ( ${ROCM_REQUIRED_USE} )"
+
+# @ECLASS_VARIABLE: ROCM_USEDEP
+# @OUTPUT_VARIABLE
+# @DESCRIPTION:
+# This is an eclass-generated USE-dependency string which can be used to
+# depend on another ROCm package being built for the same AMDGPU architecture.
+#
+# The generated USE-flag list is compatible with packages using rocm.eclass.
+#
+# Example use:
+# @CODE
+# DEPEND="sci-libs/rocBLAS[${ROCM_USEDEP}]"
+# @CODE
+
+# @FUNCTION: _rocm_set_globals
+# @DESCRIPTION:
+# Set global variables used by the eclass: ALL_AMDGPU_TARGETS,
+# OFFICIAL_AMDGPU_TARGETS, ROCM_REQUIRED_USE, and ROCM_USEDEP
+_rocm_set_globals() {
+	case ${ROCM_VERSION:-${PV}} in
+		4*)
+			ALL_AMDGPU_TARGETS=(
+				gfx803 gfx900 gfx906 gfx908
+				gfx1010 gfx1011 gfx1012 gfx1030
+			)
+			OFFICIAL_AMDGPU_TARGETS=(
+				gfx906 gfx908
+			)
+			;;
+		5*)
+			ALL_AMDGPU_TARGETS=(
+				gfx803 gfx900 gfx906 gfx908 gfx90a
+				gfx1010 gfx1011 gfx1012 gfx1030 gfx1031
+			)
+			OFFICIAL_AMDGPU_TARGETS=(
+				gfx906 gfx908 gfx90a gfx1030
+			)
+			;;
+		*)
+			die "Unknown ROCm major version! Please update rocm.eclass before bumping to new ebuilds"
+			;;
+	esac
+
+	ROCM_REQUIRED_USE+=" || ("
+	for gpu_target in ${ALL_AMDGPU_TARGETS[@]}; do
+		if [[ " ${OFFICIAL_AMDGPU_TARGETS[*]} " =~ " ${gpu_target} " ]]; then
+			IUSE+=" ${gpu_target/#/+amdgpu_targets_}"
+		else
+			IUSE+=" ${gpu_target/#/amdgpu_targets_}"
+		fi
+		ROCM_REQUIRED_USE+=" ${gpu_target/#/amdgpu_targets_}"
+	done
+	ROCM_REQUIRED_USE+=" ) "
+
+	local flags=( "${ALL_AMDGPU_TARGETS[@]/#/amdgpu_targets_}" )
+	local optflags=${flags[@]/%/(-)?}
+	ROCM_USEDEP=${optflags// /,}
+}
+_rocm_set_globals
+unset -f _rocm_set_globals
+
+
+# @FUNCTION: get_amdgpu_flags
+# @USAGE: get_amdgpu_flags
+# @DESCRIPTION:
+# Convert specified use flag of amdgpu_targets to compilation flags.
+# Append default target feature to GPU arch. See
+# https://llvm.org/docs/AMDGPUUsage.html#target-features
+get_amdgpu_flags() {
+	local AMDGPU_TARGET_FLAGS
+	for gpu_target in ${ALL_AMDGPU_TARGETS[@]}; do
+		local target_feature=
+		if use amdgpu_targets_${gpu_target}; then
+			case ${gpu_target} in
+				gfx906|gfx908)
+					target_feature=:xnack-
+					;;
+				gfx90a)
+					target_feature=:xnack+
+					;;
+				*)
+					;;
+			esac
+			AMDGPU_TARGET_FLAGS+="${gpu_target}${target_feature};"
+		fi
+	done
+	echo ${AMDGPU_TARGET_FLAGS}
+}
+
+# @FUNCTION: check_rw_permission
+# @USAGE: check_rw_permission <file>
+# @DESCRIPTION:
+# check read and write permissions on specific files.
+# allow using wildcard, for example check_rw_permission /dev/dri/render*
+check_rw_permission() {
+	[[ -r "$1" ]] && [[ -w "$1" ]] || die \
+		"${PORTAGE_USERNAME} do not have read or write permissions on $1! \n Make sure ${PORTAGE_USERNAME} is in render group and check the permissions."
+}
+
+# == phase functions ==
+
+# @FUNCTION: rocm_src_configure
+# @DESCRIPTION:
+# configure rocm packages, and setting common cmake arguments
+rocm_src_configure() {
+	# allow acces to hardware
+	addpredict /dev/kfd
+	addpredict /dev/dri/
+
+	mycmakeargs+=(
+		-DCMAKE_INSTALL_PREFIX="${EPREFIX}/usr"
+		-DAMDGPU_TARGETS="$(get_amdgpu_flags)"
+		-DCMAKE_SKIP_RPATH=TRUE
+	)
+
+	CXX="hipcc" cmake_src_configure
+}
+
+# @FUNCTION: rocm_src_test
+# @DESCRIPTION:
+# Test whether valid GPU device is present. If so, find how to, and execute test.
+# ROCm packages can have to test mechanism:
+# 1. cmake_src_test. Set MAKEOPTS="-j1" to make sure only one test on GPU at a time;
+# 2. one single gtest binary called "${PN,,}"-test;
+# 3. Some package like rocFFT have alternative test like rocfft-selftest;
+# 4. Custome testing binaries like dev-libs/rccl. Use ${ROCM_TESTS} to specify.
+rocm_src_test() {
+	addwrite /dev/kfd
+	addwrite /dev/dri/
+
+	# check permissions on /dev/kfd and /dev/dri/render*
+	check_rw_permission /dev/kfd
+	check_rw_permission /dev/dri/render*
+
+	: ${LD_LIBRARY_PATH:="${BUILD_DIR}/clients:${BUILD_DIR}/src:${BUILD_DIR}/library:${BUILD_DIR}/library/src:${BUILD_DIR}/library/src/device"}
+	export LD_LIBRARY_PATH
+	if grep -q 'build test:' "${BUILD_DIR}"/build.ninja; then
+		MAKEOPTS="-j1" cmake_src_test
+	elif [[ -d "${BUILD_DIR}"/clients/staging ]]; then
+		cd "${BUILD_DIR}/clients/staging" || die "Test directory not found!"
+		for test_program in "${PN,,}-"*test; do
+			if [[ -x ${test_program} ]]; then
+					edob ./${test_program}
+			else
+				die "The test program ${test_program} does not exist or cannot be excuted!"
+			fi
+		done
+	elif [[ ! -z "${ROCM_TESTS}" ]]; then
+		for test_program in "${ROCM_TESTS}"; do
+			cd "${BUILD_DIR}" || die
+			if [[ -x ${test_program} ]]; then
+			edob ./${test_program}
+			else
+				die "The test program ${test_program} does not exist or cannot be excuted!"
+			fi
+		done
+	else
+		die "There is no cmake tests, no \${ROCM_TESTS} executable provided, nor ${BUILD_DIR}/clients/staging where test program might be located."
+	fi
+}
+
+_ROCM_ECLASS=1
+fi
diff --git a/profiles/base/make.defaults b/profiles/base/make.defaults
index 326cb28de537..2c288d12d103 100644
--- a/profiles/base/make.defaults
+++ b/profiles/base/make.defaults
@@ -13,7 +13,7 @@ USE_EXPAND_VALUES_USERLAND="BSD GNU"
 
 # Env vars to expand into USE vars.  Modifying this requires prior
 # discussion on gentoo-dev@lists.gentoo.org.
-USE_EXPAND="ABI_MIPS ABI_S390 ABI_X86 ADA_TARGET ALSA_CARDS APACHE2_MODULES APACHE2_MPMS CALLIGRA_FEATURES CAMERAS COLLECTD_PLUGINS CPU_FLAGS_ARM CPU_FLAGS_PPC CPU_FLAGS_X86 CURL_SSL ELIBC FFTOOLS GPSD_PROTOCOLS GRUB_PLATFORMS INPUT_DEVICES KERNEL L10N LCD_DEVICES LIBREOFFICE_EXTENSIONS LLVM_TARGETS LUA_SINGLE_TARGET LUA_TARGETS MONKEYD_PLUGINS NGINX_MODULES_HTTP NGINX_MODULES_MAIL NGINX_MODULES_STREAM OFFICE_IMPLEMENTATION OPENMPI_FABRICS OPENMPI_OFED_FEATURES OPENMPI_RM PHP_TARGETS POSTGRES_TARGETS PYTHON_SINGLE_TARGET PYTHON_TARGETS QEMU_SOFTMMU_TARGETS QEMU_USER_TARGETS ROS_MESSAGES RUBY_TARGETS SANE_BACKENDS USERLAND UWSGI_PLUGINS VIDEO_CARDS VOICEMAIL_STORAGE XTABLES_ADDONS"
+USE_EXPAND="ABI_MIPS ABI_S390 ABI_X86 ADA_TARGET ALSA_CARDS AMDGPU_TARGETS APACHE2_MODULES APACHE2_MPMS CALLIGRA_FEATURES CAMERAS COLLECTD_PLUGINS CPU_FLAGS_ARM CPU_FLAGS_PPC CPU_FLAGS_X86 CURL_SSL ELIBC FFTOOLS GPSD_PROTOCOLS GRUB_PLATFORMS INPUT_DEVICES KERNEL L10N LCD_DEVICES LIBREOFFICE_EXTENSIONS LLVM_TARGETS LUA_SINGLE_TARGET LUA_TARGETS MONKEYD_PLUGINS NGINX_MODULES_HTTP NGINX_MODULES_MAIL NGINX_MODULES_STREAM OFFICE_IMPLEMENTATION OPENMPI_FABRICS OPENMPI_OFED_FEATURES OPENMPI_RM PHP_TARGETS POSTGRES_TARGETS PYTHON_SINGLE_TARGET PYTHON_TARGETS QEMU_SOFTMMU_TARGETS QEMU_USER_TARGETS ROS_MESSAGES RUBY_TARGETS SANE_BACKENDS USERLAND UWSGI_PLUGINS VIDEO_CARDS VOICEMAIL_STORAGE XTABLES_ADDONS"
 
 # USE_EXPAND variables whose contents are not shown in package manager
 # output. Changes need discussion on gentoo-dev.
-- 
2.34.1

[gentoo-dev] [PATCH v2 2/2] profiles/desc: add amdgpu_targets.desc for USE_EXPAND

[Yiyang Wu]

Signed-off-by: Yiyang Wu <xgreenlandforwyy@gmail.com>
Signed-off-by: YiyangWu <xgreenlandforwyy@gmail.com>
---
 profiles/desc/amdgpu_targets.desc | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
 create mode 100644 profiles/desc/amdgpu_targets.desc

diff --git a/profiles/desc/amdgpu_targets.desc b/profiles/desc/amdgpu_targets.desc
new file mode 100644
index 000000000000..8a3db2b56dab
--- /dev/null
+++ b/profiles/desc/amdgpu_targets.desc
@@ -0,0 +1,15 @@
+# Copyright 1999-2022 Gentoo Authors.
+# Distributed under the terms of the GNU General Public License v2
+
+# Copied from https://www.coelacanth-dream.com/posts/2019/12/30/did-rid-product-matome-p2/#fn:67
+
+gfx803 - Fiji GPU, codename fiji, including Radeon R9 Nano/Fury/FuryX, Radeon Pro Duo, FirePro S9300x2, Radeon Instinct MI8 
+gfx900 - Vega GPU, codename vega10, including Radeon Vega Frontier Edition, Radeon RX Vega 56/64, Radeon RX Vega 64 Liquid, Radeon Pro Vega 48/56/64/64X, Radeon Pro WX 8200/9100, Radeon Pro V320/V340/SSG, Radeon Instinct MI25
+gfx906 - Vega GPU, codename vega20, including Radeon (Pro) VII, Radeon Instinct MI50/MI60
+gfx908 - CDNA Accelerator, codename arcturus, including AMD Instinct MI100 Accelerator
+gfx90a - CDNA2 Accelerator, codename aldebaran, including AMD Instinct MI200 series Accelerators
+gfx1010 - RDNA GPU, codename navi10, including Radeon RX 5700XT/5700/5700M/5700B/5700XTB/5600XT/5600/5600M, Radeon Pro 5700XT/5700, Radeon Pro W5700X/W5700
+gfx1011 - RDNA GPU, codename navi12, including Radeon Pro 5600M/V520
+gfx1012 - RDNA GPU, codename navi14, including Radeon RX 5500XT/5500/5500M/5500XTB/5300/5300M, Radeon Pro 5500XT/5500M/5300/5300M, Radeon Pro W5500X/W5500/W5500M/W5300M
+gfx1030 - RDNA2 GPU, codename navi21/sienna cichlid, including Radeon RX 6950XT/6900XT/6800XT/6800, Radeon Pro W6800
+gfx1031 - RDNA2 GPU, codename navi22/navy flounder, including Radeon RX 6750XT/6700XT/6800M/6700M
-- 
2.34.1

Re: [gentoo-dev] [PATCH v2 0/2] rocm.eclass: new eclass for ROCm packages

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 3782 bytes --]



> On 15 Aug 2022, at 15:43, Yiyang Wu <xgreenlandforwyy@gmail.com> wrote:
> 
> From: YiyangWu <xgreenlandforwyy@gmail.com>
> 
> This update fixes problems pointed out last week on rocm.eclass.
> 
> Changelog from v1:
> 1. Change to the mordern EAPI check style;
> 2. fix typo in amdgpu_targets.desc;
> 3. remove trailing space in comments
> 
> Yiyang Wu (2):
>  rocm.eclass: new eclass
>  profiles/desc: add amdgpu_targets.desc for USE_EXPAND
> 
> eclass/rocm.eclass                | 275 ++++++++++++++++++++++++++++++
> profiles/base/make.defaults       |   2 +-
> profiles/desc/amdgpu_targets.desc |  15 ++
> 3 files changed, 291 insertions(+), 1 deletion(-)
> create mode 100644 eclass/rocm.eclass
> create mode 100644 profiles/desc/amdgpu_targets.desc

Please squash the rocm fixes into the first commit.

> 
> Interdiff against v1:
> diff --git a/eclass/rocm.eclass b/eclass/rocm.eclass
> index e4b448f7b894..8ca2c051ddce 100644
> --- a/eclass/rocm.eclass
> +++ b/eclass/rocm.eclass
> @@ -19,7 +19,7 @@
> # Most ROCm packages use cmake as build system, so this eclass does not export
> # phase functions which overwrites the phase functions in cmake.eclass. Ebuild
> # should explicitly call rocm_src_* in src_configure and src_test.
> -#
> +#
> # @EXAMPLE:
> # # Example for ROCm packages in https://github.com/ROCmSoftwarePlatform
> # inherit cmake rocm
> @@ -28,25 +28,25 @@
> # IUSE="test"
> # REQUIRED_USE="${ROCM_REQUIRED_USE}"
> # RESTRICT="!test? ( test )"
> -#
> +#
> # RDEPEND="
> #     dev-util/hip
> #     sci-libs/rocBLAS:${SLOT}[${ROCM_USEDEP}]
> # "
> -#
> +#
> # S=${WORKDIR}/${PN}-rocm-${PV}
> -#
> +#
> # src_configure() {
> #     local mycmakeargs=(
> #         -DBUILD_CLIENTS_TESTS=$(usex test ON OFF)
> #     )
> #     rocm_src_configure
> # }
> -#
> +#
> # src_test() {
> #     rocm_src_test
> # }
> -#
> +#
> # # Example for packages depend on ROCm libraries -- a package depend on
> # # rocBLAS, and use comma seperated ${HCC_AMDGPU_TARGET} to determine GPU
> # # architecture to compile. Requires ROCm version >5.
> @@ -67,14 +67,11 @@
> 
> if [[ ! ${_ROCM_ECLASS} ]]; then
> 
> -case ${EAPI} in
> -	0|1|2|3|4|5|6)
> -		die "${ECLASS}: unsupported EAPI=${EAPI:-0} (too old)"
> -		;;
> +case "${EAPI:-0}" in
> 	7|8)
> 		;;
> 	*)
> -		die "${ECLASS}: unsupported EAPI=${EAPI} (unknown)"
> +		die "Unsupported EAPI=${EAPI} for ${ECLASS}"
> 		;;
> esac
> 
> diff --git a/profiles/desc/amdgpu_targets.desc b/profiles/desc/amdgpu_targets.desc
> index 942ab51356fb..8a3db2b56dab 100644
> --- a/profiles/desc/amdgpu_targets.desc
> +++ b/profiles/desc/amdgpu_targets.desc
> @@ -8,7 +8,7 @@ gfx900 - Vega GPU, codename vega10, including Radeon Vega Frontier Edition, Rade
> gfx906 - Vega GPU, codename vega20, including Radeon (Pro) VII, Radeon Instinct MI50/MI60
> gfx908 - CDNA Accelerator, codename arcturus, including AMD Instinct MI100 Accelerator
> gfx90a - CDNA2 Accelerator, codename aldebaran, including AMD Instinct MI200 series Accelerators
> -gfx1010 - RDNA GPU, codename navi10, including Radeon RX 5700XT/5700/5700M/5700B/5700XTB/5600XT/5600/5600M, Radeon Pro 5700Xt/5700, Radeon Pro W5700X/W5700
> +gfx1010 - RDNA GPU, codename navi10, including Radeon RX 5700XT/5700/5700M/5700B/5700XTB/5600XT/5600/5600M, Radeon Pro 5700XT/5700, Radeon Pro W5700X/W5700
> gfx1011 - RDNA GPU, codename navi12, including Radeon Pro 5600M/V520
> gfx1012 - RDNA GPU, codename navi14, including Radeon RX 5500XT/5500/5500M/5500XTB/5300/5300M, Radeon Pro 5500XT/5500M/5300/5300M, Radeon Pro W5500X/W5500/W5500M/W5300M
> gfx1030 - RDNA2 GPU, codename navi21/sienna cichlid, including Radeon RX 6950XT/6900XT/6800XT/6800, Radeon Pro W6800
> --
> 2.34.1
> 
> 


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: [gentoo-dev] [PATCH v2 0/2] rocm.eclass: new eclass for ROCm packages

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]



> On 15 Aug 2022, at 15:47, Sam James <sam@gentoo.org> wrote:
> 
> 
> 
>> On 15 Aug 2022, at 15:43, Yiyang Wu <xgreenlandforwyy@gmail.com> wrote:
>> 
>> From: YiyangWu <xgreenlandforwyy@gmail.com>
>> 
>> This update fixes problems pointed out last week on rocm.eclass.
>> 
>> Changelog from v1:
>> 1. Change to the mordern EAPI check style;
>> 2. fix typo in amdgpu_targets.desc;
>> 3. remove trailing space in comments
>> 
>> Yiyang Wu (2):
>> rocm.eclass: new eclass
>> profiles/desc: add amdgpu_targets.desc for USE_EXPAND
>> 
>> eclass/rocm.eclass                | 275 ++++++++++++++++++++++++++++++
>> profiles/base/make.defaults       |   2 +-
>> profiles/desc/amdgpu_targets.desc |  15 ++
>> 3 files changed, 291 insertions(+), 1 deletion(-)
>> create mode 100644 eclass/rocm.eclass
>> create mode 100644 profiles/desc/amdgpu_targets.desc
> 
> Please squash the rocm fixes into the first commit.
> 

Oh, sorry, I'd missed it was in the cover letter -- I thought it was in the USE_EXPAND commit.

Ignore me!


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: [gentoo-dev] [PATCH v2 1/2] rocm.eclass: new eclass

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 11430 bytes --]

>>>>> On Mon, 15 Aug 2022, Yiyang Wu wrote:

> diff --git a/eclass/rocm.eclass b/eclass/rocm.eclass
> new file mode 100644
> index 000000000000..8ca2c051ddce
> --- /dev/null
> +++ b/eclass/rocm.eclass
> @@ -0,0 +1,275 @@
> +# Copyright 2022 Gentoo Authors
> +# Distributed under the terms of the GNU General Public License v2
> +
> +# @ECLASS: rocm.eclass
> +# @MAINTAINER:
> +# Gentoo Science Project <sci@gentoo.org>
> +# @AUTHOR:
> +# Yiyang Wu <xgreenlandforwyy@gmail.com>
> +# @SUPPORTED_EAPIS: 7 8
> +# @BLURB: Common functions and variables for ROCm packages written in HIP
> +# @DESCRIPTION:
> +# ROCm packages such as sci-libs/<roc|hip>* can utilize functions in this eclass.
> +# Currently, it handles the AMDGPU_TARGETS variable via USE_EXPAND, so user can
> +# use USE flag to control which GPU architecture to compile, and ensure coherence
> +# among dependencies. It also specify CXX=hipcc, to let hipcc compile. Another
> +# important function is src_test, which checks whether a valid KFD device exists
> +# for testing, and then execute the test program.

Please wrap lines at below 80 character positions. (This applies both to
documentation and to code.)

> +#
> +# Most ROCm packages use cmake as build system, so this eclass does not export
> +# phase functions which overwrites the phase functions in cmake.eclass. Ebuild
> +# should explicitly call rocm_src_* in src_configure and src_test.
> +#
> +# @EXAMPLE:
> +# # Example for ROCm packages in https://github.com/ROCmSoftwarePlatform
> +# inherit cmake rocm
> +# SRC_URI="https://github.com/ROCmSoftwarePlatform/${PN}/archive/rocm-${PV}.tar.gz -> ${P}.tar.gz"
> +# SLOT="0/$(ver_cut 1-2)"
> +# IUSE="test"
> +# REQUIRED_USE="${ROCM_REQUIRED_USE}"
> +# RESTRICT="!test? ( test )"
> +#
> +# RDEPEND="
> +#     dev-util/hip
> +#     sci-libs/rocBLAS:${SLOT}[${ROCM_USEDEP}]
> +# "
> +#
> +# S=${WORKDIR}/${PN}-rocm-${PV}
> +#
> +# src_configure() {
> +#     local mycmakeargs=(
> +#         -DBUILD_CLIENTS_TESTS=$(usex test ON OFF)
> +#     )
> +#     rocm_src_configure
> +# }
> +#
> +# src_test() {
> +#     rocm_src_test
> +# }
> +#
> +# # Example for packages depend on ROCm libraries -- a package depend on
> +# # rocBLAS, and use comma seperated ${HCC_AMDGPU_TARGET} to determine GPU
> +# # architecture to compile. Requires ROCm version >5.
> +# ROCM_VERSION=5
> +# inherit rocm
> +# IUSE="rocm"
> +# REQUIRED_USE="rocm? ( ${ROCM_REQUIRED_USE} )"
> +# DEPEND="rocm? ( >=dev-util/hip-${ROCM_VERSION}
> +#     >=sci-libs/rocBLAS-${ROCM_VERSION}[${ROCM_USEDEP}] )"
> +# ....
> +# src_configure() {
> +#     if use rocm; then
> +#         local AMDGPU_FLAGS=$(get_amdgpu_flags)
> +#         export HCC_AMDGPU_TARGET=${AMDGPU_FLAGS//;/,}
> +#     fi
> +#     default
> +# }

@EXAMPLE is read as a paragraph, i.e. lines will be wrapped and this is
how it will look when rendered as eclass manpage:

       #  Example for ROCm packages in https://github.com/ROCmSoftwarePlatform
       inherit   cmake   rocm    SRC_URI="https://github.com/ROCmSoftwarePlat‐
       form/${PN}/archive/rocm-${PV}.tar.gz  -> ${P}.tar.gz" SLOT="0/$(ver_cut
       1-2)" IUSE="test" REQUIRED_USE="${ROCM_REQUIRED_USE}"  RESTRICT="!test?
       ( test )"

       [...]

So, you may want to include the whole example in a pair of @CODE tokens.

> +
> +if [[ ! ${_ROCM_ECLASS} ]]; then
> +
> +case "${EAPI:-0}" in

This should be just ${EAPI} without a fallback to 0. Also, the
quotation marks are not necessary.

> +	7|8)
> +		;;
> +	*)
> +		die "Unsupported EAPI=${EAPI} for ${ECLASS}"

Whereas here it should be ${EAPI:-0}.

> +		;;
> +esac

Or even better, follow standard usage as it can be found in
multilib-minimal or toolchain-funcs:

case ${EAPI} in
	7|8) ;;
	*) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
esac

> +
> +inherit edo
> +
> +

No double empty lines please. (Pkgcheck should have complained about
this.)

> +# @ECLASS_VARIABLE: ROCM_VERSION
> +# @DEFAULT_UNSET
> +# @PRE_INHERIT
> +# @DESCRIPTION:
> +# The ROCm version of current package. Default is ${PV}, but for other packages
> +# that depend on ROCm libraries, this can be set to match the version of
> +# required ROCm libraries.
> +
> +# @ECLASS_VARIABLE: ALL_AMDGPU_TARGETS
> +# @INTERNAL
> +# @DESCRIPTION:
> +# The list of USE flags corresponding to all AMDGPU targets in this ROCm
> +# version. The value depends on ${PV}. Architectures and devices map:
> +# https://www.coelacanth-dream.com/posts/2019/12/30/did-rid-product-matome-p2
> +
> +# @ECLASS_VARIABLE: OFFICIAL_AMDGPU_TARGETS
> +# @INTERNAL
> +# @DESCRIPTION:
> +# The list of USE flags corresponding to all officlially supported AMDGPU

"officially"

> +# targets in this ROCm version, documented at
> +# https://docs.amd.com/bundle/ROCm-Installation-Guide-v${PV}/page/Prerequisite_Actions.html.
> +# USE flag of these architectures will be default on. Depends on ${PV}.
> +
> +# @ECLASS_VARIABLE: ROCM_REQUIRED_USE
> +# @OUTPUT_VARIABLE
> +# @DESCRIPTION:
> +# Requires at least one AMDGPU target to be compiled.
> +# Example use for ROCm libraries:
> +# REQUIRED_USE="${ROCM_REQUIRED_USE}"
> +# Example use for packages that depend on ROCm libraries
> +# IUSE="rocm"
> +# REQUIRED_USE="rocm? ( ${ROCM_REQUIRED_USE} )"
> +
> +# @ECLASS_VARIABLE: ROCM_USEDEP
> +# @OUTPUT_VARIABLE
> +# @DESCRIPTION:
> +# This is an eclass-generated USE-dependency string which can be used to
> +# depend on another ROCm package being built for the same AMDGPU architecture.
> +#
> +# The generated USE-flag list is compatible with packages using rocm.eclass.
> +#
> +# Example use:
> +# @CODE
> +# DEPEND="sci-libs/rocBLAS[${ROCM_USEDEP}]"
> +# @CODE
> +
> +# @FUNCTION: _rocm_set_globals
> +# @DESCRIPTION:
> +# Set global variables used by the eclass: ALL_AMDGPU_TARGETS,
> +# OFFICIAL_AMDGPU_TARGETS, ROCM_REQUIRED_USE, and ROCM_USEDEP
> +_rocm_set_globals() {
> +	case ${ROCM_VERSION:-${PV}} in
> +		4*)
> +			ALL_AMDGPU_TARGETS=(
> +				gfx803 gfx900 gfx906 gfx908
> +				gfx1010 gfx1011 gfx1012 gfx1030
> +			)
> +			OFFICIAL_AMDGPU_TARGETS=(
> +				gfx906 gfx908
> +			)
> +			;;
> +		5*)
> +			ALL_AMDGPU_TARGETS=(
> +				gfx803 gfx900 gfx906 gfx908 gfx90a
> +				gfx1010 gfx1011 gfx1012 gfx1030 gfx1031
> +			)
> +			OFFICIAL_AMDGPU_TARGETS=(
> +				gfx906 gfx908 gfx90a gfx1030
> +			)
> +			;;
> +		*)
> +			die "Unknown ROCm major version! Please update rocm.eclass before bumping to new ebuilds"
> +			;;
> +	esac
> +
> +	ROCM_REQUIRED_USE+=" || ("
> +	for gpu_target in ${ALL_AMDGPU_TARGETS[@]}; do

Add a pair of double quotes here.

> +		if [[ " ${OFFICIAL_AMDGPU_TARGETS[*]} " =~ " ${gpu_target} " ]]; then

So OFFICIAL_AMDGPU_TARGETS is an array, but then it's flattened to a
string for the test? Either use "has" for the test, or don't use an
array in the first place.

> +			IUSE+=" ${gpu_target/#/+amdgpu_targets_}"

Why "+="? I don't see any previous assignment of IUSE.

> +		else
> +			IUSE+=" ${gpu_target/#/amdgpu_targets_}"

Ditto.

> +		fi
> +		ROCM_REQUIRED_USE+=" ${gpu_target/#/amdgpu_targets_}"
> +	done
> +	ROCM_REQUIRED_USE+=" ) "
> +
> +	local flags=( "${ALL_AMDGPU_TARGETS[@]/#/amdgpu_targets_}" )
> +	local optflags=${flags[@]/%/(-)?}
> +	ROCM_USEDEP=${optflags// /,}
> +}
> +_rocm_set_globals
> +unset -f _rocm_set_globals
> +
> +
> +# @FUNCTION: get_amdgpu_flags
> +# @USAGE: get_amdgpu_flags
> +# @DESCRIPTION:
> +# Convert specified use flag of amdgpu_targets to compilation flags.
> +# Append default target feature to GPU arch. See
> +# https://llvm.org/docs/AMDGPUUsage.html#target-features
> +get_amdgpu_flags() {
> +	local AMDGPU_TARGET_FLAGS
> +	for gpu_target in ${ALL_AMDGPU_TARGETS[@]}; do

Double quotes please.

> +		local target_feature=
> +		if use amdgpu_targets_${gpu_target}; then
> +			case ${gpu_target} in
> +				gfx906|gfx908)
> +					target_feature=:xnack-
> +					;;
> +				gfx90a)
> +					target_feature=:xnack+
> +					;;
> +				*)
> +					;;
> +			esac
> +			AMDGPU_TARGET_FLAGS+="${gpu_target}${target_feature};"
> +		fi
> +	done
> +	echo ${AMDGPU_TARGET_FLAGS}
> +}
> +
> +# @FUNCTION: check_rw_permission
> +# @USAGE: check_rw_permission <file>
> +# @DESCRIPTION:
> +# check read and write permissions on specific files.
> +# allow using wildcard, for example check_rw_permission /dev/dri/render*
> +check_rw_permission() {
> +	[[ -r "$1" ]] && [[ -w "$1" ]] || die \

Quotation marks aren't necessary here.

> +		"${PORTAGE_USERNAME} do not have read or write permissions on $1! \n Make sure ${PORTAGE_USERNAME} is in render group and check the permissions."

Please don't use internal Portage variables.

> +}
> +
> +# == phase functions ==
> +
> +# @FUNCTION: rocm_src_configure
> +# @DESCRIPTION:
> +# configure rocm packages, and setting common cmake arguments
> +rocm_src_configure() {
> +	# allow acces to hardware
> +	addpredict /dev/kfd
> +	addpredict /dev/dri/
> +
> +	mycmakeargs+=(
> +		-DCMAKE_INSTALL_PREFIX="${EPREFIX}/usr"
> +		-DAMDGPU_TARGETS="$(get_amdgpu_flags)"
> +		-DCMAKE_SKIP_RPATH=TRUE
> +	)
> +
> +	CXX="hipcc" cmake_src_configure
> +}
> +
> +# @FUNCTION: rocm_src_test
> +# @DESCRIPTION:
> +# Test whether valid GPU device is present. If so, find how to, and execute test.
> +# ROCm packages can have to test mechanism:
> +# 1. cmake_src_test. Set MAKEOPTS="-j1" to make sure only one test on GPU at a time;
> +# 2. one single gtest binary called "${PN,,}"-test;
> +# 3. Some package like rocFFT have alternative test like rocfft-selftest;
> +# 4. Custome testing binaries like dev-libs/rccl. Use ${ROCM_TESTS} to specify.
> +rocm_src_test() {
> +	addwrite /dev/kfd
> +	addwrite /dev/dri/
> +
> +	# check permissions on /dev/kfd and /dev/dri/render*
> +	check_rw_permission /dev/kfd
> +	check_rw_permission /dev/dri/render*
> +
> +	: ${LD_LIBRARY_PATH:="${BUILD_DIR}/clients:${BUILD_DIR}/src:${BUILD_DIR}/library:${BUILD_DIR}/library/src:${BUILD_DIR}/library/src/device"}
> +	export LD_LIBRARY_PATH
> +	if grep -q 'build test:' "${BUILD_DIR}"/build.ninja; then
> +		MAKEOPTS="-j1" cmake_src_test
> +	elif [[ -d "${BUILD_DIR}"/clients/staging ]]; then

Quotation marks aren't necessary here.

> +		cd "${BUILD_DIR}/clients/staging" || die "Test directory not found!"
> +		for test_program in "${PN,,}-"*test; do
> +			if [[ -x ${test_program} ]]; then
> +					edob ./${test_program}
> +			else
> +				die "The test program ${test_program} does not exist or cannot be excuted!"
> +			fi
> +		done
> +	elif [[ ! -z "${ROCM_TESTS}" ]]; then

Again, no quotation marks.

Also, why the double negation? IMHO "-n" would be better readable.

> +		for test_program in "${ROCM_TESTS}"; do

What is this supposed to do? The loop will be executed exactly once,
whatever ROCM_TESTS contains.

> +			cd "${BUILD_DIR}" || die
> +			if [[ -x ${test_program} ]]; then
> +			edob ./${test_program}
> +			else
> +				die "The test program ${test_program} does not exist or cannot be excuted!"
> +			fi
> +		done
> +	else
> +		die "There is no cmake tests, no \${ROCM_TESTS} executable provided, nor ${BUILD_DIR}/clients/staging where test program might be located."
> +	fi
> +}
> +
> +_ROCM_ECLASS=1
> +fi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 507 bytes --]

Re: [gentoo-dev] [PATCH v2 1/2] rocm.eclass: new eclass

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 216 bytes --]

>>>>> On Tue, 16 Aug 2022, Ulrich Mueller wrote:

>> +			IUSE+=" ${gpu_target/#/+amdgpu_targets_}"

> Why "+="? I don't see any previous assignment of IUSE.

Please ignore this. I had missed that it's inside a loop.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 507 bytes --]

Re: [gentoo-dev] [PATCH v2 1/2] rocm.eclass: new eclass

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 173 bytes --]

>>>>> On Mon, 15 Aug 2022, Yiyang Wu wrote:

> +_rocm_set_globals() {
> +	case ${ROCM_VERSION:-${PV}} in
> +		4*)

What will happen when there's a version 40 in the future?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 507 bytes --]

Re: [gentoo-dev] [PATCH v2 1/2] rocm.eclass: new eclass

[wuyy]

Thank you very much for pointing out so much problems I missed!

On Tue, Aug 16, 2022 at 12:41:47AM +0200, Ulrich Mueller wrote:
> Please wrap lines at below 80 character positions. (This applies both to
> documentation and to code.)
> 

Thanks. Seems that I forgot yo run re-wrapping in a final round. I don't
see any `pkgcheck scan` complains about this, maybe such check can be
added?

Also, I have some long sentences in code, for throwing information. How
do I nicely wrap such long strings in bash? By incremental assigning it
to variables I guess?

> @EXAMPLE is read as a paragraph, i.e. lines will be wrapped and this is
> how it will look when rendered as eclass manpage:
> 
>        #  Example for ROCm packages in https://github.com/ROCmSoftwarePlatform
>        inherit   cmake   rocm    SRC_URI="https://github.com/ROCmSoftwarePlat‐
>        form/${PN}/archive/rocm-${PV}.tar.gz  -> ${P}.tar.gz" SLOT="0/$(ver_cut
>        1-2)" IUSE="test" REQUIRED_USE="${ROCM_REQUIRED_USE}"  RESTRICT="!test?
>        ( test )"
> 
>        [...]
> 
> So, you may want to include the whole example in a pair of @CODE tokens.
> 

Thanks for pointing out. This is my first eclass and I didn't know how
documents are rendered before.

> > +
> > +if [[ ! ${_ROCM_ECLASS} ]]; then
> > +
> > +case "${EAPI:-0}" in
> 
> This should be just ${EAPI} without a fallback to 0. Also, the
> quotation marks are not necessary.
> 
> > +	7|8)
> > +		;;
> > +	*)
> > +		die "Unsupported EAPI=${EAPI} for ${ECLASS}"
> 
> Whereas here it should be ${EAPI:-0}.
> 
> > +		;;
> > +esac
> 
> Or even better, follow standard usage as it can be found in
> multilib-minimal or toolchain-funcs:
> 
> case ${EAPI} in
> 	7|8) ;;
> 	*) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
> esac
> 

I'll take the suggestion. Thanks!

> > +
> > +inherit edo
> > +
> > +
> 
> No double empty lines please. (Pkgcheck should have complained about
> this.)

OK. But `pkgcheck scan rocm.eclass` (version 0.10.12) does not
complains. A missing feature maybe?

> > +# @DESCRIPTION:
> > +# The list of USE flags corresponding to all officlially supported AMDGPU
> 
> "officially"
> 
> > +	ROCM_REQUIRED_USE+=" || ("
> > +	for gpu_target in ${ALL_AMDGPU_TARGETS[@]}; do
> 
> Add a pair of double quotes here.

OK, I'll polish here in v2.

> > +		if [[ " ${OFFICIAL_AMDGPU_TARGETS[*]} " =~ " ${gpu_target} " ]]; then
> So OFFICIAL_AMDGPU_TARGETS is an array, but then it's flattened to a
> string for the test? Either use "has" for the test, or don't use an
> array in the first place.

Yes, I should have used a better approach.

> > +get_amdgpu_flags() {
> > +	local AMDGPU_TARGET_FLAGS
> > +	for gpu_target in ${ALL_AMDGPU_TARGETS[@]}; do
> 
> Double quotes please.
> 

Thanks for finding such problem once again.

> > +# @FUNCTION: check_rw_permission
> > +# @USAGE: check_rw_permission <file>
> > +# @DESCRIPTION:
> > +# check read and write permissions on specific files.
> > +# allow using wildcard, for example check_rw_permission /dev/dri/render*
> > +check_rw_permission() {
> > +	[[ -r "$1" ]] && [[ -w "$1" ]] || die \
> 
> Quotation marks aren't necessary here.

Yes, and after I perform another check, this function is implemented
incorrectly -- it is designed to accept wildcards (later in
rocm_src_test, there is check_rw_permission /dev/dri/render*) but when
bash pass the expanded argument to list, $1 is only the first matching
file. So I should think of a correct implementation, or stop using
wildcards. This is a bug (rare to encounter however), and I need to fix
it in v2.

> 
> > +		"${PORTAGE_USERNAME} do not have read or write permissions on $1! \n Make sure ${PORTAGE_USERNAME} is in render group and check the permissions."
> 
> Please don't use internal Portage variables.
OK, although I can't find such warnings on
https://devmanual.gentoo.org/eclass-writing. Maybe an entry should be
added?

> > +	elif [[ -d "${BUILD_DIR}"/clients/staging ]]; then
> 
> Quotation marks aren't necessary here.

What if BUILD_DIR contains spaces?

> > +		cd "${BUILD_DIR}/clients/staging" || die "Test directory not found!"
> > +		for test_program in "${PN,,}-"*test; do
> > +			if [[ -x ${test_program} ]]; then
> > +					edob ./${test_program}
> > +			else
> > +				die "The test program ${test_program} does not exist or cannot be excuted!"
> > +			fi
> > +		done
> > +	elif [[ ! -z "${ROCM_TESTS}" ]]; then
> 
> Again, no quotation marks.
> 
> Also, why the double negation? IMHO "-n" would be better readable.

Yeah, I don't understand myself either.

> 
> > +		for test_program in "${ROCM_TESTS}"; do
> 
> What is this supposed to do? The loop will be executed exactly once,
> whatever ROCM_TESTS contains.

Well, I hope ROCM_TESTS can have multiple executables, so that's why
it have the trailing "S". But currently no packages need such feature.
If ROCM_TESTS is a string using space to separate each test, then there
shouldn't be quotation marks. I'll fix that.

-- 

Re: [gentoo-dev] [PATCH v2 1/2] rocm.eclass: new eclass

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]

>>>>> On Wed, 17 Aug 2022, wuyy  wrote:

> Also, I have some long sentences in code, for throwing information. How
> do I nicely wrap such long strings in bash? By incremental assigning it
> to variables I guess?

It's not an absolute rule. Avoid long lines where possible, but don't
jump through hoops to e.g. break long strings in assignments.

>> No double empty lines please. (Pkgcheck should have complained about
>> this.)

> OK. But `pkgcheck scan rocm.eclass` (version 0.10.12) does not
> complains. A missing feature maybe?

Hm, it warns about it in ebuilds but not in eclasses. This looks like a
bug in pkgcheck to me.

>> Please don't use internal Portage variables.
> OK, although I can't find such warnings on
> https://devmanual.gentoo.org/eclass-writing. Maybe an entry should be
> added?

This is in PMS section 12.3.17 "Reserved commands and variables":
https://dev.gentoo.org/~ulm/pms/head/pms.html#x1-13700012.3.17

Let's add it to the devmanual as well:
https://github.com/gentoo/devmanual/pull/301/commits/616323bd33f6aee5b579c24e31a4605c36e08b53

>> > +	elif [[ -d "${BUILD_DIR}"/clients/staging ]]; then
>> 
>> Quotation marks aren't necessary here.

> What if BUILD_DIR contains spaces?

That's not a problem because word splitting isn't performed inside [[ ]]:
https://www.gnu.org/software/bash/manual/html_node/Conditional-Constructs.html#index-_005b_005b

Ulrich

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 507 bytes --]