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

[Michał Górny]

On Thu, 2 Jun 2016 08:27:06 -0400
Michael Orlitzky <mjo@g.o> wrote:

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

Next time, please inline and don't attach. It's awfully hard to reply
to both the reply and the attachment.

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

To be honest, I never dived into figuring out why it's like that -- but
all libraries on my system are +x, and that's how the compiler creates
them. So I'd rather follow that.

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

Sorry, I wasn't clear. I was asking why it's inside the outer loop,
rather than at the end of the function, after both loops?

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

I'd say you could support both, and prefer the simpler ;-).


Now, for the code:

> # @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
>
> 	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}"

Not sure if this is relevant but I think this breaks mtime guarantees.
In other words, output files may end up being 'earlier' than input files
depending on copy order.

In multibuild.eclass, I used 'cp -p -R' to preserve timestamps and modes.

> 	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
> }

Thinking about it... wouldn't it be better to:

a. stop overriding unpack,

b. run 'default' on ${S},

c. then copy sources in src_prepare()?

In other words, apply patches first, then copy; rather than copying, then
applying patches to each copy separately.

> # @ECLASS-VARIABLE: PHP_EXT_ECONF_ARGS
> # @DEFAULT_UNSET
> # @DESCRIPTION:
> # Set this in the ebuild to pass additional configure options to
> # econf. Formerly called my_conf.
>
> # @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_ECONF_ARGS}

I think PHPCONFIG would be better off quoted, and it would be good to
support PHP_EXT_ECONF_ARGS as an array in case someone needs to pass
whitespace there.

> 	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 doins/doexe do the same btw? It seems to be that the name is
not changed.

>
> 		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
> # @USAGE: <slot>
> # @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().
> #
> # Calling this function will change the working directory to the
> # temporary build directory for the given slot.
> 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}"

A reason why you are using ${1} before, and ${slot} here?

> 	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-ext-source-r3_addtoinifile
> # @USAGE: <relative-ini-path> <setting-or-section-name> [setting-value]
> # @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 second parameter, to create a new
> # section in the INI file. In that case, the third parameter (which
> # would otherwise be the value of the setting) is ignored.
> php-ext-source-r3_addtoinifile() {
> 	local inifile="${WORKDIR}/${1}"
> 	local inidir="${inifile%/*}"
>
> 	mkdir -p "${inidir}" || die "failed to create INI directory ${inidir}"
>
> 	# Are we adding the name of a section? Assume not by default.
> 	local my_added="${2}=${3}"
> 	if [[ ${2:0:1} == "[" ]] ; then
> 		# Ok, it's a section name.
> 		my_added="${2}"
> 	fi

How about:

  my_added=${2}${3+=${3}}

?

> 	echo "${my_added}" >> "${inifile}" || die "failed to append to ${inifile}"
> 	einfo "Added '${my_added}' to /${1}"
>
> 	insinto "/${1%/*}"
> 	doins "${inifile}"

Not that I like re-doinsing the files every time a line is inserted,
but I guess improving this is not worth the effort.

> }


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