Re: [gentoo-dev] [PATCH 1/2] php-ext-source-r3.eclass: new revision supporting EAPI=6.

[Michał Górny]

On Wed,  1 Jun 2016 12:53:37 -0400
Michael Orlitzky <mjo@g.o> wrote:

> This is a new revision of the php-ext-source eclass that supports
> EAPI=6 (only) and cleans up some of the existing code. The list of
> user-facing changes is,
>
>   * Support only EAPI=6.
>
>   * PATCHES array/variable support.
>
>   * DOCS array support (bug 512184).
>
>   * Renamed my_conf and PHPSAPILIST variables.
>
> Some refactoring was done, but not in a way that consumers should
> notice. A migration guide can be found on the wiki:
>
> https://wiki.gentoo.org/wiki/Project:PHP/Php-ext-source-r3_migration_guide
>
> Gentoo-Bug: 512184
> ---
>  eclass/php-ext-source-r3.eclass | 387 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 387 insertions(+)
>  create mode 100644 eclass/php-ext-source-r3.eclass
>
> diff --git a/eclass/php-ext-source-r3.eclass b/eclass/php-ext-source-r3.eclass
> new file mode 100644
> index 0000000..f3af823
> --- /dev/null
> +++ b/eclass/php-ext-source-r3.eclass
> @@ -0,0 +1,387 @@
> +# Copyright 1999-2016 Gentoo Foundation
> +# Distributed under the terms of the GNU General Public License v2
> +# $Id$
> +
> +# @ECLASS: php-ext-source-r3.eclass
> +# @MAINTAINER:
> +# Gentoo PHP team <php-bugs@g.o>
> +# @BLURB:
> +# A unified interface for compiling and installing standalone PHP
> +# extensions.
> +# @DESCRIPTION:
> +# A unified interface for compiling and installing standalone PHP
> +# extensions.
> +
> +inherit autotools
> +
> +EXPORT_FUNCTIONS src_unpack src_prepare src_configure src_compile src_install
> +
> +DEPEND=">=sys-devel/m4-1.4.3
> +		>=sys-devel/libtool-1.5.18"
> +RDEPEND=""

Please move all of this below EAPI check. And I think *DEPEND would be
better set in one place.

> +
> +case ${EAPI} in
> +	6) ;;
> +	*)
> +		die "php-ext-source-r3 is not compatible with EAPI=${EAPI}"

I think you can use ${ECLASS} here btw.

> +esac
> +
> +# @ECLASS-VARIABLE: PHP_EXT_NAME
> +# @REQUIRED
> +# @DESCRIPTION:
> +# The extension name. This must be set, otherwise the eclass dies.
> +# Only automagically set by php-ext-pecl-r3.eclass, so unless your ebuild
> +# inherits that eclass, you must set this manually before inherit.
> +[[ -z "${PHP_EXT_NAME}" ]] && \
> +	die "no extension name specified for the php-ext-source-r3 eclass"
> +
> +# @ECLASS-VARIABLE: PHP_EXT_INI
> +# @DESCRIPTION:
> +# Controls whether or not to add a line to php.ini for the extension.
> +# Defaults to "yes" and should not be changed in most cases.
> +[[ -z "${PHP_EXT_INI}" ]] && PHP_EXT_INI="yes"
> +
> +# @ECLASS-VARIABLE: PHP_EXT_ZENDEXT
> +# @DESCRIPTION:
> +# Controls whether the extension is a ZendEngine extension or not.
> +# Defaults to "no". If you don't know what this is, you don't need it.
> +[[ -z "${PHP_EXT_ZENDEXT}" ]] && PHP_EXT_ZENDEXT="no"
> +
> +# @ECLASS-VARIABLE: USE_PHP
> +# @REQUIRED
> +# @DESCRIPTION:
> +# Lists the PHP slots compatibile the extension is compatibile with

typo: compatible, and also appears twice ;-).

