[gentoo-dev] [PATCH] freedict.eclass: rename dict.eclass, generalize

[gentoo-dev] [PATCH] freedict.eclass: rename dict.eclass, generalize

[Marty E. Plummer]

Reworked to be usable with app-dicts/dictd-* ebuilds to avoid code
duplication

You can reference this pull request to get an idea as to the usage.

https://github.com/gentoo/gentoo/pull/8106

Package-Manager: Portage-2.3.31, Repoman-2.3.9
---
 eclass/dict.eclass     | 75 ++++++++++++++++++++++++++++++++++++++++++
 eclass/freedict.eclass | 51 ----------------------------
 2 files changed, 75 insertions(+), 51 deletions(-)
 create mode 100644 eclass/dict.eclass
 delete mode 100644 eclass/freedict.eclass

diff --git a/eclass/dict.eclass b/eclass/dict.eclass
new file mode 100644
index 00000000000..8d523e3863e
--- /dev/null
+++ b/eclass/dict.eclass
@@ -0,0 +1,75 @@
+# Copyright 1999-2018 Gentoo Foundation
+# Distributed under the terms of the GNU General Public License v2
+
+# @ECLASS: dict.eclass
+# @MAINTAINER:
+# maintainer-needed@gentoo.org
+# @AUTHOR:
+# Original author: Seemant Kulleen
+# @BLURB: Ease the installation of dict and freedict translation dictionaries
+# @DESCRIPTION:
+# This eclass exists to ease the installation of dictd and freedictd translation
+# dictionaries.  The only variables which need to be defined in the actual
+# ebuilds are FORLANG and TOLANG for the source and target languages,
+# respectively, and DICTS if the package ships more than one dictionary
+# and cannot be determined from ${PN}.
+
+# @ECLASS-VARIABLE: DICTS
+# @DESCRIPTION:
+# Array of dictionary files (foo.dict.dz and foo.index) to be installed during
+# dict_src_install.
+
+# @ECLASS-VARIABLE: FORLANG
+# @DESCRIPTION:
+# Please see above for a description.
+
+# @ECLASS-VARIABLE: TOLANG
+# @DESCRIPTION:
+# Please see above for a description.
+
+if [[ -z ${_DICT_ECLASS} ]]; then
+_DICT_ECLASS=1
+
+case ${EAPI:-0} in
+	6) ;;
+	*) die "${ECLASS}.eclass is banned in EAPI=${EAPI}" ;;
+esac
+
+if [[ ${PN} == *freedict-* ]]; then
+	MY_P=${PN/freedict-/}
+	DICTS=( ${MY_P} )
+
+	DESCRIPTION="Freedict for language translation from ${FORLANG} to ${TOLANG}"
+	HOMEPAGE="http://freedict.sourceforge.net/"
+	SRC_URI="http://freedict.sourceforge.net/download/linux/${MY_P}.tar.gz"
+elif [[ ${PN} == *dictd-* ]]; then
+	MY_P=${PN/dictd-/}
+	DICTS=( ${MY_P} )
+
+	DESCRIPTION="${MY_P} dictionary for dictd"
+	HOMEPAGE="http://www.dict.org/"
+fi
+
+LICENSE="GPL-2+"
+SLOT="0"
+IUSE=""
+
+RDEPEND="app-text/dictd"
+
+S="${WORKDIR}"
+
+# @FUNCTION: dict_src_install
+# @DESCRIPTION:
+# The freedict src_install function, which is exported
+dict_src_install() {
+	insinto /usr/$(get_libdir)/dict
+	for dict in "${DICTS[@]}"; do
+		doins ${dict}.dict.dz
+		doins ${dict}.index
+	done
+	einstalldocs
+}
+
+EXPORT_FUNCTIONS src_install
+
+fi
diff --git a/eclass/freedict.eclass b/eclass/freedict.eclass
deleted file mode 100644
index b795f53f954..00000000000
--- a/eclass/freedict.eclass
+++ /dev/null
@@ -1,51 +0,0 @@
-# Copyright 1999-2018 Gentoo Foundation
-# Distributed under the terms of the GNU General Public License v2
-
-# @ECLASS: freedict.eclass
-# @MAINTAINER:
-# maintainer-needed@gentoo.org
-# @AUTHOR:
-# Original author: Seemant Kulleen
-# @BLURB: Ease the installation of freedict translation dictionaries
-# @DESCRIPTION:
-# This eclass exists to ease the installation of freedict translation
-# dictionaries.  The only variables which need to be defined in the actual
-# ebuilds are FORLANG and TOLANG for the source and target languages,
-# respectively.
-
-# @ECLASS-VARIABLE: FORLANG
-# @DESCRIPTION:
-# Please see above for a description.
-
-# @ECLASS-VARIABLE: TOLANG
-# @DESCRIPTION:
-# Please see above for a description.
-
-case ${EAPI:-0} in
-	6) ;;
-	*) die "${ECLASS}.eclass is banned in EAPI=${EAPI}" ;;
-esac
-
-MY_P=${PN/freedict-/}
-
-DESCRIPTION="Freedict for language translation from ${FORLANG} to ${TOLANG}"
-HOMEPAGE="http://freedict.sourceforge.net/"
-SRC_URI="http://freedict.sourceforge.net/download/linux/${MY_P}.tar.gz"
-
-LICENSE="GPL-2+"
-SLOT="0"
-
-RDEPEND="app-text/dictd"
-
-S="${WORKDIR}"
-
-# @FUNCTION: freedict_src_install
-# @DESCRIPTION:
-# The freedict src_install function, which is exported
-freedict_src_install() {
-	insinto /usr/$(get_libdir)/dict
-	doins ${MY_P}.dict.dz
-	doins ${MY_P}.index
-}
-
-EXPORT_FUNCTIONS src_install
-- 
2.17.0

