Gentoo Archives: gentoo-dev

From: "Michał Górny" <mgorny@g.o>
To: "Vadim A. Misbakh-Soloviov" <gentoo@×××.name>
Cc: gentoo-dev@l.g.o
Subject: Re: [gentoo-dev] [RFC] lua.eclass
Date: Tue, 26 Jul 2016 14:22:58
Message-Id: 20160726162226.3f6ca4e2.mgorny@gentoo.org
In Reply to: [gentoo-dev] [RFC] lua.eclass by "Vadim A. Misbakh-Soloviov"
1 On Wed, 20 Jul 2016 20:36:20 +0700
2 "Vadim A. Misbakh-Soloviov" <gentoo@×××.name> wrote:
3
4 > # Copyright 1999-2016 Gentoo Foundation
5 > # Distributed under the terms of the GNU General Public License v2
6 >
7 > # @ECLASS: lua.eclass
8 > # @MAINTAINER:
9 > # mva <lua@×××.name>
10 > # @AUTHOR:
11 > # Author: Vadim A. Misbakh-Soloviov <lua@×××.name>
12 > # @BLURB: An eclass for installing Lua packages with proper support for multiple Lua slots.
13 > # @DESCRIPTION:
14 > # The Lua eclass is designed to allow an easier installation of Lua packages
15 > # and their incorporation into the Gentoo Linux system.
16 > #
17 > # Currently available targets are:
18 > # * lua51 - Lua (PUC-Rio) 5.1
19 > # * lua52 - Lua (PUC-Rio) 5.2
20 > # * lua53 - Lua (PUC-Rio) 5.3
21 > # * luajit2 - LuaJIT 2.x
22 > #
23 > # This eclass does not define the implementation of the configure,
24 > # compile, test, or install phases. Instead, the default phases are
25 > # used. Specific implementations of these phases can be provided in
26 > # the ebuild either to be run for each Lua implementation, or for all
27 > # Lua implementations, as follows:
28 > #
29 > # * each_lua_configure
30 > # * all_lua_configure
31 >
32 > # @ECLASS-VARIABLE: LUA_COMPAT
33 > # @REQUIRED
34 > # @DESCRIPTION:
35 > # This variable contains a space separated list of targets (see above) a package
36 > # is compatible to. It must be set before the `inherit' call.
37 > : ${LUA_COMPAT:=lua51 lua52 lua53 luajit2}
38
39 Am I missing something or are you not verifying the value anywhere?
40
41 Besides, are you really convinced this is a sane default? I don't know
42 bad Lua is these days but if you start with all impls on, adding a new
43 Lua version is going to be painful.
44
45 >
46 > # @ECLASS-VARIABLE: LUA_PATCHES
47 > # @DEFAULT_UNSET
48 > # @DESCRIPTION:
49 > # A String or Array of filenames of patches to apply to all implementations.
50
51 Why do you need a special variable for it? Why not just let PATCHES do
52 their job?
53
54 >
55 > # @ECLASS-VARIABLE: LUA_OPTIONAL
56 > # @DESCRIPTION:
57 > # Set the value to "yes" to make the dependency on a Lua interpreter
58 > # optional and then lua_implementations_depend() to help populate
59 > # DEPEND and RDEPEND.
60 >
61 > # @ECLASS-VARIABLE: LUA_S
62 > # @DEFAULT_UNSET
63 > # @DESCRIPTION:
64 > # If defined this variable determines the source directory name after
65 > # unpacking. This defaults to the name of the package. Note that this
66 > # variable supports a wildcard mechanism to help with github tarballs
67 > # that contain the commit hash as part of the directory name.
68
69 1. New GitHub tarballs no longer have the commit hash (fetched using
70 the links provided nowadays).
71
72 2. Implicit wildcards are a very bad idea, and we already have
73 a dedicated eclass to handle this more safely.
74
75 3. How is that exactly related to Lua support?
76
77 > # @ECLASS-VARIABLE: LUA_QA_ALLOWED_LIBS
78 > # @DEFAULT_UNSET
79 > # @DESCRIPTION:
80 > # If defined this variable contains a whitelist of shared objects that
81 > # are allowed to exist even if they don't link to liblua. This avoids
82 > # the QA check that makes this mandatory. This is most likely not what
83 > # you are looking for if you get the related "Missing links" QA warning,
84 > # since the proper fix is almost always to make sure the shared object
85 > # is linked against liblua. There are cases were this is not the case
86 > # and the shared object is generic code to be used in some other way.
87 > # When set this argument is passed to "grep -E" to remove reporting of
88 > # these shared objects.
89 >
90 > : ${GLOBAL_CFLAGS-${CFLAGS}}
91 > : ${GLOBAL_CXXFLAGS-${CXXFLAGS}}
92 > : ${GLOBAL_LDFLAGS-${LDFLAGS}}
93 >
94 > : ${NOCCACHE-false}
95 > : ${NODISTCC-false}
96
97 Why are those variables not namespaced? You're just asking for trouble.
98 Not that I see their relevance to Lua but I'll probably complain later
99 on.
100
101 > [[ -n "${IS_MULTILIB}" ]] && multilib="multilib-minimal"
102
103 Lacks namespace, undescribed.
104
105 > case ${VCS} in
106 > git)
107 > VCS="git-r3"
108 > ;;
109 > hg)
110 > VCS="mercurial"
111 > ;;
112 > svn)L
113 > VCS="subversion"
114 > ;;
115 > esac
116
117 Likewise. How is it exactly related to Lua? Why are you doing
118 all-in-one eclass? What's the problem with people sourcing the correct
119 eclass?
120
121 > [[ -n "${GITHUB_A}" && -n "${BITBUCKET_A}" ]] && die "Only one of GITHUB_A or BITBUCKET_A should be set!"
122
123 Likewise. Completely unrelated to Lua support.
124
125 > if [[ -n "${GITHUB_A}" ]]; then
126 > GITHUB_PN="${GITHUB_PN:-${PN}}"
127 > EVCS_URI="https://github.com/${GITHUB_A}/${GITHUB_PN}"
128 > DL="archive"
129 > elif [[ -n "${BITBUCKET_A}" ]]; then
130 > BITBUCKET_PN="${BITBUCKET_PN:-${PN}}"
131 > EVCS_URI="https://bitbucket.org/${BITBUCKET_A}/${BITBUCKET_PN}"
132 > DL="get"
133 > fi
134 > if [[ -z "${EGIT_REPO_URI}" && -z "${EHG_REPO_URI}" && -z "${SRC_URI}" && -n "${EVCS_URI}" ]]; then
135 > if [[ "${VCS}" = git* ]]; then
136 > EGIT_REPO_URI="${EVCS_URI}"
137 > elif [[ "${VCS}" = "mercurial" ]]; then
138 > EHG_REPO_URI="${EVCS_URI}"
139 > elif [[ -z "${VCS}" && "${PV}" != *9999* ]]; then
140 > SRC_URI="${EVCS_URI}/${DL}/${GITHUB_PV:-${PV}}.tar.gz -> ${P}.tar.gz"
141
142 Why this terribly confusing implicit magic behavior? You're just asking
143 people to spend a lot of time looking wtf is happening, and why stuff
144 appears out of nowhere.
145
146 > fi
147 > fi
148 >
149 > inherit eutils ${multilib} toolchain-funcs flag-o-matic ${VCS}
150 >
151 > EXPORT_FUNCTIONS src_unpack src_prepare src_configure src_compile src_install pkg_setup src_test
152 >
153 > case ${EAPI:-0} in
154 > 0|1|2|3)
155 > die "Unsupported EAPI=${EAPI} (too old) for lua.eclass"
156 > ;;
157 > 4|5|6)
158 > # S is no longer automatically assigned when it doesn't exist.
159 > S="${WORKDIR}"
160 > ;;
161
162 It's a new eclass. Don't support old EAPIs, or you're going to go into
163 compatibility mess for EAPIs that are going to be barely used.
164
165 > *)
166 > ewarn "Unknown EAPI=${EAPI} for lua.eclass. Some things may become broken"
167 > ewarn "Please, review lua.eclass for compatibility with new EAPI"
168
169 ewarn? That's not a good sign. Why should a user care that developer
170 committed ebuild without waiting for lua.eclass support first?
171
172 > ;;
173 > esac
174 >
175 > lua_implementation_depend() {
176 > local lua_pn=
177 > local lua_slot=
178 >
179 > case $1 in
180 > lua51)
181 > lua_pn="dev-lang/lua"
182 > lua_slot=":5.1"
183 > ;;
184 > lua52)
185 > lua_pn="dev-lang/lua"
186 > lua_slot=":5.2"
187 > ;;
188 > lua53)
189 > lua_pn="dev-lang/lua"
190 > lua_slot=":5.3"
191 > ;;
192 > luajit2)
193 > lua_pn="dev-lang/luajit"
194 > lua_slot=":2"
195 > ;;
196 > *) die "$1: unknown Lua implementation"
197 > esac
198 >
199 > echo "$2${lua_pn}$3${lua_slot}"
200 > }
201 >
202 > # @FUNCTION: lua_implementation_command
203 > # @RETURN: the path to the given lua implementation
204 > # @DESCRIPTION:
205 > lua_implementation_command() {
206 > local _lua_name=
207 > local _lua_slotted=$(lua_implementation_depend $1)
208 > _lua_name=${_lua_slotted//:}
209 >
210 > case $1 in
211 > luajit*)
212 > _lua_name=${_lua_slotted/:/-}
213 > ;;
214 > esac
215
216 This is confusing. Why not put both inside the case?
217
218 >
219 > local lua=$(readlink -fs $(type -p $(basename ${_lua_name} 2>/dev/null)) 2>/dev/null)
220
221 readlink is not very portable. Canonicalization gives the strong
222 suggestion you're doing something nifty, relying on stuff you're not
223 supposed to rely on and creating code that's going to easily go kaboom.
224
225 > [[ -x ${lua} ]] || die "Unable to locate executable Lua interpreter"
226 > echo "${lua}"
227 > }
228 >
229 > # @FUNCTION: lua_samelib
230 > # @RETURN: use flag string with current lua implementations
231
232 The function name doesn't suggest any relevance to USE flags.
233
234 > # @DESCRIPTION:
235 > # Convenience function to output the use dependency part of a
236 > # dependency. Used as a building block for lua_add_rdepend() and
237 > # lua_add_bdepend(), but may also be useful in an ebuild to specify
238 > # more complex dependencies.
239 > lua_samelib() {
240 > local res=
241 > for _lua_implementation in $LUA_COMPAT; do
242 > has -${_lua_implementation} $@ || \
243 > res="${res}lua_targets_${_lua_implementation}?,"
244
245 +=
246
247 > done
248 >
249 > echo "[${res%,}]"
250 > }
251 >
252 > _lua_atoms_samelib_generic() {
253 > eshopts_push -o noglob
254 > echo "LUATARGET? ("
255 > for token in $*; do
256 > case "$token" in
257 > "||" | "(" | ")" | *"?")
258
259 Magic sub-syntax? You've spent too much time with Arfrever. This is not
260 going to fly.
261
262 > echo "${token}"
263 > ;;
264 > *])
265 > echo "${token%[*}[LUATARGET,${token/*[}"
266 > #"]}" # <- kludge for vim's syntax highlighting engine to don't mess up all the things below this line
267 > ;;
268 > *)
269 > echo "${token}[LUATARGET]"
270 > ;;
271 > esac
272 > done
273 > echo ")"
274 > eshopts_pop
275 > }
276 >
277 > _lua_atoms_samelib() {
278 > local atoms=$(_lua_atoms_samelib_generic "$*")
279 >
280 > for _lua_implementation in $LUA_COMPAT; do
281 > echo "${atoms//LUATARGET/lua_targets_${_lua_implementation}}"
282 > done
283 > }
284 >
285 > _lua_wrap_conditions() {
286 > local conditions="$1"
287 > local atoms="$2"
288 >
289 > for condition in $conditions; do
290 > atoms="${condition}? ( ${atoms} )"
291 > done
292 >
293 > echo "$atoms"
294 > }
295 >
296 > # @FUNCTION: lua_add_rdepend
297 > # @USAGE: dependencies
298 > # @DESCRIPTION:
299 > # Adds the specified dependencies, with use condition(s) to RDEPEND,
300 > # taking the current set of lua targets into account. This makes sure
301 > # that all lua dependencies of the package are installed for the same
302 > # lua targets. Use this function for all lua dependencies instead of
303 > # setting RDEPEND yourself. The list of atoms uses the same syntax as
304 > # normal dependencies.
305
306 Do not attempt to parse PMS stuff manually. You're going to most likely
307 run into some terrible corner cases. Not to mention it's all going to
308 be ultra-inefficient. Like we need to spend even more time
309 in the global scope...
310
311 > #
312 > # Note: runtime dependencies are also added as build-time test
313 > # dependencies.
314 > lua_add_rdepend() {
315 > case $# in
316 > 1) ;;
317 > 2)
318 > [[ "${GENTOO_DEV}" == "yes" ]] && eqawarn "You can now use the usual syntax in lua_add_rdepend for $CATEGORY/$PF"
319
320 What's this now? Compatibility for API that was never public?
321
322 > lua_add_rdepend "$(_lua_wrap_conditions "$1" "$2")"
323 > return
324 > ;;
325 > *)
326 > die "bad number of arguments to $0"
327
328 To ebuild.sh? You're looking for FUNCNAME.
329
330 > ;;
331 > esac
332 >
333 > local dependency=$(_lua_atoms_samelib "$1")
334 >
335 > RDEPEND="${RDEPEND} $dependency"
336 >
337 > # Add the dependency as a test-dependency since we're going to
338 > # execute the code during test phase.
339 > DEPEND="${DEPEND} test? ( ${dependency} )"
340 > has test "$IUSE" || IUSE="${IUSE} test"
341
342 Do all lua packages always have tests? You seem to be doing a lot of
343 implicit assumptions here.
344
345 > }
346 >
347 > # @FUNCTION: lua_add_bdepend
348 > # @USAGE: dependencies
349 > # @DESCRIPTION:
350 > # Adds the specified dependencies, with use condition(s) to DEPEND,
351 > # taking the current set of lua targets into account. This makes sure
352 > # that all lua dependencies of the package are installed for the same
353 > # lua targets. Use this function for all lua dependencies instead of
354 > # setting DEPEND yourself. The list of atoms uses the same syntax as
355 > # normal dependencies.
356 > lua_add_bdepend() {
357 > case $# in
358 > 1) ;;
359 > 2)
360 > [[ "${GENTOO_DEV}" == "yes" ]] && eqawarn "You can now use the usual syntax in lua_add_bdepend for $CATEGORY/$PF"
361 > lua_add_bdepend "$(_lua_wrap_conditions "$1" "$2")"
362 > return
363 > ;;
364 > *)
365 > die "bad number of arguments to $0"
366 > ;;
367 > esac
368 >
369 > local dependency=$(_lua_atoms_samelib "$1")
370 >
371 > DEPEND="${DEPEND} $dependency"
372 > RDEPEND="${RDEPEND}"
373 > }
374 >
375 > # @FUNCTION: lua_get_use_implementations
376 > # @DESCRIPTION:
377 > # Gets an array of lua use targets enabled by the user
378 > lua_get_use_implementations() {
379 > local i implementation
380 > for implementation in ${LUA_COMPAT}; do
381 > use lua_targets_${implementation} && i+=" ${implementation}"
382 > done
383 > echo $i
384 > }
385 >
386 > # @FUNCTION: lua_get_use_targets
387 > # @DESCRIPTION:
388 > # Gets an array of lua use targets that the ebuild sets
389 > lua_get_use_targets() {
390 > local t implementation
391 > for implementation in ${LUA_COMPAT}; do
392 > t+=" lua_targets_${implementation}"
393 > done
394 > echo $t
395 > }
396 >
397 > # @FUNCTION: lua_implementations_depend
398 > # @RETURN: Dependencies suitable for injection into DEPEND and RDEPEND.
399 > # @DESCRIPTION:
400 > # Produces the dependency string for the various implementations of lua
401 > # which the package is being built against. This should not be used when
402 > # LUA_OPTIONAL is unset but must be used if LUA_OPTIONAL=yes. Do not
403 > # confuse this function with lua_implementation_depend().
404 > #
405 > # @EXAMPLE:
406 > # EAPI=5
407 > # LUA_OPTIONAL=yes
408 > #
409 > # inherit lua
410 > # ...
411 > # DEPEND="lua? ( $(lua_implementations_depend) )"
412 > # RDEPEND="${DEPEND}"
413 > lua_implementations_depend() {
414 > local depend
415 > for _lua_implementation in ${LUA_COMPAT}; do
416 > depend="${depend}${depend+ }lua_targets_${_lua_implementation}? ( $(lua_implementation_depend $_lua_implementation) )"
417 > done
418 > echo "${depend}"
419 > }
420 >
421 > IUSE+="$(lua_get_use_targets)"
422
423 Why +=? I don't see it set anywhere else.
424
425 > # If you specify LUA_OPTIONAL you also need to take care of
426 > # lua useflag and dependency.
427 > if [[ ${LUA_OPTIONAL} != yes ]]; then
428 > DEPEND="${DEPEND} $(lua_implementations_depend)"
429 > RDEPEND="${RDEPEND} $(lua_implementations_depend)"
430 > REQUIRED_USE+=" || ( $(lua_get_use_targets) )"
431 > fi
432 >
433 > _lua_invoke_environment() {
434 > old_S=${S}
435
436 Start using 'local' for local variables. You're polluting
437 the environment terribly.
438
439 > if [ -z "${LUA_S}" ]; then
440 > sub_S=${P}
441 > else
442 > sub_S=${LUA_S}
443
444 Wait, so LUA_S is supposed to be just the basename? Because with name
445 suggesting S variable almost every developer will put full path there...
446
447 > fi
448 >
449 > # Special case, for GitHub fetches of ancient packages. With this,
450 > # we allow the star glob to just expand to whatever directory it's
451 > # called.
452 > if [[ "${sub_S}" = *"*"* ]]; then
453 > pushd "${WORKDIR}"/all &>/dev/null
454 > sub_S=$(eval ls -d "${sub_S}" 2>/dev/null)
455
456 No. Don't use 'eval' as it is dangerous. Don't use 'ls' as it is meant
457 for user-readable output and not machines.
458
459 > popd &>/dev/null
460 > fi
461 >
462 > environment="${1}"; shift
463 >
464 > my_WORKDIR="${WORKDIR}"/"${environment}"
465 > S="${my_WORKDIR}"/"${sub_S}"
466 > EGIT_CHECKOUT_DIR="${S}"
467 > EHG_CHECKOUT_DIR="${S}"
468 > EBZR_UNPACK_DIR="${S}"
469 > BUILD_DIR="${S}"
470 > CMAKE_USE_DIR="${S}"
471
472 Find more eclasses to pollute the environment for!
473
474 > if [[ -d "${S}" ]]; then
475 > pushd "$S" &>/dev/null
476 > elif [[ -d "${my_WORKDIR}" ]]; then
477 > pushd "${my_WORKDIR}" &>/dev/null
478 > else
479 > pushd "${WORKDIR}" &>/dev/null
480 > fi
481
482 Err... what's this guessing game?
483
484 >
485 > ebegin "Running ${_PHASE:-${EBUILD_PHASE}} phase for $environment"
486 > "$@"
487 > popd &>/dev/null
488 >
489 > S=${old_S}
490 > }
491 >
492 > _lua_each_implementation() {
493 > local invoked=no
494 > for _lua_implementation in ${LUA_COMPAT}; do
495 > # only proceed if it's requested
496 > use lua_targets_${_lua_implementation} || continue
497 >
498 > LUA=$(lua_implementation_command ${_lua_implementation})
499 > TARGET=${_lua_implementation};
500 > lua_impl=$(basename ${LUA})
501 > invoked=yes
502 >
503 > if [[ -n "$1" ]]; then
504 > _lua_invoke_environment ${_lua_implementation} "$@"
505 > fi
506 >
507 > unset LUA TARGET lua_impl
508 > done
509 >
510 > if [[ ${invoked} == "no" ]]; then
511 > eerror "You need to select at least one compatible Lua installation target via LUA_TARGETS in make.conf."
512 > eerror "Compatible targets for this package are: ${LUA_COMPAT}"
513 > eerror
514 > die "No compatible Lua target selected."
515
516 How can this happen, REQUIRED_USE considered?
517
518 > fi
519 > }
520 >
521 > # @FUNCTION: lua_pkg_setup
522 > # @DESCRIPTION:
523 > # Check whether at least one lua target implementation is present.
524 > lua_pkg_setup() {
525 > # This only checks that at least one implementation is present
526 > # before doing anything; by leaving the parameters empty we know
527 > # it's a special case.
528 > _lua_each_implementation
529 > }
530
531 I'm sorry but I'm going to stop here. This eclass is probably the worst
532 eclass ever written for Gentoo. It looks like a terrible masterpiece
533 combination of base.eclass with python.eclass, that meant to solve some
534 minor Lua problem and instead grew into a monster that's got almost
535 nothing to do with Lua but instead aims to reinvent every other eclass
536 ever written.
537
538 Kill it with fire. Start over. Focus on Lua. Stop reinventing
539 everything else, and attempting to convert ebuilds into some terrible
540 openrc-class semi-broken declarative crap which attempts to guess what
541 the developer meant.
542
543 --
544 Best regards,
545 Michał Górny
546 <http://dev.gentoo.org/~mgorny/>

Replies

Subject Author
Re: [gentoo-dev] [RFC] lua.eclass "Vadim A. Misbakh-Soloviov" <gentoo@×××.name>