Gentoo Archives: gentoo-dev

From: wuyy <xgreenlandforwyy@×××××.com>
To: Ulrich Mueller <ulm@g.o>
Cc: "gentoo-dev@l.g.o" <gentoo-dev@l.g.o>, Benda Xu <heroxbd@g.o>
Subject: Re: [gentoo-dev] [PATCH v2 1/2] rocm.eclass: new eclass
Date: Wed, 17 Aug 2022 03:14:49
Message-Id: YvxdH7ujD77lsbfX@HEPwuyy
In Reply to: Re: [gentoo-dev] [PATCH v2 1/2] rocm.eclass: new eclass by Ulrich Mueller
1 Thank you very much for pointing out so much problems I missed!
2
3 On Tue, Aug 16, 2022 at 12:41:47AM +0200, Ulrich Mueller wrote:
4 > Please wrap lines at below 80 character positions. (This applies both to
5 > documentation and to code.)
6 >
7
8 Thanks. Seems that I forgot yo run re-wrapping in a final round. I don't
9 see any `pkgcheck scan` complains about this, maybe such check can be
10 added?
11
12 Also, I have some long sentences in code, for throwing information. How
13 do I nicely wrap such long strings in bash? By incremental assigning it
14 to variables I guess?
15
16 > @EXAMPLE is read as a paragraph, i.e. lines will be wrapped and this is
17 > how it will look when rendered as eclass manpage:
18 >
19 > # Example for ROCm packages in https://github.com/ROCmSoftwarePlatform
20 > inherit cmake rocm SRC_URI="https://github.com/ROCmSoftwarePlat‐
21 > form/${PN}/archive/rocm-${PV}.tar.gz -> ${P}.tar.gz" SLOT="0/$(ver_cut
22 > 1-2)" IUSE="test" REQUIRED_USE="${ROCM_REQUIRED_USE}" RESTRICT="!test?
23 > ( test )"
24 >
25 > [...]
26 >
27 > So, you may want to include the whole example in a pair of @CODE tokens.
28 >
29
30 Thanks for pointing out. This is my first eclass and I didn't know how
31 documents are rendered before.
32
33 > > +
34 > > +if [[ ! ${_ROCM_ECLASS} ]]; then
35 > > +
36 > > +case "${EAPI:-0}" in
37 >
38 > This should be just ${EAPI} without a fallback to 0. Also, the
39 > quotation marks are not necessary.
40 >
41 > > + 7|8)
42 > > + ;;
43 > > + *)
44 > > + die "Unsupported EAPI=${EAPI} for ${ECLASS}"
45 >
46 > Whereas here it should be ${EAPI:-0}.
47 >
48 > > + ;;
49 > > +esac
50 >
51 > Or even better, follow standard usage as it can be found in
52 > multilib-minimal or toolchain-funcs:
53 >
54 > case ${EAPI} in
55 > 7|8) ;;
56 > *) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
57 > esac
58 >
59
60 I'll take the suggestion. Thanks!
61
62 > > +
63 > > +inherit edo
64 > > +
65 > > +
66 >
67 > No double empty lines please. (Pkgcheck should have complained about
68 > this.)
69
70 OK. But `pkgcheck scan rocm.eclass` (version 0.10.12) does not
71 complains. A missing feature maybe?
72
73 > > +# @DESCRIPTION:
74 > > +# The list of USE flags corresponding to all officlially supported AMDGPU
75 >
76 > "officially"
77 >
78 > > + ROCM_REQUIRED_USE+=" || ("
79 > > + for gpu_target in ${ALL_AMDGPU_TARGETS[@]}; do
80 >
81 > Add a pair of double quotes here.
82
83 OK, I'll polish here in v2.
84
85 > > + if [[ " ${OFFICIAL_AMDGPU_TARGETS[*]} " =~ " ${gpu_target} " ]]; then
86 > So OFFICIAL_AMDGPU_TARGETS is an array, but then it's flattened to a
87 > string for the test? Either use "has" for the test, or don't use an
88 > array in the first place.
89
90 Yes, I should have used a better approach.
91
92 > > +get_amdgpu_flags() {
93 > > + local AMDGPU_TARGET_FLAGS
94 > > + for gpu_target in ${ALL_AMDGPU_TARGETS[@]}; do
95 >
96 > Double quotes please.
97 >
98
99 Thanks for finding such problem once again.
100
101 > > +# @FUNCTION: check_rw_permission
102 > > +# @USAGE: check_rw_permission <file>
103 > > +# @DESCRIPTION:
104 > > +# check read and write permissions on specific files.
105 > > +# allow using wildcard, for example check_rw_permission /dev/dri/render*
106 > > +check_rw_permission() {
107 > > + [[ -r "$1" ]] && [[ -w "$1" ]] || die \
108 >
109 > Quotation marks aren't necessary here.
110
111 Yes, and after I perform another check, this function is implemented
112 incorrectly -- it is designed to accept wildcards (later in
113 rocm_src_test, there is check_rw_permission /dev/dri/render*) but when
114 bash pass the expanded argument to list, $1 is only the first matching
115 file. So I should think of a correct implementation, or stop using
116 wildcards. This is a bug (rare to encounter however), and I need to fix
117 it in v2.
118
119 >
120 > > + "${PORTAGE_USERNAME} do not have read or write permissions on $1! \n Make sure ${PORTAGE_USERNAME} is in render group and check the permissions."
121 >
122 > Please don't use internal Portage variables.
123 OK, although I can't find such warnings on
124 https://devmanual.gentoo.org/eclass-writing. Maybe an entry should be
125 added?
126
127 > > + elif [[ -d "${BUILD_DIR}"/clients/staging ]]; then
128 >
129 > Quotation marks aren't necessary here.
130
131 What if BUILD_DIR contains spaces?
132
133 > > + cd "${BUILD_DIR}/clients/staging" || die "Test directory not found!"
134 > > + for test_program in "${PN,,}-"*test; do
135 > > + if [[ -x ${test_program} ]]; then
136 > > + edob ./${test_program}
137 > > + else
138 > > + die "The test program ${test_program} does not exist or cannot be excuted!"
139 > > + fi
140 > > + done
141 > > + elif [[ ! -z "${ROCM_TESTS}" ]]; then
142 >
143 > Again, no quotation marks.
144 >
145 > Also, why the double negation? IMHO "-n" would be better readable.
146
147 Yeah, I don't understand myself either.
148
149 >
150 > > + for test_program in "${ROCM_TESTS}"; do
151 >
152 > What is this supposed to do? The loop will be executed exactly once,
153 > whatever ROCM_TESTS contains.
154
155 Well, I hope ROCM_TESTS can have multiple executables, so that's why
156 it have the trailing "S". But currently no packages need such feature.
157 If ROCM_TESTS is a string using space to separate each test, then there
158 shouldn't be quotation marks. I'll fix that.
159
160 --

Replies

Subject Author
Re: [gentoo-dev] [PATCH v2 1/2] rocm.eclass: new eclass Ulrich Mueller <ulm@g.o>