Re: [gentoo-dev] [PATCH] freedict.eclass: rename dict.eclass, generalize

[Marty E. Plummer]

On Tue, Apr 24, 2018 at 11:03:44PM -0500, Marty E. Plummer wrote:
> Reworked to be usable with app-dicts/dictd-* ebuilds to avoid code
> duplication
> 
> You can reference this pull request to get an idea as to the usage.
> 
> https://github.com/gentoo/gentoo/pull/8106
> 
> Package-Manager: Portage-2.3.31, Repoman-2.3.9
Actually, looking into the dictd.confd it looks like that needs to be
modified a bit to be able to go ahead with this sort of change. Thoughts
and comments? Its my understanding that noarch flat data should go into
/lib & /usr/lib using the new abi which 17.1 uses... could be that
freedict.eclass as it exists right now is and has been wrong regarding
where the dictionaries get installed to...

Re: [gentoo-dev] [PATCH] freedict.eclass: rename dict.eclass, generalize

[Ulrich Mueller]

>>>>> On Tue, 24 Apr 2018, Marty E Plummer wrote:

> Reworked to be usable with app-dicts/dictd-* ebuilds to avoid code
> duplication

I don't see much code duplication there, so I think it would be
cleaner to have a second eclass, rather than adding conditionals to
the existing one.

> You can reference this pull request to get an idea as to the usage.

> https://github.com/gentoo/gentoo/pull/8106

