[gentoo-dev] gstreamer eclass review

[gentoo-dev] gstreamer eclass review

[Gilles Dartiguelongue]

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

Hi list,

gstreamer-1 has been around for some time now and it is needed for gnome
3.6 to enter the tree. Since gstreamer devs have been a bit busy irl, a
few guys from gnome herd decided to take a look at it and try to bump
everything. I had an itch to scratch wrt current eclass writing so I
decided to start with that and bump all plugins using these new eclasses
to see how they fare. The results seems to be a lot more pleasant to
read and easier to understand.

Since this is basically a rewrite, I'll attach the full files for review
and three ebuilds using it (one regular plugin, one plugin linking to
installed gstreamer libs and one of the ebuilds with no external
dependency.

The goal of these new eclasses is mainly to drop all revision dependent
code while maintaining (to some extent) backward compatibility with 0.10
slot ebuilds.

We are currently not planning on dropping the mostly empty eclasses just
in case there is a need for pack specific changes in the future.

The eclasses were already overlooked by gstreamer herd but I'd like more
eyes to go over them beforing pushing this to the tree.

Since this is mostly privates eclasses, I'd like to proceed to tree
inclusion by next week. There should be little to no breakage and this
would help bump last 0.10 releases as well.

-- 
Gilles Dartiguelongue <eva@gentoo.org>
Gentoo

[-- Attachment #2: gst-plugins10.eclass --]
[-- Type: text/plain, Size: 7682 bytes --]

# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

# @ECLASS: gst-plugins10.eclass
# @MAINTAINER:
# gstreamer@gentoo.org
# @AUTHOR:
# Gilles Dartiguelongue <eva@gentoo.org>
# Saleem Abdulrasool <compnerd@gentoo.org>
# foser <foser@gentoo.org>
# zaheerm <zaheerm@gentoo.org>
# @BLURB: Manages build for invididual ebuild for gst-plugins.
# @DESCRIPTION:
# Eclass to make external gst-plugins emergable on a per-plugin basis and
# to solve the problem with gst-plugins generating far too much unneeded
# dependancies.
#
# GStreamer consuming applications should depend on the specific plugins they
# need as defined in their source code.
#
# In case of spider usage, obtain recommended plugins to use from Gentoo
# developers responsible for gstreamer <gstreamer@gentoo.org> or the application
# developer.

# XXX: what was GST_ORC intended for. Isn't it better to leave it to the
#      ebuild reponsability ?

inherit eutils multilib toolchain-funcs versionator

GST_EXPF=""
case "${EAPI:-0}" in
	1|2|3|4|5)
		GST_EXPF="${GST_EXPF} src_configure src_compile src_install"
		;;
	0)
		die "EAPI=\"${EAPI}\" is not supported anymore"
		;;
	*)
		die "EAPI=\"${EAPI}\" is not supported yet"
		;;
esac
EXPORT_FUNCTIONS ${GST_EXPF}

# @ECLASS-VARIABLE: GST_LA_PUNT
# @DESCRIPTION:
# Should we delete all the .la files?
# NOT to be used without due consideration.
if has "${EAPI:-0}" 0 1 2 3; then
	: ${GST_LA_PUNT:="no"}
else
	: ${GST_LA_PUNT:="yes"}
fi

# @ECLASS-VARIABLE: GST_ORC
# @DESCRIPTION:
# Ebuild supports dev-lang/orc.
: ${GST_ORC:="no"}

# @ECLASS-VARIABLE: GST_PLUGINS_BUILD
# @DESCRIPTION:
# Defines the plugins to be built.
# May be set by an ebuild and contain more than one indentifier, space
# seperated (only src_configure can handle mutiple plugins at this time).
GST_PLUGINS_BUILD=${PN/gst-plugins-/}

# @ECLASS-VARIABLE: GST_PLUGINS_BUILD_DIR
# @DESCRIPTION:
# Actual build directory of the plugin.
# Most often the same as the configure switch name.
GST_PLUGINS_BUILD_DIR=${PN/gst-plugins-/}

