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