On Wed, 20 Jul 2016 20:36:20 +0700
"Vadim A. Misbakh-Soloviov" <gen...@mva.name> wrote:

> # Copyright 1999-2016 Gentoo Foundation
> # Distributed under the terms of the GNU General Public License v2
> 
> # @ECLASS: lua.eclass
> # @MAINTAINER:
> # mva <l...@mva.name>
> # @AUTHOR:
> # Author: Vadim A. Misbakh-Soloviov <l...@mva.name>
> # @BLURB: An eclass for installing Lua packages with proper support for 
> multiple Lua slots.
> # @DESCRIPTION:
> # The Lua eclass is designed to allow an easier installation of Lua packages
> # and their incorporation into the Gentoo Linux system.
> #
> # Currently available targets are:
> #  * lua51 - Lua (PUC-Rio) 5.1
> #  * lua52 - Lua (PUC-Rio) 5.2
> #  * lua53 - Lua (PUC-Rio) 5.3
> #  * luajit2 - LuaJIT 2.x
> #
> # This eclass does not define the implementation of the configure,
> # compile, test, or install phases. Instead, the default phases are
> # used.  Specific implementations of these phases can be provided in
> # the ebuild either to be run for each Lua implementation, or for all
> # Lua implementations, as follows:
> #
> #  * each_lua_configure
> #  * all_lua_configure
> 
> # @ECLASS-VARIABLE: LUA_COMPAT
> # @REQUIRED
> # @DESCRIPTION:
> # This variable contains a space separated list of targets (see above) a 
> package
> # is compatible to. It must be set before the `inherit' call.
> : ${LUA_COMPAT:=lua51 lua52 lua53 luajit2}

Am I missing something or are you not verifying the value anywhere?

Besides, are you really convinced this is a sane default? I don't know
bad Lua is these days but if you start with all impls on, adding a new
Lua version is going to be painful.

> 
> # @ECLASS-VARIABLE: LUA_PATCHES
> # @DEFAULT_UNSET
> # @DESCRIPTION:
> # A String or Array of filenames of patches to apply to all implementations.

Why do you need a special variable for it? Why not just let PATCHES do
their job?

> 
> # @ECLASS-VARIABLE: LUA_OPTIONAL
> # @DESCRIPTION:
> # Set the value to "yes" to make the dependency on a Lua interpreter
> # optional and then lua_implementations_depend() to help populate
> # DEPEND and RDEPEND.
> 
> # @ECLASS-VARIABLE: LUA_S
> # @DEFAULT_UNSET
> # @DESCRIPTION:
> # If defined this variable determines the source directory name after
> # unpacking. This defaults to the name of the package. Note that this
> # variable supports a wildcard mechanism to help with github tarballs
> # that contain the commit hash as part of the directory name.

1. New GitHub tarballs no longer have the commit hash (fetched using
the links provided nowadays).

2. Implicit wildcards are a very bad idea, and we already have
a dedicated eclass to handle this more safely.

3. How is that exactly related to Lua support?

> # @ECLASS-VARIABLE: LUA_QA_ALLOWED_LIBS
> # @DEFAULT_UNSET
> # @DESCRIPTION:
> # If defined this variable contains a whitelist of shared objects that
> # are allowed to exist even if they don't link to liblua. This avoids
> # the QA check that makes this mandatory. This is most likely not what
> # you are looking for if you get the related "Missing links" QA warning,
> # since the proper fix is almost always to make sure the shared object
> # is linked against liblua. There are cases were this is not the case
> # and the shared object is generic code to be used in some other way.
> # When set this argument is passed to "grep -E" to remove reporting of
> # these shared objects.
> 
> : ${GLOBAL_CFLAGS-${CFLAGS}}
> : ${GLOBAL_CXXFLAGS-${CXXFLAGS}}
> : ${GLOBAL_LDFLAGS-${LDFLAGS}}
> 
> : ${NOCCACHE-false}
> : ${NODISTCC-false}