> +# Example:
> +# @CODE
> +# USE_PHP="php5-6 php7-0"
> +# @CODE
> +[[ -z "${USE_PHP}" ]] && \
> +	die "USE_PHP is not set for the php-ext-source-r3 eclass"
> +
> +# @ECLASS-VARIABLE: PHP_EXT_OPTIONAL_USE
> +# @DESCRIPTION:
> +# If set, all of the dependencies added by this eclass will be
> +# conditional on USE=${PHP_EXT_OPTIONAL_USE}. This is needed when
> +# ebuilds have to inherit this eclass unconditionally, but only
> +# actually use it when (for example) the user has USE=php.
> +
> +# @ECLASS-VARIABLE: PHP_EXT_S
> +# @DESCRIPTION:
> +# The relative location of the temporary build directory for the PHP
> +# extension within the source package. This is useful for packages that
> +# bundle the PHP extension. Defaults to ${S}.
> +[[ -z "${PHP_EXT_S}" ]] && PHP_EXT_S="${S}"
> +
> +# @ECLASS-VARIABLE: PHP_EXT_SAPIS
> +# @DESCRIPTION:
> +# A list of SAPIs for which we'll install this extension. Formerly
> +# called PHPSAPILIST.
> +[[ -z "${PHP_EXT_SAPIS}" ]] && PHP_EXT_SAPIS="apache2 cli cgi fpm embed phpdbg"

Is this default something that might get extended in the future? I
think it might be worth noting that here.

> +
> +
> +# Make sure at least one target is installed. First, start a USE
> +# conditional like "php?", but only when PHP_EXT_OPTIONAL_USE is
> +# non-null. The option group "|| (..." is always started here.
> +REQUIRED_USE="${PHP_EXT_OPTIONAL_USE}${PHP_EXT_OPTIONAL_USE:+? ( }|| ( "
> +for target in ${USE_PHP}; do

Environment pollution! unset target, or make this local.

> +	# Now loop through each USE_PHP target and add the corresponding
> +	# dev-lang/php slot to PHPDEPEND.
> +	IUSE="${IUSE} php_targets_${target}"

Any reason not to use += here as well?

> +	# Strip "+" characters from USE_PHP???
> +	# target=${target/+}
> +	REQUIRED_USE+="php_targets_${target} "
> +	slot=${target/php}
> +	slot=${slot/-/.}
> +	PHPDEPEND="${PHPDEPEND}
> +	php_targets_${target}? ( dev-lang/php:${slot} )"
> +done
> +# Finally, end the optional group that we started before the loop. Close
> +# the USE-conditional if PHP_EXT_OPTIONAL_USE is non-null.
> +REQUIRED_USE+=") ${PHP_EXT_OPTIONAL_USE:+ )}"
> +
> +RDEPEND="${RDEPEND}
> +	${PHP_EXT_OPTIONAL_USE}${PHP_EXT_OPTIONAL_USE:+? ( }
> +	${PHPDEPEND}
> +	${PHP_EXT_OPTIONAL_USE:+ )}"
> +
> +DEPEND="${DEPEND}
> +	${PHP_EXT_OPTIONAL_USE}${PHP_EXT_OPTIONAL_USE:+? ( }
> +	${PHPDEPEND}
> +	${PHP_EXT_OPTIONAL_USE:+ )}
> +"
> +
> +# @ECLASS-VARIABLE: PHP_EXT_SKIP_PHPIZE
> +# @DESCRIPTION:
> +# By default, we run "phpize" in php-ext-source-r3_src_unpack(). Set
> +# PHP_EXT_SKIP_PHPIZE="yes" in your ebuild if you do not want to run
> +# phpize (and the autoreconf that becomes necessary afterwards).
> +
> +# @FUNCTION: php-ext-source-r3_src_unpack
> +# @DESCRIPTION:
> +# Runs the default src_unpack and then makes a copy for each PHP slot.
> +php-ext-source-r3_src_unpack() {
> +	default_src_unpack
> +
> +	local slot orig_s="${PHP_EXT_S}"
> +	for slot in $(php_get_slots); do
> +		cp -r "${orig_s}" "${WORKDIR}/${slot}" || \
> +			die "failed to copy sources from ${orig_s} to ${WORKDIR}/${slot}"
> +	done
> +}
> +
> +
> +# @FUNCTION: php-ext-source-r3_src_prepare
> +# @DESCRIPTION:
> +# For each PHP slot, we initialize the environment, run the default
> +# src_prepare() for PATCHES/eapply_user support, and then call
> +# php-ext-source-r3_phpize.
> +php-ext-source-r3_src_prepare() {
> +	for slot in $(php_get_slots); do
> +		php_init_slot_env "${slot}"
> +		default
> +		php-ext-source-r3_phpize
> +	done
> +}
> +
> +# @FUNCTION: php-ext-source-r3_phpize
> +# @DESCRIPTION:
> +# Subject to PHP_EXT_SKIP_PHPIZE, this function runs phpize and
> +# autoreconf in a manner that avoids warnings.
> +php-ext-source-r3_phpize() {
> +	if [[ "${PHP_EXT_SKIP_PHPIZE}" != 'yes' ]] ; then
> +		# Create configure out of config.m4. We use autotools_run_tool
> +		# to avoid some warnings about WANT_AUTOCONF and
> +		# WANT_AUTOMAKE (see bugs #329071 and #549268).
> +		autotools_run_tool "${PHPIZE}"
> +
> +		# Force libtoolize to run and regenerate autotools files (bug
> +		# #220519).
> +		rm aclocal.m4 || die "failed to remove aclocal.m4"
> +		eautoreconf
> +	fi
> +}
> +
> +
> +# @ECLASS-VARIABLE: PHP_EXT_EXTRA_ECONF
> +# @DESCRIPTION:
> +# Set this in the ebuild to pass configure options to econf. Formerly
> +# called my_conf.

