Re: [gentoo-dev] eclass/vdr-plugin-2.eclass EAPI=6 changes, plz review

["Michał Górny" <mgorny@gentoo.org>]

Dnia 16 maja 2016 11:39:23 CEST, Joerg Bornkessel <hd_brummy@gentoo.org> napisał(a):
>Hallo,
>after my last commit disaster,
>i bring my changes to review before i break some things again.
>
>- Added changes to make it work with eapi=6
>- removed some unneeded code parts (never they was used in any
>ebuilds, i though they was integrated to make the eclass more
>flexibel,...)

After reading this I don't see anything breakingly wrong, though I haven't tested it.

However, a few general suggestions:

1. Split this into multiple commits. Generic cleanup should happen separately from EAPI 6 support. And it would probably be a good idea to remove features one at a time, so that reverting would be possible.

2. Prefer logic that evaluates to true for future EAPIs. For example, instead of [[ ${EAPI} == 6 ]], it's better to say != [012345], so that we won't have to extend the list with every new EAPI if nothing changes.

3. Please namespace the eclass functions. Using generic names is really asking for trouble. vdr_ prefix should be sufficient.

4. Please kill that src_util helper. It makes the logic terribly hard to follow, and there's really no reason to use case over separate functions.

5. There are some missing || die, for example after cd.

I'm sorry i can't really to specific parts of the eclass but I'm replying from a phone.