# @ECLASS-VARIABLE: GST_TARBALL_SUFFIX
# @DESCRIPTION:
# Most projects hosted on gstreamer.freedesktop.org mirrors provide tarballs as
# tar.bz2 or tar.xz. This eclass defaults to bz2 for EAPI 0, 1, 2, 3 and
# defaults to xz for everything else. This is because the gstreamer mirrors
# are moving to only have xz tarballs for new releases.
if has "${EAPI:-0}" 0 1 2 3; then
	: ${GST_TARBALL_SUFFIX:="bz2"}
else
	: ${GST_TARBALL_SUFFIX:="xz"}
fi

# Even though xz-utils are in @system, they must still be added to DEPEND; see
# http://archives.gentoo.org/gentoo-dev/msg_a0d4833eb314d1be5d5802a3b710e0a4.xml
if [[ ${GST_TARBALL_SUFFIX} == "xz" ]]; then
	DEPEND="${DEPEND} app-arch/xz-utils"
fi

# @ECLASS-VARIABLE: GST_ORG_MODULE
# @DESCRIPTION:
# Name of the module as hosted on gstreamer.freedesktop.org mirrors.
# Leave unset if package name matches module name.
: ${GST_ORG_MODULE:=$PN}

# @ECLASS-VARIABLE: GST_ORG_PVP
# @INTERNAL
# @DESCRIPTION:
# Major and minor numbers of the version number.
: ${GST_ORG_PVP:=$(get_version_component_range 1-2)}


DESCRIPTION="${BUILD_GST_PLUGINS} plugin for gstreamer"
HOMEPAGE="http://gstreamer.freedesktop.org/"
SRC_URI="http://gstreamer.freedesktop.org/src/${GST_ORG_MODULE}/${GST_ORG_MODULE}-${PV}.tar.${GST_TARBALL_SUFFIX}"

LICENSE="GPL-2"
SLOT="${GST_ORG_PVP}"

if [[ ${PN} != ${GST_ORG_MODULE} ]]; then
	# Do not run test phase for invididual plugin ebuilds.
	RESTRICT="test"
fi

RDEPEND="${RDEPEND}
	>=dev-libs/glib-2.6:2
	media-libs/gstreamer:${SLOT}
"

if [[ ${GST_ORC} = "yes" ]]; then
  IUSE="+orc"
	RDEPEND="${RDEPEND} orc? ( >=dev-lang/orc-0.4.6 )"
#else
# XXX: verify with old ebuilds.
# DEPEND="${DEPEND} dev-libs/liboil"
fi

# added to remove circular deps
# 6/2/2006 - zaheerm
if [[ ${PN} != ${GST_ORG_MODULE} ]]; then
	RDEPEND="${RDEPEND} media-libs/${GST_ORG_MODULE}:${SLOT}"
fi

DEPEND="${RDEPEND} ${DEPEND}
	>=sys-apps/sed-4
	>=sys-devel/gettext-0.17
	virtual/pkgconfig
"

S="${WORKDIR}/${GST_ORG_MODULE}-${PV}"


# @FUNCTION: gst-plugins10_get_plugins
# @INTERNAL
# @DESCRIPTION:
# Get the list of plugins requiring external dependencies.
gst-plugins10_get_plugins() {
	# Must be called from src_prepare/src_configure
	GST_PLUGINS_LIST=$(sed -rn 's/^AG_GST_CHECK_FEATURE\((\w+),.*/ \1 /p' \
		"${S}"/configure.* | tr '[:upper:]' '[:lower:]')
}

# @FUNCTION: gst-plugins10_find_plugin_dir
# @INTERNAL
# @DESCRIPTION:
# Finds plugin build directory and cd to it.
gst-plugins10_find_plugin_dir() {
	if [[ ! -d ${S}/ext/${GST_PLUGINS_BUILD_DIR} ]]; then
		if [[ ! -d ${S}/sys/${GST_PLUGINS_BUILD_DIR} ]]; then
			ewarn "No such plugin directory"
			die
		fi
		einfo "Building system plugin ${GST_PLUGINS_BUILD_DIR} ..."
		cd "${S}"/sys/${GST_PLUGINS_BUILD_DIR}
	else
		einfo "Building external plugin ${GST_PLUGINS_BUILD_DIR} ..."
		cd "${S}"/ext/${GST_PLUGINS_BUILD_DIR}
	fi
}

