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

[Michael Orlitzky]

Thanks for the detailed review. I followed every suggestion except the
doexe thing for *.so files (only because I don't understand the
reasoning yet). The new version is attached.



On 06/01/2016 01:47 PM, Michał Górny wrote:
>> +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.

I moved them all after the REQUIRED_USE mess.


>> +case ${EAPI} in
>> +	6) ;;
>> +	*)
>> +		die "php-ext-source-r3 is not compatible with EAPI=${EAPI}"
>
> I think you can use ${ECLASS} here btw.

Done.


>> +# @DESCRIPTION:
>> +# Lists the PHP slots compatibile the extension is compatibile with
>
> typo: compatible, and also appears twice ;-).

Fixed.


>> +# @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.

Yeah, I think we'll keep every SAPI currently used in the tree in the
list by default. We added "phpdbg" even though php:5.5 doesn't support
it. I documented that.


>> +# 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.

I renamed it to _php_target and unset it after the loop.


>> +	# 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?

Nope, and likewise with PHPDEPEND in the same loop. I changed them both.


>> +# @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.

It's PHP_EXT_ECONF_ARGS now, and I updated the migration guide.


>> +		# 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?

I'd never heard this before... why? I suppose the only trade-off is that
having them -x prevents them from showing up in bash's tab-completion
for executables.


>> +# @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.

Done.


>> +# @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.

I added USAGE for every function that takes an argument. I also figured
out where the eclass manpages come from and made sure that the other doc
issues (like DEFAULT_UNSET) are fixed w.r.t. eclass-to-manpage.sh.


>> +				cat "${FILESDIR}/${PHP_EXT_INIFILE}" >> "${ED}/${file}"
>
> || die

Fixed all of these.


>> +		# 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?

There's a longer answer to that question, but the fact that it's outside
of the loop is intentional and consistent with -r2.


>> +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.

Fixed all of the local variable issues.


>> +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%/*}).

Fixed both `dirname` calls.


>> +	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.

Fixed.


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

Yes, good catch. I cleaned that up.


>> +# @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.

Yeah, I changed the first parameter to <setting-or-section-name> and
made the other two optional.


>> +# 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'?

It's weird, but it's not wrong, and that's the way -r2 did it. There are
a few ebuilds (not maintained by the PHP team) that call
*addtoinifiles() themselves, and I don't want to annoy those people too
much for cosmetic changes.