Hi.

On 18-04-2011 23:02, Ulrich Mueller wrote:
>>>>>> On Mon, 18 Apr 2011, Jorge Manuel B S Vicetto wrote:
> 
>> The mysql team now uses 3 eclasses: mysql-v2.eclass[2] (base
>> eclass), mysql-autotools.eclass[3] (for autotools based releases)
>> and mysql-cmake.eclass[4] (for cmake based releases). The first 2
>> eclasses are complete, pending any updates from the review. The
>> mysql-cmake eclass is still under development, but can also benefit
>> from a review.
> 
> I didn't go through all of it, but here are a few things that I've
> noticed in mysql-v2.eclass:

Thank you Ulrich for the review.
I'm attaching the diff to this email, but the commitdiff can also be
seen in the overlay -
http://git.overlays.gentoo.org/gitweb/?p=proj/mysql.git;a=commitdiff;h=7cd4cedb1dcade2a63018fc82a2622606c524126

>> # @ECLASS: mysql.eclass
> 
> Shouldn't this match the filename of the eclass? (Same for
> mysql-autotools.eclass.)

Fixed.

>> # @DESCRIPTION:
>> # The mysql.eclass provides almost all the code to build the mysql ebuilds
>> # including the src_unpack, src_prepare, src_configure, src_compile,
>> # scr_install, pkg_preinst, pkg_postinst, pkg_config and pkg_postrm
>> # phase hooks.
> 
> Name of the eclass should be updated.

Fixed.

>> MYSQL_EXPF="src_unpack src_compile src_install"
>> case "${EAPI:-0}" in
>> 	2|3|4) MYSQL_EXPF+=" src_prepare src_configure" ;;
>> 	*) die "Unsupported EAPI: ${EAPI}" ;;
>> esac
> 
>> EXPORT_FUNCTIONS ${MYSQL_EXPF}
> 
> You don't need a global variable here:
> ,----
> | EXPORT_FUNCTIONS src_unpack src_compile src_install
> | case "${EAPI:-0}" in
> | 	2|3|4) EXPORT_FUNCTIONS src_prepare src_configure ;;
> | 	*) die "Unsupported EAPI: ${EAPI}" ;;
> | esac
> `----
> 
> or even:
> ,----
> | case "${EAPI:-0}" in
> | 	2|3|4) ;;
> | 	*) die "Unsupported EAPI: ${EAPI}" ;;
> | esac
> | EXPORT_FUNCTIONS src_unpack src_prepare src_configure src_compile src_install
> `----

I was following base.eclass example, but switched to the last
alternative you presented.

>> # @ECLASS-VARIABLE: XTRADB_VER
>> # @DESCRIPTION:
>> # Version of the XTRADB storage engine
>> XTRADB_VER="${XTRADB_VER}"
> 
> Is this assignment needed, or could you use @DEFAULT_UNSET instead?
> (Assuming it's for the eclass manpage.) Same for other variables.

Did you mean using ": ${XTRADB_VER:=}"? Done.

>> # Having different flavours at the same time is not a good idea
>> for i in "mysql" "mysql-community" "mysql-cluster" "mariadb" ; do
>> 	[[ "${i}" == ${PN} ]] ||
> 
> Quotes are not necessary here.

Fixed.

>> pbxt_patch_available \
>> && PBXT_P="pbxt-${PBXT_VERSION}" \
>> && PBXT_SRC_URI="http://www.primebase.org/download/${PBXT_P}.tar.gz mirror://sourceforge/pbxt/${PBXT_P}.tar.gz" \
>> && SRC_URI="${SRC_URI} pbxt? ( ${PBXT_SRC_URI} )" \
> 
>> # PBXT_NEWSTYLE means pbxt is in storage/ and gets enabled as other plugins
>> # vs. built outside the dir
>> pbxt_available \
>> && IUSE="${IUSE} pbxt" \
>> && mysql_version_is_at_least "5.1.40" \
>> && PBXT_NEWSTYLE=1
> 
>> xtradb_patch_available \
>> && XTRADB_P="percona-xtradb-${XTRADB_VER}" \
>> && XTRADB_SRC_URI_COMMON="${PERCONA_VER}/source/${XTRADB_P}.tar.gz" \
>> && XTRADB_SRC_B1="http://www.percona.com/" \
>> && XTRADB_SRC_B2="${XTRADB_SRC_B1}/percona-builds/" \
>> && XTRADB_SRC_URI1="${XTRADB_SRC_B2}/Percona-Server/Percona-Server-${XTRADB_SRC_URI_COMMON}" \
>> && XTRADB_SRC_URI2="${XTRADB_SRC_B2}/xtradb/${XTRADB_SRC_URI_COMMON}" \
>> && XTRADB_SRC_URI3="${XTRADB_SRC_B1}/${PN}/xtradb/${XTRADB_SRC_URI_COMMON}" \
>> && SRC_URI="${SRC_URI} xtradb? ( ${XTRADB_SRC_URI1} ${XTRADB_SRC_URI2} ${XTRADB_SRC_URI3} )" \
>> && IUSE="${IUSE} xtradb"
> 
> Probably a matter of taste, but I'd use "if" blocks instead of the
> multiple && here.

I switched to if blocks here.

>> 				mv --strip-trailing-slashes -T "${old_MY_DATADIR_s}" "${MY_DATADIR_s}" \
> 
> Both options --strip-trailing-slashes and -T are GNUisms and may not
> exist on other userlands (like BSD).

I'll let Robin take a look at this one.

> Ulrich

-- 
Regards,

Jorge Vicetto (jmbsvicetto) - jmbsvicetto at gentoo dot org
Gentoo- forums / Userrel / Devrel / KDE / Elections / RelEng