# @FUNCTION: gst-plugins10_system_link
# @USAGE: gst-plugins10_system_link gst-libs/gst/audio:gstreamer-audio [...]
# @DESCRIPTION:
# Walks through makefiles in order to make sure build will link against system
# librairies.
# Takes a list of path fragments and corresponding pkgconfig libraries
# separated by colon (:). Will replace the path fragment by the output of
# pkgconfig.
gst-plugins10_system_link() {
	local directory libs pkgconfig pc tuple
	pkgconfig=$(tc-getPKG_CONFIG)

	gst-plugins10_find_plugin_dir

	for tuple in $@ ; do
		directory="$(echo ${tuple} | cut -f1 -d':')"
		pc="$(echo ${tuple} | cut -f2 -d':')-${SLOT}"
		libs="$(${pkgconfig} --libs-only-l ${pc})"

		sed -e "s:\$(top_builddir)/${directory}/.*\.la:${libs}:" \
			-i Makefile.am Makefile.in || die
	done
}

# @FUNCTION: gst-plugins10_remove_unversioned_binaries
# @INTERNAL
# @DEPRECATED
# @DESCRIPTION:
# Remove the unversioned binaries gstreamer provides to prevent file collision
# with other slots.
gst-plugins10_remove_unversioned_binaries() {
	cd "${D}"/usr/bin
	local gst_bins
	for gst_bins in *-${SLOT} ; do
		[[ -e ${gst_bins} ]] || continue
		rm ${gst_bins/-${SLOT}/}
		einfo "Removed ${gst_bins/-${SLOT}/}"
	done
}

# @FUNCTION: gst-plugins10_src_configure
gst-plugins10_src_configure() {
	local plugin gst_conf

	gst-plugins10_get_plugins

	for plugin in ${GST_PLUGINS_LIST} ; do
		gst_conf="${gst_conf} --disable-${plugin}"
	done

	for plugin in ${GST_PLUGINS_BUILD} ; do
		gst_conf="${gst_conf} --enable-${plugin}"
	done

	if grep -q "ORC_CHECK" configure.* ; then
		if [[ ${GST_ORC} = "yes" ]]; then
			gst_conf="${gst_conf} $(use_enable orc)"
		else
			gst_conf="${gst_conf} --disable-orc"
		fi
	else
		if [[ ${GST_ORC} = "yes" ]]; then
			eqawarn "This ebuild declares supporting USE=orc but does not."
			eqawarn "Please report this as a bug at http://bugs.gentoo.org/"
		fi
	fi

	if grep -q "AM_MAINTAINER_MODE" configure.* ; then
		gst_conf="${gst_conf} --disable-maintainer-mode"
	fi

	if grep -q "disable-schemas-compile" configure ; then
		gst_conf="${gst_conf} --disable-schemas-compile"
	fi

	einfo "Configuring to build ${GST_PLUGINS_BUILD} plugin(s) ..."
	econf \
		--with-package-name="Gentoo GStreamer ebuild" \
		--with-package-origin="http://www.gentoo.org" \
		${gst_conf} $@
}

# @FUNCTION: gst-plugins10_src_compile
gst-plugins10_src_compile() {
	gst-plugins10_find_plugin_dir

	if has "${EAPI:-0}" 0 1 2 3 ; then
		emake || die
	else
		default
	fi
}

# @FUNCTION: gst-plugins10_src_install
gst-plugins10_src_install() {
	gst-plugins10_find_plugin_dir

	if has "${EAPI:-0}" 0 1 2 3 ; then
		emake install DESTDIR="${D}" || die
		[[ -e README ]] && dodoc README
	else
		default
	fi

	[[ ${GST_LA_PUNT} = "yes" ]] && prune_libtool_files --modules
}