> Package-Manager: Portage-2.3.31, Repoman-2.3.9
> ---
>  eclass/dict.eclass     | 75 ++++++++++++++++++++++++++++++++++++++++++
>  eclass/freedict.eclass | 51 ----------------------------
>  2 files changed, 75 insertions(+), 51 deletions(-)
>  create mode 100644 eclass/dict.eclass
>  delete mode 100644 eclass/freedict.eclass
> diff --git a/eclass/dict.eclass b/eclass/dict.eclass
> new file mode 100644
> index 00000000000..8d523e3863e
> --- /dev/null
> +++ b/eclass/dict.eclass
> @@ -0,0 +1,75 @@
> +# Copyright 1999-2018 Gentoo Foundation
> +# Distributed under the terms of the GNU General Public License v2
> +
> +# @ECLASS: dict.eclass
> +# @MAINTAINER:
> +# maintainer-needed@gentoo.org
> +# @AUTHOR:
> +# Original author: Seemant Kulleen
> +# @BLURB: Ease the installation of dict and freedict translation dictionaries
> +# @DESCRIPTION:
> +# This eclass exists to ease the installation of dictd and freedictd translation
> +# dictionaries.  The only variables which need to be defined in the actual
> +# ebuilds are FORLANG and TOLANG for the source and target languages,
> +# respectively, and DICTS if the package ships more than one dictionary
> +# and cannot be determined from ${PN}.
> +
> +# @ECLASS-VARIABLE: DICTS
> +# @DESCRIPTION:
> +# Array of dictionary files (foo.dict.dz and foo.index) to be installed during
> +# dict_src_install.
> +
> +# @ECLASS-VARIABLE: FORLANG
> +# @DESCRIPTION:
> +# Please see above for a description.
> +
> +# @ECLASS-VARIABLE: TOLANG
> +# @DESCRIPTION:
> +# Please see above for a description.
> +
> +if [[ -z ${_DICT_ECLASS} ]]; then
> +_DICT_ECLASS=1

Unless dict.eclass is inherited by another eclass, this is not needed
and will only unnecessarily add a variable to the environment.

> +
> +case ${EAPI:-0} in
> +	6) ;;
> +	*) die "${ECLASS}.eclass is banned in EAPI=${EAPI}" ;;
> +esac
> +
> +if [[ ${PN} == *freedict-* ]]; then
> +	MY_P=${PN/freedict-/}
> +	DICTS=( ${MY_P} )
> +
> +	DESCRIPTION="Freedict for language translation from ${FORLANG} to ${TOLANG}"
> +	HOMEPAGE="http://freedict.sourceforge.net/"
> +	SRC_URI="http://freedict.sourceforge.net/download/linux/${MY_P}.tar.gz"
> +elif [[ ${PN} == *dictd-* ]]; then
> +	MY_P=${PN/dictd-/}
> +	DICTS=( ${MY_P} )
> +
> +	DESCRIPTION="${MY_P} dictionary for dictd"
> +	HOMEPAGE="http://www.dict.org/"
> +fi
> +
> +LICENSE="GPL-2+"
> +SLOT="0"
> +IUSE=""

Why this empty assignment? Please remove.

> +
> +RDEPEND="app-text/dictd"
> +
> +S="${WORKDIR}"
> +
> +# @FUNCTION: dict_src_install
> +# @DESCRIPTION:
> +# The freedict src_install function, which is exported
> +dict_src_install() {
> +	insinto /usr/$(get_libdir)/dict
> +	for dict in "${DICTS[@]}"; do

Local variable declaration for dict is missing (but see below).

> +		doins ${dict}.dict.dz
> +		doins ${dict}.index
> +	done

The loop is not needed, because doins accepts several arguments.
For example:
doins "${DICTS[@]/%/.dict.dz}"

> +	einstalldocs
> +}
> +
> +EXPORT_FUNCTIONS src_install
> +
> +fi

Re: [gentoo-dev] [PATCH] freedict.eclass: rename dict.eclass, generalize

[Marty E. Plummer]

On Wed, Apr 25, 2018 at 07:28:12AM +0200, Ulrich Mueller wrote:
> >>>>> On Tue, 24 Apr 2018, Marty E Plummer wrote:
> 
> > Reworked to be usable with app-dicts/dictd-* ebuilds to avoid code
> > duplication
> 
> I don't see much code duplication there, so I think it would be
> cleaner to have a second eclass, rather than adding conditionals to
> the existing one.
> 
I mean if you take into account app-dicts/dictd-* and freedict.eclass;
without the above pr (which I did originally have with a new eclass,
until I realized that app-text/dictd's conf file hardcodes /usr/lib/dict
anyways so the freedict dictionaries get [at least on amd64 and other
targets which use/will use lib64] installed outside of the search path
anyways) they are all almost entirely identical with the contents of
freedict.eclass.

