WIP: Add PHP_EXTENSION_DIR environment

[glensc]

This adds PHP_EXTENSION_DIR environment variable, so at runtime can easily install extensions there.

NOTE: This is WIP to get feedback on the implementation first.

[glensc]

if backward compatibility is wanted could make the value be same as it is for current php versions, and make it with fixed value in 7.4 (or 7.3 already?) for example.

[tianon]

I don't really understand what problem this solves? What's wrong with the default value that PHP itself provides for us? It's already arguably pretty trivial to discover in a programmatic way (which is pretty canonical too, since it's being queried from PHP itself, and is thus not Docker-specific in any way):

php/docker-php-ext-enable

Line 4 in c856411

extDir="$(php -r 'echo ini_get("extension_dir");')"

[glensc]

it adds env variable so i can do COPY:

also it makes the path consistent, so if i build from same dockerfile extension, don't need to figure out the extension dir.

note: $PHP_INI_DIR already exists, which is good.

ARG PHP_VERSION=7.2
ARG CI_REGISTRY=gitlab.example.net:4567

FROM $CI_REGISTRY/library/php:$PHP_VERSION-cli-alpine AS build

# ...

FROM php:$PHP_VERSION-fpm-alpine3.8 AS base
# build final runtime image
FROM base

ARG EXTENSION_DIR=/usr/local/lib/php/extensions/no-debug-non-zts-20170718