[-- Attachment #3: gst-plugins-bad.eclass --]
[-- Type: text/plain, Size: 1172 bytes --]

# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

# @ECLASS: gst-plugins10-bad.eclass
# @MAINTAINER:
# gstreamer@gentoo.org
# @AUTHOR:
# Gilles Dartiguelongue <eva@gentoo.org>
# Saleem Abdulrasool <compnerd@gentoo.org>
# foser <foser@gentoo.org>
# zaheerm <zaheerm@gentoo.org>
# @BLURB: Manages build for invididual ebuild for gst-plugins-bad.
# @DESCRIPTION:
# See gst-plugins10.eclass documentation.

GST_ORG_MODULE="gst-plugins-bad"

inherit eutils gst-plugins10

case "${EAPI:-0}" in
	1|2|3|4|5)
		;;
	0)
		die "EAPI=\"${EAPI}\" is not supported anymore"
		;;
	*)
		die "EAPI=\"${EAPI}\" is not supported yet"
		;;
esac


if [[ ${PN} != ${GST_ORG_MODULE} ]]; then
# -bad-0.10.20 uses orc optionally instead of liboil unconditionally.
# While <0.10.20 configure always check for liboil, it is used only by
# non-split plugins in gst/ (legacyresample and mpegdemux), so we only
# builddep for all old packages, and have a RDEPEND in old versions of
# media-libs/gst-plugins-bad
	if [[ ${SLOT} = "0.10" ]] && ! version_is_at_least "0.10.20"; then
		DEPEND="${DEPEND} >=dev-libs/liboil-0.3.8"
	fi
fi


[-- Attachment #4: gst-plugins-base.eclass --]
[-- Type: text/plain, Size: 690 bytes --]

# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

# @ECLASS: gst-plugins-base.eclass
# @MAINTAINER:
# gstreamer@gentoo.org
# @AUTHOR:
# Gilles Dartiguelongue <eva@gentoo.org>
# Saleem Abdulrasool <compnerd@gentoo.org>
# foser <foser@gentoo.org>
# zaheerm <zaheerm@gentoo.org>
# @BLURB: Manages build for invididual ebuild for gst-plugins-base.
# @DESCRIPTION:
# See gst-plugins10.eclass documentation.

GST_ORG_MODULE="gst-plugins-base"

inherit gst-plugins10

case "${EAPI:-0}" in
	1|2|3|4|5)
		;;
	0)
		die "EAPI=\"${EAPI}\" is not supported anymore"
		;;
	*)
		die "EAPI=\"${EAPI}\" is not supported yet"
		;;
esac


[-- Attachment #5: gst-plugins-good.eclass --]
[-- Type: text/plain, Size: 1148 bytes --]

# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

# @ECLASS: gst-plugins-good.eclass
# @MAINTAINER:
# gstreamer@gentoo.org
# @AUTHOR:
# Gilles Dartiguelongue <eva@gentoo.org>
# Saleem Abdulrasool <compnerd@gentoo.org>
# foser <foser@gentoo.org>
# zaheerm <zaheerm@gentoo.org>
# @BLURB: Manages build for invididual ebuild for gst-plugins-good.
# @DESCRIPTION:
# See gst-plugins10.eclass documentation.

GST_ORG_MODULE="gst-plugins-good"

inherit eutils gst-plugins10

case "${EAPI:-0}" in
	1|2|3|4|5)
		;;
	0)
		die "EAPI=\"${EAPI}\" is not supported anymore"
		;;
	*)
		die "EAPI=\"${EAPI}\" is not supported yet"
		;;
esac


if [[ ${PN} != ${GST_ORG_MODULE} ]]; then
# -good-0.10.24 uses orc optionally instead of liboil unconditionally.
# While <0.10.24 configure always checks for liboil, it is linked to only by
# non-split plugins in gst/, so we only builddep for all old packages, and have
# a RDEPEND in old versions of media-libs/gst-plugins-good
	if [[ ${SLOT} = "0.10" ]] && ! version_is_at_least "0.10.24"; then
		DEPEND="${DEPEND} >=dev-libs/liboil-0.3.8"
	fi
fi