Another thought I had was moving the src_install into dict.eclass and
have freedict inherit it, adding the differences.

Re: [gentoo-dev] [PATCH] freedict.eclass: rename dict.eclass, generalize

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

>>>>> On Wed, 25 Apr 2018, Marty E Plummer wrote:

>> I don't see much code duplication there, so I think it would be
>> cleaner to have a second eclass, rather than adding conditionals to
>> the existing one.

> I mean if you take into account app-dicts/dictd-* and freedict.eclass;
> without the above pr (which I did originally have with a new eclass,
> until I realized that app-text/dictd's conf file hardcodes /usr/lib/dict
> anyways so the freedict dictionaries get [at least on amd64 and other
> targets which use/will use lib64] installed outside of the search path
> anyways) they are all almost entirely identical with the contents of
> freedict.eclass.

> Another thought I had was moving the src_install into dict.eclass and
> have freedict inherit it, adding the differences.

$ egrep -v '^(#|$)' freedict.eclass | wc -l
18

IMHO not worth the effort for 18 lines of code. Keep it simple.

Ulrich

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [gentoo-dev] [PATCH] freedict.eclass: rename dict.eclass, generalize

[Michał Górny]

W dniu wto, 24.04.2018 o godzinie 23∶03 -0500, użytkownik Marty E.
Plummer napisał:
> Reworked to be usable with app-dicts/dictd-* ebuilds to avoid code
> duplication
> 
> You can reference this pull request to get an idea as to the usage.
> 
> https://github.com/gentoo/gentoo/pull/8106
> 
> Package-Manager: Portage-2.3.31, Repoman-2.3.9
> ---
>  eclass/dict.eclass     | 75 ++++++++++++++++++++++++++++++++++++++++++
>  eclass/freedict.eclass | 51 ----------------------------
>  2 files changed, 75 insertions(+), 51 deletions(-)
>  create mode 100644 eclass/dict.eclass
>  delete mode 100644 eclass/freedict.eclass
> 
> diff --git a/eclass/dict.eclass b/eclass/dict.eclass
> new file mode 100644
> index 00000000000..8d523e3863e
> --- /dev/null
> +++ b/eclass/dict.eclass
> @@ -0,0 +1,75 @@
> +# Copyright 1999-2018 Gentoo Foundation
> +# Distributed under the terms of the GNU General Public License v2
> +
> +# @ECLASS: dict.eclass
> +# @MAINTAINER:
> +# maintainer-needed@gentoo.org
> +# @AUTHOR:
> +# Original author: Seemant Kulleen
> +# @BLURB: Ease the installation of dict and freedict translation dictionaries
> +# @DESCRIPTION:
> +# This eclass exists to ease the installation of dictd and freedictd translation
> +# dictionaries.  The only variables which need to be defined in the actual
> +# ebuilds are FORLANG and TOLANG for the source and target languages,
> +# respectively, and DICTS if the package ships more than one dictionary
> +# and cannot be determined from ${PN}.
> +
> +# @ECLASS-VARIABLE: DICTS
> +# @DESCRIPTION:
> +# Array of dictionary files (foo.dict.dz and foo.index) to be installed during
> +# dict_src_install.
> +
> +# @ECLASS-VARIABLE: FORLANG
> +# @DESCRIPTION:
> +# Please see above for a description.
> +
> +# @ECLASS-VARIABLE: TOLANG
> +# @DESCRIPTION:
> +# Please see above for a description.

I don't see those two being used anywhere.