COPY --from=build $EXTENSION_DIR/*.so $EXTENSION_DIR/
COPY --from=build $PHP_INI_DIR/conf.d/*.ini $PHP_INI_DIR/conf.d/

[tianon]

Ok, that kind of makes some sense, but in that case, why not simply modify php.ini's extension_dir value to have PHP look elsewhere for extensions?

Looking at the stock image, the only file in the default directory that's currently used is sodium.so, and that'd be trivial to remove (simply remove/update /usr/local/etc/php/conf.d/docker-php-ext-sodium.ini), so it should be reasonably safe to simply create a new .ini file there which adjusts extension_dir to point elsewhere, ala:

FROM php:7.2-fpm-alpine3.8
RUN echo 'extension_dir=/usr/local/lib/php/extensions' > /usr/local/etc/php/conf.d/extension-dir.ini
COPY --from=build .../*.so /usr/local/lib/php/extensions/

To clarify, I'm mostly opposed to this change to the default because it'll break users and because PHP upstream has to have a good reason for why they've chosen this specific directory structure by default, so I don't think it's our place to override that without a really solid justification as to why the reasons they decided don't make sense for the way PHP gets used inside Docker, and I don't think we have that right now (I can't really even find a good description of why they chose this in the first place, but I admittedly didn't look terribly hard for the justification -- the fact that it's the default setting for their build scripts is already a pretty strong justification for me).

[glensc]

it's for the non-containerized world, where you have multiple versions of the php engine installed.

[TimWolla]

I'd like to add a 👍 here. With Buildkit Docker supports parallel builds of multi-stage images and I was experimenting with building each extension in its own stage to improve both build performance as well as build cacheability.

This is what I have come up with:

FROM php:7-fpm AS opcache
RUN docker-php-ext-install opcache

*snip*

#######

FROM	php:7-fpm AS intl
RUN	export DEBIAN_FRONTEND="noninteractive"; \
	buildDeps=" \
		libicu-dev \
	"; \
	set -ex \
&&	apt-get update \
&&	apt-get install -y $buildDeps --no-install-recommends \
&&	docker-php-ext-install intl

#######

FROM	php:7-fpm

RUN	mkdir /exts/ \
&&	apt-get update \
*snip*
&&	apt-get install -y libicu57 --no-install-recommends \
&&	rm -rf /var/lib/apt/lists/*

COPY	--from=opcache /usr/local/lib/php/extensions/*/opcache.* /exts/
*snip*
COPY	--from=intl /usr/local/lib/php/extensions/*/intl.* /exts/

RUN	mv /exts/* "$(php -r 'echo ini_get("extension_dir");')" \
&&	rmdir /exts/ \
&&	docker-php-ext-enable opcache \
&&	*snip*
&&	docker-php-ext-enable intl

which is not exactly tidy. The approach works in general, though (i.e. the image successfully starts and the extensions appear in phpinfo()).

[tianon]

Sorry, finally coming back to this 🙇

I have an alternate multi-stage approach that I think is much cleaner and doesn't require any base image changes:

FROM php:7-fpm AS base

RUN set -eux; \
	extDir="$(php -r 'echo ini_get("extension_dir");')"; \
	mv "$extDir" /exts; \
	echo 'extension_dir = /exts' >> "$PHP_INI_DIR/conf.d/exts.ini"; \
# trust, but verify
	extDir="$(php -r 'echo ini_get("extension_dir");')"; \
	[ "$extDir" = '/exts' ]; \
# the phpize-generated configure script uses "php-config" to find the right "extension_dir", but that's hard-coded with the PHP-compiled value and doesn't query the PHP ini configuration... (which feels like a bug IMO)
# however, "php-config" is a simple shell script, so this is easily fixed
	sed -ri -e 's!^extension_dir=.*$!extension_dir="$(php -r "echo ini_get(\\"extension_dir\\");")"!' /usr/local/bin/php-config; \
# again: trust, but verify
	extDir="$(php-config --extension-dir)"; \
	[ "$extDir" = '/exts' ]

FROM base AS opcache
RUN docker-php-ext-install -j "$(nproc)" opcache

FROM base AS intl
RUN set -eux; \
	apt-get update; \
	apt-get install -y --no-install-recommends libicu-dev; \
	rm -rf /var/lib/apt/lists/*
RUN docker-php-ext-install -j "$(nproc)" intl

FROM base
COPY --from=opcache /exts /exts
COPY --from=intl /exts /exts
RUN set -eux; \
	docker-php-ext-enable opcache intl; \
	php -m

Most of that first stage is verification/comment, but there is what I'd consider a bug in php-config that it hard-codes the extension_dir rather than querying it from the PHP configuration, so this includes a sed for fixing that (since php-config is just a very simple shell script).

Here's the output from that last build stage:

...
Step 11/11 : RUN set -eux; 	docker-php-ext-enable opcache intl; 	php -m
 ---> Running in 0957a308dfb8
+ docker-php-ext-enable opcache intl
+ php -m
[PHP Modules]
Core
ctype
curl
date
dom
fileinfo
filter
ftp
hash
iconv
intl
json
libxml
mbstring
mysqlnd
openssl
pcre
PDO
pdo_sqlite
Phar
posix
readline
Reflection
session
SimpleXML
sodium
SPL
sqlite3
standard
tokenizer
xml
xmlreader
xmlwriter
Zend OPcache
zlib

[Zend Modules]
Zend OPcache

Removing intermediate container 0957a308dfb8
 ---> 4a26612d27ff
Successfully built 4a26612d27ff

[TimWolla]

I have an alternate multi-stage approach that I think is much cleaner and doesn't require any base image changes:

I agree with that. Your suggestion looks pretty good (now I just need to remember it until I get around to trying it).

[glensc]

but what is actually bad exposing that environment variable? I don't see it being a huge layer or performance issue.

with the env being supported (in upstream image) you can still do your overly complicated multi-layer build :)

OTOH, if you're suggesting to use fixed path for extension_dir in base PHP images, then I'm in with all hands!

[tianon]

Sorry for the delay -- my biggest concern is that it's yet another deviation from upstream's own supported functionality / recommendations / defaults.

If the bug in the upstream php-config shell script were fixed, my example snippet would get shorter too. If you're willing to ditch some of the overzealous trappings I tend to include in all my Dockerfiles (the constant "trust but verify", ala [ "$extDir" = '/exts' ]), you can get even shorter as well:

FROM php:8.1-fpm

RUN set -eux; \
# if you know this is empty (or don't care about what's in it by default), you can even ditch this `mv` and just update the `php.ini` settings
	extDir="$(php -r 'echo ini_get("extension_dir");')"; \
	mv "$extDir" /exts; \
	echo 'extension_dir = /exts' >> "$PHP_INI_DIR/conf.d/exts.ini"; \
# the phpize-generated configure script uses "php-config" to find the right "extension_dir", but that's hard-coded with the PHP-compiled value and doesn't query the PHP ini configuration... (which feels like a bug IMO)
# however, "php-config" is a simple shell script, so this is easily fixed
	sed -ri -e 's!^extension_dir=.*$!extension_dir="$(php -r "echo ini_get(\\"extension_dir\\");")"!' /usr/local/bin/php-config

[tianon]

#1452 (comment) is extremely related 👀 😇