Why are those variables not namespaced? You're just asking for trouble.
Not that I see their relevance to Lua but I'll probably complain later
on.

> [[ -n "${IS_MULTILIB}" ]] && multilib="multilib-minimal"

Lacks namespace, undescribed.

> case ${VCS} in
>       git)
>               VCS="git-r3"
>               ;;
>       hg)
>               VCS="mercurial"
>               ;;
>       svn)L
>               VCS="subversion"
>               ;;
> esac

Likewise. How is it exactly related to Lua? Why are you doing
all-in-one eclass? What's the problem with people sourcing the correct
eclass?

> [[ -n "${GITHUB_A}" && -n "${BITBUCKET_A}" ]] && die "Only one of GITHUB_A or 
> BITBUCKET_A should be set!"

Likewise. Completely unrelated to Lua support.

> if [[ -n "${GITHUB_A}" ]]; then
>       GITHUB_PN="${GITHUB_PN:-${PN}}"
>       EVCS_URI="https://github.com/${GITHUB_A}/${GITHUB_PN}";
>       DL="archive"
> elif [[ -n "${BITBUCKET_A}" ]]; then
>       BITBUCKET_PN="${BITBUCKET_PN:-${PN}}"
>       EVCS_URI="https://bitbucket.org/${BITBUCKET_A}/${BITBUCKET_PN}";
>       DL="get"
> fi
> if [[ -z "${EGIT_REPO_URI}" && -z "${EHG_REPO_URI}" && -z "${SRC_URI}" && -n 
> "${EVCS_URI}" ]]; then
>       if [[ "${VCS}" = git* ]]; then
>               EGIT_REPO_URI="${EVCS_URI}"
>       elif [[ "${VCS}" = "mercurial" ]]; then
>               EHG_REPO_URI="${EVCS_URI}"
>       elif [[ -z "${VCS}" && "${PV}" != *9999* ]]; then
>               SRC_URI="${EVCS_URI}/${DL}/${GITHUB_PV:-${PV}}.tar.gz -> 
> ${P}.tar.gz"

Why this terribly confusing implicit magic behavior? You're just asking
people to spend a lot of time looking wtf is happening, and why stuff
appears out of nowhere.

>       fi
> fi
> 
> inherit eutils ${multilib} toolchain-funcs flag-o-matic ${VCS}
> 
> EXPORT_FUNCTIONS src_unpack src_prepare src_configure src_compile src_install 
> pkg_setup src_test
> 
> case ${EAPI:-0} in
>       0|1|2|3)
>               die "Unsupported EAPI=${EAPI} (too old) for lua.eclass"
>               ;;
>       4|5|6)
>               # S is no longer automatically assigned when it doesn't exist.
>               S="${WORKDIR}"
>               ;;

It's a new eclass. Don't support old EAPIs, or you're going to go into
compatibility mess for EAPIs that are going to be barely used.

>       *)
>               ewarn "Unknown EAPI=${EAPI} for lua.eclass. Some things may 
> become broken"
>               ewarn "Please, review lua.eclass for compatibility with new 
> EAPI"

ewarn? That's not a good sign. Why should a user care that developer
committed ebuild without waiting for lua.eclass support first?