>
><snipp .diff>
>-- vdr-plugin-2.eclass 2016-05-15 22:03:21.807417485 +0200
>+++ vdr-plugin-2.eclass.new 2016-05-15 22:01:10.000000000 +0200
>@@ -1,4 +1,4 @@
>-# Copyright 1999-2015 Gentoo Foundation
>+# Copyright 1999-2016 Gentoo Foundation
> # Distributed under the terms of the GNU General Public License v2
> # $Id$
>
>@@ -90,7 +90,7 @@
> # @CODE
>
> # Applying your own local/user patches:
>-# This is done by using the
>+# This is done by using the
> # (EAPI = 4,5) epatch_user() function of the eutils.eclass,
> # (EAPI = 6) eapply_user function integrated in EAPI = 6.
> # Simply add your patches into one of these directories:
>@@ -104,10 +104,7 @@
> inherit flag-o-matic toolchain-funcs unpacker
>
> case ${EAPI:-0} in
>-   4|5)
>-   ;;
>-   6)
>-   ewarn "EAPI 6 support for test purpose only, plz report bugs to
>vdr@gentoo.org"
>+   4|5|6)
>    ;;
>    *) die "EAPI ${EAPI} unsupported."
>    ;;
>@@ -156,6 +153,7 @@
> #      EBUILD=${CATEGORY}/${PN}
> #      EBUILD_V=${PVR}
> #  EOT
>+#  obsolet? fix me later...
>    {
>        echo "VDRPLUGIN_DB=1"
>        echo "CREATOR=ECLASS"
>@@ -232,6 +230,7 @@
>    #sed -i Makefile \
>    #   -e "s:^DVBDIR.*$:DVBDIR = ${DVB_INCLUDE_DIR}:" \
>    #   -e 's:-I$(DVBDIR)/include:-I$(DVBDIR):'
>+   # obsolet? fix me later...
>
>    if ! grep -q APIVERSION Makefile; then
>        ebegin "  Converting to APIVERSION"
>@@ -289,7 +288,7 @@
> linguas_support() {
> #  Patching Makefile for linguas support.
>#  Only locales, enabled through the LINGUAS (make.conf) variable will
>be
>-#  "compiled" and installed.
>+#  compiled and installed.
>
>    einfo "Patching for Linguas support"
>    einfo "available Languages for ${P} are:"
>@@ -311,12 +310,9 @@
> vdr_i18n() {
> #  i18n handling was deprecated since >=media-video/vdr-1.5.9,
> #  finally with >=media-video/vdr-1.7.27 it has been dropped entirely
>and some
>-#  plugins will fail to "compile" because they're still using the old
>variant.
>+#  plugins will fail to compile because they're still using the old
>variant.
> #  Simply remove the i18n.o object from Makefile (OBJECT) and
> #  remove "static const tI18nPhrase*" from i18n.h.
>-#
>-#  Plugins that are still using the old method will be pmasked until
>they're
>-#  fixed or in case of maintainer timeout they'll be masked for
>removal.
>
>    gettext_missing
>
>@@ -391,6 +387,7 @@
>
>    # Plugins need to be compiled with position independent code,
>otherwise linking
>    # VDR against it will fail
>+   # depricated if fi, as we have only >=vdr-2 in the tree, fix me
>later...
>    if has_version ">=media-video/vdr-1.7.13"; then
>        append-cxxflags -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
>-D_LARGEFILE64_SOURCE
>    fi
>@@ -500,7 +497,9 @@
>        die "vdr-plugin-2_src_prepare not called!"
>    fi
>
>-   [[ ${PATCHES[@]} ]] && epatch "${PATCHES[@]}"
>+   [[ ${EAPI} == [45] ]] && [[ ${PATCHES[@]} ]] && epatch
>"${PATCHES[@]}"
>+   [[ ${EAPI} == "6" ]] && [[ ${PATCHES[@]} ]] && eapply
>"${PATCHES[@]}"
>+
>    debug-print "$FUNCNAME: applying user patches"
>
>    vdr-plugin-2_src_util prepare
>@@ -522,14 +521,12 @@
>            fi
>            cd "${S}"
>
>-           BUILD_TARGETS=${BUILD_TARGETS:-${VDRPLUGIN_MAKE_TARGET:-all
>}}
>-               emake ${BUILD_PARAMS} \
>-                   ${BUILD_TARGETS} \
>-                   LOCALEDIR="${TMP_LOCALE_DIR}" \
>-                   LOCDIR="${TMP_LOCALE_DIR}" \
>-                   LIBDIR="${S}" \
>-                   TMPDIR="${T}" \
>-                   || die "emake failed"
>+           emake all ${BUILD_PARAMS} \
>+               LOCALEDIR="${TMP_LOCALE_DIR}" \
>+               LOCDIR="${TMP_LOCALE_DIR}" \
>+               LIBDIR="${S}" \
>+               TMPDIR="${T}" \
>+               || die "emake all failed"
>            ;;
>        esac
>
>@@ -570,12 +567,11 @@
>
>    local SOFILE_STRING=$(grep SOFILE Makefile)
>    if [[ -n ${SOFILE_STRING} ]]; then
>-       BUILD_TARGETS=${BUILD_TARGETS:-${VDRPLUGIN_MAKE_TARGET:-install
>}}
>-       einstall ${BUILD_PARAMS} \
>-           ${BUILD_TARGETS} \
>-           TMPDIR="${T}" \
>-           DESTDIR="${D}" \
>-           || die "einstall (makefile target) failed"
>+       emake install \
>+       ${BUILD_PARAMS} \
>+       TMPDIR="${T}" \
>+       DESTDIR="${D}" \
>+       || die "emake install (makefile target) failed"
>    else
>        dev_check "Plugin use still the old Makefile handling"
>        insinto "${VDR_PLUGIN_DIR}"
>@@ -609,9 +605,14 @@
>    create_header_checksum_file ${vdr_plugin_list}
>    create_plugindb_file ${vdr_plugin_list}
>
>-   local docfile
>-   for docfile in README* HISTORY CHANGELOG; do
>-       [[ -f ${docfile} ]] && dodoc ${docfile}
>+   local commondoc=( README* HISTORY CHANGELOG )
>+   for docfile in "${commondoc[@]}"; do
>+       if [[ ${EAPI} == "6" ]]; then
>+           local DOCS="${DOCS} ${docfile}"
>+           [[ -f ${docfile} ]] && einstalldocs "${DOCS[@]}"
>+       else
>+           [[ -f ${docfile} ]] && dodoc ${docfile}
>+       fi
>    done
>
>    # if VDR_CONFD_FILE is empty and ${FILESDIR}/confd exists take it
></snapp>
>
>Attached:
>vdr-plugin-2.eclass
>vdr-plugin-2.eclass.diff
>vdr-plugin-2.eclass.new
>
>
>Cheers
>/dev/joerg


-- 
Best regards,
Michał Górny (by phone)