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 rocmSRC_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
> > +# @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 trail