[-- Attachment #6: gst-plugins-ugly.eclass --]
[-- Type: text/plain, Size: 690 bytes --]

# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

# @ECLASS: gst-plugins-ugly.eclass
# @MAINTAINER:
# gstreamer@gentoo.org
# @AUTHOR:
# Gilles Dartiguelongue <eva@gentoo.org>
# Saleem Abdulrasool <compnerd@gentoo.org>
# foser <foser@gentoo.org>
# zaheerm <zaheerm@gentoo.org>
# @BLURB: Manages build for invididual ebuild for gst-plugins-ugly.
# @DESCRIPTION:
# See gst-plugins10.eclass documentation.

GST_ORG_MODULE="gst-plugins-ugly"

inherit gst-plugins10

case "${EAPI:-0}" in
	1|2|3|4|5)
		;;
	0)
		die "EAPI=\"${EAPI}\" is not supported anymore"
		;;
	*)
		die "EAPI=\"${EAPI}\" is not supported yet"
		;;
esac


[-- Attachment #7: gst-plugins-libvisual-1.0.2.ebuild --]
[-- Type: text/plain, Size: 453 bytes --]

# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

EAPI="5"

inherit gst-plugins-base gst-plugins10

KEYWORDS="~amd64 ~hppa ~ppc ~ppc64 ~x86 ~amd64-fbsd"
IUSE=""

RDEPEND=">=media-libs/libvisual-0.4
	>=media-plugins/libvisual-plugins-0.4"
DEPEND="${RDEPEND}"

src_prepare() {
	gst-plugins10_system_link \
		gst-libs/gst/audio:gstreamer-audio \
		gst-libs/gst/video:gstreamer-video
}

[-- Attachment #8: gst-plugins-flac-1.0.2.ebuild --]
[-- Type: text/plain, Size: 374 bytes --]

# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

EAPI="5"

inherit gst-plugins-good

DESCRIPTION="GStreamer encoder/decoder/tagger for FLAC"
KEYWORDS="~alpha ~amd64 ~arm ~hppa ~ia64 ~mips ~ppc ~ppc64 ~sh ~sparc ~x86 ~amd64-fbsd ~x86-fbsd"
IUSE=""

RDEPEND=">=media-libs/flac-1.1.4"
DEPEND="${RDEPEND}"

[-- Attachment #9: gst-plugins-ugly-1.0.2.ebuild --]
[-- Type: text/plain, Size: 960 bytes --]

# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

EAPI="5"

# order is important, gnome2 after gst-plugins
inherit eutils flag-o-matic gst-plugins-ugly gst-plugins10

DESCRIPTION="Basepack of plugins for gstreamer"
HOMEPAGE="http://gstreamer.sourceforge.net"

LICENSE="GPL-2"
KEYWORDS="~alpha ~amd64 ~arm ~hppa ~ia64 ~mips ~ppc ~ppc64 ~sh ~sparc ~x86 ~amd64-fbsd ~x86-fbsd"
IUSE="+orc"

RDEPEND="
	>=dev-libs/glib-2.32:2
	orc? ( >=dev-lang/orc-0.4.16 )
"
DEPEND="${RDEPEND}
	=media-libs/gst-plugins-base-${PV}:${SLOT}
	>=dev-util/gtk-doc-am-1.12
"

DOCS="AUTHORS ChangeLog NEWS README RELEASE"

GST_PLUGINS_BUILD=""

src_configure() {
	# gst doesnt handle optimisations well
	strip-flags
	replace-flags "-O3" "-O2"
	filter-flags "-fprefetch-loop-arrays" # see bug 22249

	gst-plugins10_src_configure
}

src_compile() {
	default
}

src_install() {
	default
	prune_libtool_files --modules
}

[gentoo-dev] Re: gstreamer eclass review

[Duncan]

Gilles Dartiguelongue posted on Sun, 18 Nov 2012 20:06:30 +0100 as
excerpted:

It's admittedly a style thing thus pretty much up to the author, purely
bikeshedding in the original sense of the essay's trivial color choice,
but...

> # Even though xz-utils are in @system, they must still be added to DEPEND; see
> # http://archives.gentoo.org/gentoo-dev/msg_a0d4833eb314d1be5d5802a3b710e0a4.xml
> if [[ ${GST_TARBALL_SUFFIX} == "xz" ]]; then
> 	DEPEND="${DEPEND} app-arch/xz-utils"
> fi

Single-clause conditional tests (without an else clause) such as the above
can be trivially rewritten like so:

[[ ${GST_TARBALL_SUFFIX} == "xz" ]] && \
 	DEPEND="${DEPEND} app-arch/xz-utils"

Two lines (or one if short enough) instead of three and more concise (fi
line omitted entirely, "if" omitted, "; then" replaced with &&).

It's possible to do similar with if/then/else -> &&/||, but that may
require {} for clarity if not bash parsing, and IMO is not nearly as
clear as the more straightforward no-else-clause case.  So if there's
an else clause, I prefer if/then/else; if not, I prefer the && or || logic.

> if [[ ${PN} != ${GST_ORG_MODULE} ]]; then
> 	# Do not run test phase for invididual plugin ebuilds.
>	RESTRICT="test"
> fi

Another example, this one definitely a one-liner (plus comment,
"invididual" typo left intact)...

# Do not run test phase for invididual plugin ebuilds.
[[ ${PN} != ${GST_ORG_MODULE} ]] && RESTRICT="test"

Or, avoiding that ! in the test and changing the && to ||...

[[ ${PN} == ${GST_ORG_MODULE} ]] || RESTRICT="test"

Also, while we're here, "test" is a string-literal with no possibility of
spaces or anything else that could otherwise confuse bash, so needs no
quotes.  And I'm not sure if the != pattern matching trigger was
deliberate or not (see bash's "help [[" output).  Thus...

[[ ${PN} = ${GST_ORG_MODULE} ]] || RESTRICT=test

... or...

[[ ${PN} == ${GST_ORG_MODULE} ]] || RESTRICT=test

... with the single vs. double equals making the string match (single) vs
pattern match (double) explicit.  If the test-internal-not form is
retained, the string match form would be...

[[ ${PN} ! = ${GST_ORG_MODULE} ]] && RESTRICT=test

... while the pattern match form would retain the != that was used
(the distinction being the separating space, != is a pattern match
as is ==, ! = is a string match as is a single = ).


> # added to remove circular deps # 6/2/2006 - zaheerm
if [[ ${PN} != ${GST_ORG_MODULE} ]]; then
> 	RDEPEND="${RDEPEND} media-libs/${GST_ORG_MODULE}:${SLOT}"
> fi

Ditto... Further examples skipped.

> # @FUNCTION: gst-plugins10_src_install gst-plugins10_src_install() {
> 	gst-plugins10_find_plugin_dir
> 
> 	if has "${EAPI:-0}" 0 1 2 3 ; then
> 		emake install DESTDIR="${D}" || die
>		[[ -e README ]] && dodoc README
> 	else
> 		default
> 	fi
> 
> 	[[ ${GST_LA_PUNT} = "yes" ]] && prune_libtool_files --modules
> }

Here (last line before the function-closing "}", as I said above, I prefer
if/then/else where there's an else clause, so wouldn't change the upper
conditional) we see a counter-example, doing it just as I suggested.
The if/then version (keeping function indent) would look like this.

	if [[ ${GST_LA_PUNT} = "yes" ]]; then
		prune_libtool_files --modules
	fi

Of course regardless of that, the quotes around "yes" may be omitted as
it's a string literal.  (And here, the explicit single-equals string-
literal match is used, as opposed to the double-equals pattern match.  Of
course either one would work here as it's a trivial pattern, just noting
it for consistency.)

	[[ ${GST_LA_PUNT} = yes ]] && prune_libtool_files --modules


But as I said up top, that's (mostly, the pattern matching vs string
matching will occasionally bite if you're not on the lookout for it)
trivial style stuff.  You may well prefer the way it is now.  But in that
case, why the counter-example, omitting if/then?  (You may of course
fairly point out that consistency is a trivial style thing too. =:^)

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [gentoo-dev] Re: gstreamer eclass review

[Gilles Dartiguelongue]

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

Le dimanche 18 novembre 2012 à 20:45 +0000, Duncan a écrit :
> Gilles Dartiguelongue posted on Sun, 18 Nov 2012 20:06:30 +0100 as
> excerpted:
> 
[...]
> But as I said up top, that's (mostly, the pattern matching vs string
> matching will occasionally bite if you're not on the lookout for it)
> trivial style stuff.  You may well prefer the way it is now.  But in that
> case, why the counter-example, omitting if/then?  (You may of course
> fairly point out that consistency is a trivial style thing too. =:^)

I admittedly didn't really look for consistency in these details. I
picked stuff from the 4 eclasses and merged them in one, having the
gnome2 eclasses open for reference which might explain some
inconsistencies in style.

Anyway if you read all that up and only mailed about this, I guess you
found no problem with the rest, right ?

-- 
Gilles Dartiguelongue <eva@gentoo.org>
Gentoo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[gentoo-dev] Re: gstreamer eclass review

[Duncan]

Gilles Dartiguelongue posted on Sun, 18 Nov 2012 23:59:44 +0100 as
excerpted:

> Anyway if you read all that up and only mailed about this, I guess you
> found no problem with the rest, right ?

Yes, but I already admitted to bikeshedding the easy stuff, so I'd 
honestly not assign too much review weight to it.

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [gentoo-dev] Re: gstreamer eclass review

[Alec Warner]

On Sun, Nov 18, 2012 at 12:45 PM, Duncan <1i5t5.duncan@cox.net> wrote:
> Gilles Dartiguelongue posted on Sun, 18 Nov 2012 20:06:30 +0100 as
> excerpted:
>
> It's admittedly a style thing thus pretty much up to the author, purely
> bikeshedding in the original sense of the essay's trivial color choice,
> but...
>
>> # Even though xz-utils are in @system, they must still be added to DEPEND; see
>> # http://archives.gentoo.org/gentoo-dev/msg_a0d4833eb314d1be5d5802a3b710e0a4.xml
>> if [[ ${GST_TARBALL_SUFFIX} == "xz" ]]; then
>>       DEPEND="${DEPEND} app-arch/xz-utils"
>> fi
>
> Single-clause conditional tests (without an else clause) such as the above
> can be trivially rewritten like so:
>
> [[ ${GST_TARBALL_SUFFIX} == "xz" ]] && \
>         DEPEND="${DEPEND} app-arch/xz-utils"
>
> Two lines (or one if short enough) instead of three and more concise (fi
> line omitted entirely, "if" omitted, "; then" replaced with &&).

http://www.quickmeme.com/meme/3ru1zx/

>
> It's possible to do similar with if/then/else -> &&/||, but that may
> require {} for clarity if not bash parsing, and IMO is not nearly as
> clear as the more straightforward no-else-clause case.  So if there's
> an else clause, I prefer if/then/else; if not, I prefer the && or || logic.
>
>> if [[ ${PN} != ${GST_ORG_MODULE} ]]; then
>>       # Do not run test phase for invididual plugin ebuilds.
>>       RESTRICT="test"
>> fi
>
> Another example, this one definitely a one-liner (plus comment,
> "invididual" typo left intact)...
>
> # Do not run test phase for invididual plugin ebuilds.
> [[ ${PN} != ${GST_ORG_MODULE} ]] && RESTRICT="test"
>
> Or, avoiding that ! in the test and changing the && to ||...
>
> [[ ${PN} == ${GST_ORG_MODULE} ]] || RESTRICT="test"
>
> Also, while we're here, "test" is a string-literal with no possibility of
> spaces or anything else that could otherwise confuse bash, so needs no
> quotes.  And I'm not sure if the != pattern matching trigger was
> deliberate or not (see bash's "help [[" output).  Thus...
>
> [[ ${PN} = ${GST_ORG_MODULE} ]] || RESTRICT=test
>
> ... or...
>
> [[ ${PN} == ${GST_ORG_MODULE} ]] || RESTRICT=test
>
> ... with the single vs. double equals making the string match (single) vs
> pattern match (double) explicit.  If the test-internal-not form is
> retained, the string match form would be...
>
> [[ ${PN} ! = ${GST_ORG_MODULE} ]] && RESTRICT=test
>
> ... while the pattern match form would retain the != that was used
> (the distinction being the separating space, != is a pattern match
> as is ==, ! = is a string match as is a single = ).
>
>
>> # added to remove circular deps # 6/2/2006 - zaheerm
> if [[ ${PN} != ${GST_ORG_MODULE} ]]; then
>>       RDEPEND="${RDEPEND} media-libs/${GST_ORG_MODULE}:${SLOT}"
>> fi
>
> Ditto... Further examples skipped.
>
>> # @FUNCTION: gst-plugins10_src_install gst-plugins10_src_install() {
>>       gst-plugins10_find_plugin_dir
>>
>>       if has "${EAPI:-0}" 0 1 2 3 ; then
>>               emake install DESTDIR="${D}" || die
>>               [[ -e README ]] && dodoc README
>>       else
>>               default
>>       fi
>>
>>       [[ ${GST_LA_PUNT} = "yes" ]] && prune_libtool_files --modules
>> }
>
> Here (last line before the function-closing "}", as I said above, I prefer
> if/then/else where there's an else clause, so wouldn't change the upper
> conditional) we see a counter-example, doing it just as I suggested.
> The if/then version (keeping function indent) would look like this.
>
>         if [[ ${GST_LA_PUNT} = "yes" ]]; then
>                 prune_libtool_files --modules
>         fi
>
> Of course regardless of that, the quotes around "yes" may be omitted as
> it's a string literal.  (And here, the explicit single-equals string-
> literal match is used, as opposed to the double-equals pattern match.  Of
> course either one would work here as it's a trivial pattern, just noting
> it for consistency.)
>
>         [[ ${GST_LA_PUNT} = yes ]] && prune_libtool_files --modules
>
>
> But as I said up top, that's (mostly, the pattern matching vs string
> matching will occasionally bite if you're not on the lookout for it)
> trivial style stuff.  You may well prefer the way it is now.  But in that
> case, why the counter-example, omitting if/then?  (You may of course
> fairly point out that consistency is a trivial style thing too. =:^)
>
> --
> Duncan - List replies preferred.   No HTML msgs.
> "Every nonfree program has a lord, a master --
> and if you use the program, he is your master."  Richard Stallman
>
>

Re: [gentoo-dev] gstreamer eclass review

[Tomáš Chvátal]

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

Hi Gilles,

The eclass itself looks fine, I would just ask if you would not mind to use
the bash += syntax rather than VAR="VAR something" as it is shorter and
easier to read.

Cheers

Tom

[-- Attachment #2: Type: text/html, Size: 742 bytes --]

Re: [gentoo-dev] gstreamer eclass review

[Gilles Dartiguelongue]

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

Le mercredi 21 novembre 2012 à 16:27 +0100, Tomáš Chvátal a écrit :
> Hi Gilles,
> 
> 
> The eclass itself looks fine, I would just ask if you would not mind
> to use the bash += syntax rather than VAR="VAR something" as it is
> shorter and easier to read.

Sure, I'm not a fan of +=, it seems alien to shell for me, but if this
is preferred, I have no problem changing it.

-- 
Gilles Dartiguelongue <eva@gentoo.org>
Gentoo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [gentoo-dev] gstreamer eclass review

[Maxim Kammerer]

Hi, after the commit 3 days ago all kinds of plugins suddenly depend
on gst-plugins-bad. E.g.: gst-plugins-{dts,faad,libmms,resindvd,xvid}.
Is gst-plugins-bad eclass the wrong one to inherit in their case?
Also, vp8 plugin both inherits from the eclas, and has an explicit
dependency on media-libs/gst-plugins-bad — perhaps one or the other
should be removed.

-- 
Maxim Kammerer
Liberté Linux: http://dee.su/liberte

Re: [gentoo-dev] gstreamer eclass review

[Gilles Dartiguelongue]

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

Le jeudi 06 décembre 2012 à 06:01 +0200, Maxim Kammerer a écrit :
> Hi, after the commit 3 days ago all kinds of plugins suddenly depend
> on gst-plugins-bad. E.g.: gst-plugins-{dts,faad,libmms,resindvd,xvid}.
> Is gst-plugins-bad eclass the wrong one to inherit in their case?
> Also, vp8 plugin both inherits from the eclas, and has an explicit
> dependency on media-libs/gst-plugins-bad — perhaps one or the other
> should be removed.
> 
Please open bug reports at gentoo's bugzilla if you have problem with
the ebuilds.

-- 
Gilles Dartiguelongue <eva@gentoo.org>
Gentoo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]