Please don't name this *EXTRA_ECONF. EXTRA_ECONF is a Portage variable
that's meant to be set by user in make.conf/env, and not ebuilds.

> +# @FUNCTION: php-ext-source-r3_src_configure
> +# @DESCRIPTION:
> +# Takes care of standard configure for PHP extensions (modules).
> +php-ext-source-r3_src_configure() {
> +	# net-snmp creates these, bug #385403.
> +	addpredict /usr/share/snmp/mibs/.index
> +	addpredict /var/lib/net-snmp/mib_indexes
> +
> +	local slot
> +	for slot in $(php_get_slots); do
> +		php_init_slot_env "${slot}"
> +		# Set the correct config options
> +		econf --with-php-config=${PHPCONFIG} ${PHP_EXT_EXTRA_ECONF}
> +	done
> +}
> +
> +# @FUNCTION: php-ext-source-r3_src_compile
> +# @DESCRIPTION:
> +# Compile a standard standalone PHP extension.
> +php-ext-source-r3_src_compile() {
> +	# net-snmp creates these, bug #324739.
> +	addpredict /usr/share/snmp/mibs/.index
> +	addpredict /var/lib/net-snmp/mib_indexes
> +
> +	# shm extension creates a semaphore file, bug #173574.
> +	addpredict /session_mm_cli0.sem
> +	local slot
> +	for slot in $(php_get_slots); do
> +		php_init_slot_env "${slot}"
> +		emake
> +	done
> +}
> +
> +# @FUNCTION: php-ext-source-r3_src_install
> +# @DESCRIPTION:
> +# Install a standard standalone PHP extension. Uses einstalldocs()
> +# to support the DOCS variable/array.
> +php-ext-source-r3_src_install() {
> +	local slot
> +	for slot in $(php_get_slots); do
> +		php_init_slot_env "${slot}"
> +
> +		# Let's put the default module away. Strip $EPREFIX from
> +		# $EXT_DIR before calling newins (which handles EPREFIX itself).
> +		insinto "${EXT_DIR#$EPREFIX}"
> +		newins "modules/${PHP_EXT_NAME}.so" "${PHP_EXT_NAME}.so"

Wouldn't it be more appropriate to use exeinto/doexe to have the shared
libs +x?

> +
> +		INSTALL_ROOT="${D}" emake install-headers
> +	done
> +	einstalldocs
> +	php-ext-source-r3_createinifiles
> +}
> +
> +# @FUNCTION: php_get_slots
> +# @DESCRIPTION:
> +# Get a list of PHP slots contained in both the ebuild's USE_PHP and the
> +# user's PHP_TARGETS.
> +php_get_slots() {
> +	local s=""
> +	local slot
> +	for slot in ${USE_PHP}; do
> +		use php_targets_${slot} && s+=" ${slot/-/.}"
> +	done
> +	echo $s
> +}
> +
> +# @FUNCTION: php_init_slot_env
> +# @DESCRIPTION:
> +# Takes a slot name, and initializes some global variables to values
> +# corresponding to that slot. For example, it sets the path to the "php"
> +# and "phpize" binaries, which will differ for each slot. This function
> +# is intended to be called while looping through a list of slots
> +# obtained from php_get_slots().

