Re: [gentoo-dev] mozextension.eclass last call!

[Petteri Räty]

Jory A. Pratt wrote:
> 	Unless there are any major objections tomorrow night I will commit
> mozextension.eclass to the tree. You can find mozextension.eclass at
> http://dev.gentoo.org/~anarchy/eclass . You can also find the firefox
> and firefox-bin ebuilds at http://dev.gentoo.org/~anarchy/ebuilds that
> are waiting to go to be added to the tree which inherit mozextension.
>

http://dev.gentoo.org/~anarchy/eclass/mozconfig-2.eclass

	if use debug; then
		mozconfig_annotate +debug \
			--enable-debug \
			--enable-tests \
			--disable-reorder \
			--disable-strip \
			--disable-strip-libs \
			--enable-debugger-info-modules=ALL_MODULES
	else
		mozconfig_annotate -debug \
			--disable-debug \
			--disable-tests \
			--enable-reorder \
			--enable-strip \
			--enable-strip-libs

I would use a local variable for the common options to avoid code
duplication.

http://dev.gentoo.org/~anarchy/eclass/mozcoreconf.eclass

	declare MOZ=$([[ ${PN} == mozilla || ${PN} == gecko-sdk ]] && echo true
|| echo false)
	declare FF=$([[ ${PN} == *firefox ]] && echo true || echo false)
	declare TB=$([[ ${PN} == *thunderbird ]] && echo true || echo false)
	declare SB=$([[ ${PN} == *sunbird ]] && echo true || echo false)
	declare EM=$([[${PN} == *enig]] && echo true || echo false)

[[${PN} == *enig]] && echo true || echo false
This seems to be duplicated here many times. Consider making this a
helper function.


mozconfig_final() {
	declare ac opt hash reason

You seem to be using declare to keep variables local. I think it is more
common to use the local keyword for this:

betelgeuse@pena /usr/portage/eclass $ grep local * | wc -l
810
betelgeuse@pena /usr/portage/eclass $ grep declare * | wc -l
41

Why is mozconfig_final here three times?


http://dev.gentoo.org/~anarchy/eclass/mozextension.eclass

Consider adding debug-print calls to your functions.

		if [ "${x:0:2}" = "./" ] ; then
			srcdir=""
		else
			srcdir="${DISTDIR}/"
		fi

srcdir is not declared local in the beginning of the function
Check this for other variables too.

cd "${WORKDIR}" || die "$myfail"

for consistency you should probably use ${myfail}

[ ${#} -ne 1 ]  && die "'xpi_install' takes exactly one argument"

you can use FUNCNAME variable to get the name of the function

cd ${x}
Don't know if this can contain spaces but best to be sure and add
quotes. I also noticed a couple of other places where pathnames are not
quoted.

emid=$(sed -n '/<\?em:id>\?/!d; s/.*\([\"{].*[}\"]\).*/\1/; s/\"//g; p;
q' ${x}/install.rdf)

Consider adding some comments about what this is supposed to do so that
people who are looking at the code the first time get some kind of an idea.

Regards,
Petteri