>               ;;
> esac
> 
> lua_implementation_depend() {
>       local lua_pn=
>       local lua_slot=
> 
>       case $1 in
>               lua51)
>                       lua_pn="dev-lang/lua"
>                       lua_slot=":5.1"
>                       ;;
>               lua52)
>                       lua_pn="dev-lang/lua"
>                       lua_slot=":5.2"
>                       ;;
>               lua53)
>                       lua_pn="dev-lang/lua"
>                       lua_slot=":5.3"
>                       ;;
>               luajit2)
>                       lua_pn="dev-lang/luajit"
>                       lua_slot=":2"
>                       ;;
>               *) die "$1: unknown Lua implementation"
>       esac
> 
>       echo "$2${lua_pn}$3${lua_slot}"
> }
> 
> # @FUNCTION: lua_implementation_command
> # @RETURN: the path to the given lua implementation
> # @DESCRIPTION:
> lua_implementation_command() {
>       local _lua_name=
>       local _lua_slotted=$(lua_implementation_depend $1)
>       _lua_name=${_lua_slotted//:}
> 
>       case $1 in
>               luajit*)
>                       _lua_name=${_lua_slotted/:/-}
>                       ;;
>       esac

This is confusing. Why not put both inside the case?

> 
>       local lua=$(readlink -fs $(type -p $(basename ${_lua_name} 
> 2>/dev/null)) 2>/dev/null)

readlink is not very portable. Canonicalization gives the strong
suggestion you're doing something nifty, relying on stuff you're not
supposed to rely on and creating code that's going to easily go kaboom.

>       [[ -x ${lua} ]] || die "Unable to locate executable Lua interpreter"
>       echo "${lua}"
> }
> 
> # @FUNCTION: lua_samelib
> # @RETURN: use flag string with current lua implementations

The function name doesn't suggest any relevance to USE flags.

> # @DESCRIPTION:
> # Convenience function to output the use dependency part of a
> # dependency. Used as a building block for lua_add_rdepend() and
> # lua_add_bdepend(), but may also be useful in an ebuild to specify
> # more complex dependencies.
> lua_samelib() {
>       local res=
>       for _lua_implementation in $LUA_COMPAT; do
>               has -${_lua_implementation} $@ || \
>                       res="${res}lua_targets_${_lua_implementation}?,"

+=

>       done
> 
>       echo "[${res%,}]"
> }
> 
> _lua_atoms_samelib_generic() {
>       eshopts_push -o noglob
>       echo "LUATARGET? ("
>       for token in $*; do
>               case "$token" in
>                       "||" | "(" | ")" | *"?")

Magic sub-syntax? You've spent too much time with Arfrever. This is not
going to fly.

>                               echo "${token}"
>                               ;;
>                       *])
>                               echo "${token%[*}[LUATARGET,${token/*[}"
>                               #"]}" # <- kludge for vim's syntax highlighting 
> engine to don't mess up all the things below this line
>                               ;;
>                       *)
>                               echo "${token}[LUATARGET]"
>                               ;;
>               esac
>       done
>       echo ")"
>       eshopts_pop
> }
> 
> _lua_atoms_samelib() {
>       local atoms=$(_lua_atoms_samelib_generic "$*")
> 
>       for _lua_implementation in $LUA_COMPAT; do
>               echo "${atoms//LUATARGET/lua_targets_${_lua_implementation}}"
>       done
> }
> 
> _lua_wrap_conditions() {
>       local conditions="$1"
>       local atoms="$2"
> 
>       for condition in $conditions; do
>               atoms="${condition}? ( ${atoms} )"
>       done
> 
>       echo "$atoms"
> }
> 
> # @FUNCTION: lua_add_rdepend
> # @USAGE: dependencies
> # @DESCRIPTION:
> # Adds the specified dependencies, with use condition(s) to RDEPEND,
> # taking the current set of lua targets into account. This makes sure
> # that all lua dependencies of the package are installed for the same
> # lua targets. Use this function for all lua dependencies instead of
> # setting RDEPEND yourself. The list of atoms uses the same syntax as
> # normal dependencies.

Do not attempt to parse PMS stuff manually. You're going to most likely
run into some terrible corner cases. Not to mention it's all going to
be ultra-inefficient. Like we need to spend even more time
in the global scope...

> #
> # Note: runtime dependencies are also added as build-time test
> # dependencies.
> lua_add_rdepend() {
>       case $# in
>               1) ;;
>               2)
>                       [[ "${GENTOO_DEV}" == "yes" ]] && eqawarn "You can now 
> use the usual syntax in lua_add_rdepend for $CATEGORY/$PF"

What's this now? Compatibility for API that was never public?