Please make it explicit that it changes working directory.

> +php_init_slot_env() {
> +	local libdir=$(get_libdir)
> +
> +	PHPIZE="${EPREFIX}/usr/${libdir}/${1}/bin/phpize"
> +	PHPCONFIG="${EPREFIX}/usr/${libdir}/${1}/bin/php-config"
> +	PHPCLI="${EPREFIX}/usr/${libdir}/${1}/bin/php"
> +	PHPCGI="${EPREFIX}/usr/${libdir}/${1}/bin/php-cgi"
> +	PHP_PKG="$(best_version =dev-lang/php-${1:3}*)"
> +	PHPPREFIX="${EPREFIX}/usr/${libdir}/${slot}"
> +	EXT_DIR="$(${PHPCONFIG} --extension-dir 2>/dev/null)"
> +	PHP_CURRENTSLOT=${1:3}
> +
> +	PHP_EXT_S="${WORKDIR}/${1}"
> +	cd "${PHP_EXT_S}" || die "failed to change directory to ${PHP_EXT_S}"
> +}
> +
> +# @FUNCTION: php_slot_ini_files
> +# @INTERNAL
> +# @DESCRIPTION:
> +# Output a list of relative paths to INI files for the given
> +# slot. Usually there will be one INI file per SAPI.

Please add @USAGE to make clear what ${1} is. In fact, many functions
seem to miss @USAGE.

