On Thu, 2 Jun 2016 08:27:06 -0400 Michael Orlitzky <m...@gentoo.org> 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/>
pgpD0hHQPy9ZK.pgp
Description: OpenPGP digital signature
