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. --