> +php_slot_ini_files() {
> +	local slot_ini_files=""
> +	local x
> +	for x in ${PHP_EXT_SAPIS} ; do
> +		if [[ -f "${EPREFIX}/etc/php/${x}-${1}/php.ini" ]] ; then
> +			slot_ini_files+=" etc/php/${x}-${1}/ext/${PHP_EXT_NAME}.ini"
> +		fi
> +	done
> +
> +	echo "${slot_ini_files}"
> +}
> +
> +# @FUNCTION: php-ext-source-r3_createinifiles
> +# @DESCRIPTION:
> +# Builds INI files for every enabled slot and SAPI.
> +php-ext-source-r3_createinifiles() {
> +	local slot
> +	for slot in $(php_get_slots); do
> +		php_init_slot_env "${slot}"
> +
> +		local file
> +		for file in $(php_slot_ini_files "${slot}") ; do
> +			if [[ "${PHP_EXT_INI}" = "yes" ]] ; then
> +				# Add the needed lines to the <ext>.ini files
> +				php-ext-source-r3_addextension "${PHP_EXT_NAME}.so" "${file}"
> +			fi
> +
> +			if [[ -n "${PHP_EXT_INIFILE}" ]] ; then
> +				cat "${FILESDIR}/${PHP_EXT_INIFILE}" >> "${ED}/${file}"

|| die

> +				einfo "Added contents of ${FILESDIR}/${PHP_EXT_INIFILE}" \
> +					  "to ${file}"
> +			fi
> +			inidir="${file/${PHP_EXT_NAME}.ini/}"
> +			inidir="${inidir/ext/ext-active}"
> +			dodir "/${inidir}"
> +			dosym "/${file}" "/${file/ext/ext-active}"
> +		done
> +
> +		# Add support for installing PHP files into a version dependent
> +		# directory
> +		PHP_EXT_SHARED_DIR="${EPREFIX}/usr/share/php/${PHP_EXT_NAME}"

Should this be repeated inside the loop?

> +	done
> +}
> +
> +# @FUNCTION: php-ext-source-r3_addextension
> +# @INTERNAL
> +# @DESCRIPTION:
> +# Add a line to an INI file that will enable the given extension. The
> +# first parameter is the path to the extension (.so) file, and the
> +# second parameter is the name of the INI file in which it should be
> +# loaded. This function determines the setting name (either
> +# "extension=..." or "zend_extension=...") and then calls
> +# php-ext-source-r3_addtoinifile to do the actual work.
> +php-ext-source-r3_addextension() {
> +	if [[ "${PHP_EXT_ZENDEXT}" = "yes" ]] ; then
> +		ext_type="zend_extension"
> +		ext_file="${EXT_DIR}/${1}" # Zend extensions need the path...
> +	else
> +		ext_type="extension"
> +		ext_file="${1}"
> +	fi

I think you want those variables local.

> +
> +	php-ext-source-r3_addtoinifile "${2}" "${ext_type}" "${ext_file}"
> +}
> +
> +# @FUNCTION: php-ext-source-r3_addtoinifile
> +# @INTERNAL
> +# @DESCRIPTION:
> +# Add a setting=value to one INI file. The first argument is the
> +# relative path to the INI file. The second argument is the setting
> +# name, and the third argument is its value.
> +#
> +# You can also pass "[Section]" as the first parameter, to create a new
> +# section in the INI file. In that case, the second parameter (which
> +# would otherwise be the value of the setting) is ignored.
> +php-ext-source-r3_addtoinifile() {
> +	local inifile="${WORKDIR}/${1}"
> +	local inidir="$(dirname ${inifile})"

I would suggest avoiding external tools like dirname when simple bash
subst can do the work (e.g. ${inifile%/*}).

> +	if [[ ! -d "${inidir}" ]] ; then
> +		mkdir -p "${inidir}" || die "failed to create INI directory ${inidir}"

'mkdir -p' is safe to call when the directory exists, so I don't think
you need the conditional.

> +	fi
> +
> +	# Are we adding the name of a section?
> +	if [[ ${2:0:1} == "[" ]] ; then
> +		echo "${2}" >> "${inifile}"

|| die

> +		my_added="${2}"

Again, missing local.

> +	else
> +		echo "${2}=${3}" >> "${inifile}"

|| die

> +		my_added="${2}=${3}"
> +	fi

All above considered, wouldn't it be simpler to set my_added first,
and then just echo it to the file outside the conditional?

> +
> +	einfo "Added '${my_added}' to /${1}"
> +
> +	insinto /$(dirname "${1}")
> +	doins "${inifile}"
> +}
> +
> +# @FUNCTION: php-ext-source-r3_addtoinifiles
> +# @USAGE: <setting name> <setting value> [message to output]; or just [section name]

I think you are mixing '[]' meaning optional and meaning literal '[]'
here.

> +# @DESCRIPTION:
> +# Add settings to every php.ini file installed by this extension.
> +# You can also add new [Section]s -- see the example below.
> +#
> +# @CODE
> +# Add some settings for the extension:
> +#
> +# php-ext-source-r3_addtoinifiles "zend_optimizer.optimization_level" "15"
> +# php-ext-source-r3_addtoinifiles "zend_optimizer.enable_loader" "0"
> +# php-ext-source-r3_addtoinifiles "zend_optimizer.disable_licensing" "0"

Hmm... just to make it clear... is there any reason you use two
arguments instead of the more obvious 'foo=15'?

> +#
> +# Adding values to a section in php.ini file installed by the extension:
> +#
> +# php-ext-source-r3_addtoinifiles "[Debugger]"
> +# php-ext-source-r3_addtoinifiles "debugger.enabled" "on"
> +# php-ext-source-r3_addtoinifiles "debugger.profiler_enabled" "on"
> +# @CODE
> +php-ext-source-r3_addtoinifiles() {
> +	local slot
> +	for slot in $(php_get_slots); do
> +		for file in $(php_slot_ini_files "${slot}") ; do
> +			php-ext-source-r3_addtoinifile "${file}" "${1}" "${2}"
> +		done
> +	done
> +}



-- 
Best regards,
Michał Górny
<http://dev.gentoo.org/~mgorny/>