>                       lua_add_rdepend "$(_lua_wrap_conditions "$1" "$2")"
>                       return
>                       ;;
>               *)
>                       die "bad number of arguments to $0"

To ebuild.sh? You're looking for FUNCNAME.

>                       ;;
>       esac
> 
>       local dependency=$(_lua_atoms_samelib "$1")
> 
>       RDEPEND="${RDEPEND} $dependency"
> 
>       # Add the dependency as a test-dependency since we're going to
>       # execute the code during test phase.
>       DEPEND="${DEPEND} test? ( ${dependency} )"
>       has test "$IUSE" || IUSE="${IUSE} test"

Do all lua packages always have tests? You seem to be doing a lot of
implicit assumptions here.

> }
> 
> # @FUNCTION: lua_add_bdepend
> # @USAGE: dependencies
> # @DESCRIPTION:
> # Adds the specified dependencies, with use condition(s) to DEPEND,
> # taking the current set of lua targets into account. This makes sure
> # that all lua dependencies of the package are installed for the same
> # lua targets. Use this function for all lua dependencies instead of
> # setting DEPEND yourself. The list of atoms uses the same syntax as
> # normal dependencies.
> lua_add_bdepend() {
>       case $# in
>               1) ;;
>               2)
>                       [[ "${GENTOO_DEV}" == "yes" ]] && eqawarn "You can now 
> use the usual syntax in lua_add_bdepend for $CATEGORY/$PF"
>                       lua_add_bdepend "$(_lua_wrap_conditions "$1" "$2")"
>                       return
>                       ;;
>               *)
>                       die "bad number of arguments to $0"
>                       ;;
>       esac
> 
>       local dependency=$(_lua_atoms_samelib "$1")
> 
>       DEPEND="${DEPEND} $dependency"
>       RDEPEND="${RDEPEND}"
> }
> 
> # @FUNCTION: lua_get_use_implementations
> # @DESCRIPTION:
> # Gets an array of lua use targets enabled by the user
> lua_get_use_implementations() {
>       local i implementation
>       for implementation in ${LUA_COMPAT}; do
>               use lua_targets_${implementation} && i+=" ${implementation}"
>       done
>       echo $i
> }
> 
> # @FUNCTION: lua_get_use_targets
> # @DESCRIPTION:
> # Gets an array of lua use targets that the ebuild sets
> lua_get_use_targets() {
>       local t implementation
>       for implementation in ${LUA_COMPAT}; do
>               t+=" lua_targets_${implementation}"
>       done
>       echo $t
> }
> 
> # @FUNCTION: lua_implementations_depend
> # @RETURN: Dependencies suitable for injection into DEPEND and RDEPEND.
> # @DESCRIPTION:
> # Produces the dependency string for the various implementations of lua
> # which the package is being built against. This should not be used when
> # LUA_OPTIONAL is unset but must be used if LUA_OPTIONAL=yes. Do not
> # confuse this function with lua_implementation_depend().
> #
> # @EXAMPLE:
> # EAPI=5
> # LUA_OPTIONAL=yes
> #
> # inherit lua
> # ...
> # DEPEND="lua? ( $(lua_implementations_depend) )"
> # RDEPEND="${DEPEND}"
> lua_implementations_depend() {
>       local depend
>       for _lua_implementation in ${LUA_COMPAT}; do
>               depend="${depend}${depend+ }lua_targets_${_lua_implementation}? 
> ( $(lua_implementation_depend $_lua_implementation) )"
>       done
>       echo "${depend}"
> }
> 
> IUSE+="$(lua_get_use_targets)"

Why +=? I don't see it set anywhere else.