> +
> +if [[ -z ${_DICT_ECLASS} ]]; then
> +_DICT_ECLASS=1
> +
> +case ${EAPI:-0} in
> +	6) ;;
> +	*) die "${ECLASS}.eclass is banned in EAPI=${EAPI}" ;;
> +esac
> +
> +if [[ ${PN} == *freedict-* ]]; then
> +	MY_P=${PN/freedict-/}
> +	DICTS=( ${MY_P} )
> +
> +	DESCRIPTION="Freedict for language translation from ${FORLANG} to ${TOLANG}"
> +	HOMEPAGE="http://freedict.sourceforge.net/"
> +	SRC_URI="http://freedict.sourceforge.net/download/linux/${MY_P}.tar.gz"
> +elif [[ ${PN} == *dictd-* ]]; then
> +	MY_P=${PN/dictd-/}
> +	DICTS=( ${MY_P} )
> +
> +	DESCRIPTION="${MY_P} dictionary for dictd"
> +	HOMEPAGE="http://www.dict.org/"
> +fi
> +
> +LICENSE="GPL-2+"
> +SLOT="0"
> +IUSE=""
> +
> +RDEPEND="app-text/dictd"

Why the RDEPEND?  The packages seem only to install data files (without
calling any extra tools or anything), so I don't see why they need to
depend on anything.

> 
> +
> +S="${WORKDIR}"
> +
> +# @FUNCTION: dict_src_install
> +# @DESCRIPTION:
> +# The freedict src_install function, which is exported
> +dict_src_install() {
> +	insinto /usr/$(get_libdir)/dict
> +	for dict in "${DICTS[@]}"; do
> +		doins ${dict}.dict.dz
> +		doins ${dict}.index
> +	done
> +	einstalldocs
> +}
> +
> +EXPORT_FUNCTIONS src_install
> +
> +fi

That said, I don't really see any gain from having this eclass, really.

-- 
Best regards,
Michał Górny

Re: [gentoo-dev] [PATCH] freedict.eclass: rename dict.eclass, generalize

[Marty E. Plummer]

On Wed, Apr 25, 2018 at 08:47:16AM +0200, Ulrich Mueller wrote:
> >>>>> On Wed, 25 Apr 2018, Marty E Plummer wrote:
> 
> >> I don't see much code duplication there, so I think it would be
> >> cleaner to have a second eclass, rather than adding conditionals to
> >> the existing one.
> 
> > I mean if you take into account app-dicts/dictd-* and freedict.eclass;
> > without the above pr (which I did originally have with a new eclass,
> > until I realized that app-text/dictd's conf file hardcodes /usr/lib/dict
> > anyways so the freedict dictionaries get [at least on amd64 and other
> > targets which use/will use lib64] installed outside of the search path
> > anyways) they are all almost entirely identical with the contents of
> > freedict.eclass.
> 
> > Another thought I had was moving the src_install into dict.eclass and
> > have freedict inherit it, adding the differences.
> 
> $ egrep -v '^(#|$)' freedict.eclass | wc -l
> 18
> 
> IMHO not worth the effort for 18 lines of code. Keep it simple.
> 
> Ulrich

Still, optimization or no, if app-text/dictd is looking at /usr/lib/dict
unconditionally and the app-dict/freedict-* dict files are ending up in
/usr/lib64/dict conditionally, something needs to be fixed about that.

Re: [gentoo-dev] [PATCH] freedict.eclass: rename dict.eclass, generalize

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

>>>>> On Wed, 25 Apr 2018, Marty E Plummer wrote:

> Still, optimization or no, if app-text/dictd is looking at
> /usr/lib/dict unconditionally and the app-dict/freedict-* dict files
> are ending up in /usr/lib64/dict conditionally, something needs to
> be fixed about that.

So the dictionaries are installed in /usr/lib64 but looked up in
/usr/lib.

These are architecture independent files, right? They should be
installed under /usr/share then.

Ulrich

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]