>>>>> 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:
> # @ECLASS: mysql.eclass
Shouldn't this match the filename of the eclass? (Same for
mysql-autotools.eclass.)
> # @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.
> 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
`----
> # @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.
> # 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.
> 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.
> 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).
Ulrich