> # If you specify LUA_OPTIONAL you also need to take care of
> # lua useflag and dependency.
> if [[ ${LUA_OPTIONAL} != yes ]]; then
>       DEPEND="${DEPEND} $(lua_implementations_depend)"
>       RDEPEND="${RDEPEND} $(lua_implementations_depend)"
>       REQUIRED_USE+=" || ( $(lua_get_use_targets) )"
> fi
> 
> _lua_invoke_environment() {
>       old_S=${S}

Start using 'local' for local variables. You're polluting
the environment terribly.

>       if [ -z "${LUA_S}" ]; then
>               sub_S=${P}
>       else
>               sub_S=${LUA_S}

Wait, so LUA_S is supposed to be just the basename? Because with name
suggesting S variable almost every developer will put full path there...

>       fi
> 
>       # Special case, for GitHub fetches of ancient packages. With this,
>       # we allow the star glob to just expand to whatever directory it's
>       # called.
>       if [[ "${sub_S}" = *"*"* ]]; then
>               pushd "${WORKDIR}"/all &>/dev/null
>               sub_S=$(eval ls -d "${sub_S}" 2>/dev/null)

No. Don't use 'eval' as it is dangerous. Don't use 'ls' as it is meant
for user-readable output and not machines.

>               popd &>/dev/null
>       fi
> 
>       environment="${1}"; shift
> 
>       my_WORKDIR="${WORKDIR}"/"${environment}"
>       S="${my_WORKDIR}"/"${sub_S}"
>       EGIT_CHECKOUT_DIR="${S}"
>       EHG_CHECKOUT_DIR="${S}"
>       EBZR_UNPACK_DIR="${S}"
>       BUILD_DIR="${S}"
>       CMAKE_USE_DIR="${S}"

Find more eclasses to pollute the environment for!

>       if [[ -d "${S}" ]]; then
>               pushd "$S" &>/dev/null
>       elif [[ -d "${my_WORKDIR}" ]]; then
>               pushd "${my_WORKDIR}" &>/dev/null
>       else
>               pushd "${WORKDIR}" &>/dev/null
>       fi

Err... what's this guessing game?

> 
>       ebegin "Running ${_PHASE:-${EBUILD_PHASE}} phase for $environment"
>       "$@"
>       popd &>/dev/null
> 
>       S=${old_S}
> }
> 
> _lua_each_implementation() {
>       local invoked=no
>       for _lua_implementation in ${LUA_COMPAT}; do
>               # only proceed if it's requested
>               use lua_targets_${_lua_implementation} || continue
> 
>               LUA=$(lua_implementation_command ${_lua_implementation})
>               TARGET=${_lua_implementation};
>               lua_impl=$(basename ${LUA})
>               invoked=yes
> 
>               if [[ -n "$1" ]]; then
>                       _lua_invoke_environment ${_lua_implementation} "$@"
>               fi
> 
>               unset LUA TARGET lua_impl
>       done
> 
>       if [[ ${invoked} == "no" ]]; then
>               eerror "You need to select at least one compatible Lua 
> installation target via LUA_TARGETS in make.conf."
>               eerror "Compatible targets for this package are: ${LUA_COMPAT}"
>               eerror
>               die "No compatible Lua target selected."

How can this happen, REQUIRED_USE considered?

>       fi
> }
> 
> # @FUNCTION: lua_pkg_setup
> # @DESCRIPTION:
> # Check whether at least one lua target implementation is present.
> lua_pkg_setup() {
>       # This only checks that at least one implementation is present
>       # before doing anything; by leaving the parameters empty we know
>       # it's a special case.
>       _lua_each_implementation
> }

I'm sorry but I'm going to stop here. This eclass is probably the worst
eclass ever written for Gentoo. It looks like a terrible masterpiece
combination of base.eclass with python.eclass, that meant to solve some
minor Lua problem and instead grew into a monster that's got almost
nothing to do with Lua but instead aims to reinvent every other eclass
ever written.

Kill it with fire. Start over. Focus on Lua. Stop reinventing
everything else, and attempting to convert ebuilds into some terrible
openrc-class semi-broken declarative crap which attempts to guess what
the developer meant.

-- 
Best regards,
Michał Górny
<http://dev.gentoo.org/~mgorny/>

Attachment: pgpubp7cYDvjE.pgp
Description: OpenPGP digital signature

Reply via email to