Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SAPI variant #939

Closed
wants to merge 7 commits into from
Closed

Add SAPI variant #939

wants to merge 7 commits into from

Conversation

vlastv
Copy link

@vlastv vlastv commented Jan 10, 2020

This allow use NGINX Unit with minimal extended this variant image.

There have already been such requests #510, and they will be more often, or developers will write their own variations.

@vlastv
Copy link
Author

vlastv commented Jan 10, 2020

I can create a NGINX Unit variant, similar to Apache, instead of SAPI.

@lubo
Copy link

lubo commented Jan 13, 2020

Thanks @vlastv, I hope this gets picked, we've been waiting for a long time. Personally, I'd prefer SAPI variant to the Unit one, so the users can choose any server they want.

@tianon
Copy link
Member

tianon commented Jan 14, 2020

My comment from #510 (comment) is still very relevant -- this really doesn't make much sense as a variant by itself, but I do think it would be reasonable to add to the CLI variant (as I noted in #510 (comment)), assuming we can verify that the size impact is somewhere near negligible. Looking at the Travis results on this PR, I think the size impact is ~1MB? Can you please test and confirm?

@lubo
Copy link

lubo commented Jan 14, 2020

Size difference between 7.3-cli-alpine3.11 (80MB) and 7.3-sapi-alpine3.11 (81.1MB) is just 1.1MB. Diff of the images:

$ container-diff diff -nt file daemon://php:7.3-{cli,sapi}-alpine3.11

-----File-----

These entries have been added to php:7.3-cli-alpine3.11:
FILE                                                 SIZE
/usr/local/include/php/sapi/embed                    1.9K
/usr/local/include/php/sapi/embed/php_embed.h        1.9K
/usr/local/lib/libphp7.so                            14.2M

These entries have been deleted from php:7.3-cli-alpine3.11:
FILE                                     SIZE
/usr/local/bin/php-cgi                   13.3M
/usr/local/php/man/man1/php-cgi.1        15B

These entries have been changed between php:7.3-cli-alpine3.11 and php:7.3-sapi-alpine3.11:
FILE                                                     SIZE1        SIZE2
/usr/local/bin/phpdbg                                    13.5M        13.5M
/usr/local/bin/php                                       13.4M        13.4M
/usr/local/lib/php/.registry/pear.reg                    95.7K        95.7K
/usr/local/include/php/main/php_config.h                 59.5K        59.5K
/usr/local/lib/php/.registry/xml_util.reg                28.5K        28.5K
/usr/local/lib/php/.registry/archive_tar.reg             19.3K        19.3K
/usr/local/bin/phar.phar                                 14.5K        14.5K
/usr/local/lib/php/.registry/structures_graph.reg        10.6K        10.6K
/usr/local/lib/php/.registry/console_getopt.reg          10.4K        10.4K
/usr/local/lib/php/.filemap                              6.8K         6.8K
/usr/local/include/php/main/build-defs.h                 4.1K         4.2K
/usr/local/bin/php-config                                2.7K         2.7K
/etc/shadow                                              452B         452B
/usr/local/lib/php/.channels/__uri.reg                   267B         267B

As we can see, 7.3-sapi-alpine3.11 is only 1.1MB bigger because it doesn't include php-cgi. If libphp7.so was included in php:7.3-cli-alpine3.11, it'd be 94.2MB (nearly 18% bigger).

I prefer using the smallest and as stripped-down images as possible. Since containers using SAPI probably won't ever need php-cgi, adding a new variant makes sense to me.

@vlastv
Copy link
Author

vlastv commented Jan 14, 2020

@tianon, above provided a fairly complete picture on size of images.

@lubo
Copy link

lubo commented Jan 21, 2020

@tianon So, what's it gonna be? Does what I wrote earlier make sense to you? Do you still think it should be incorporated into CLI variant instead?

@lubo
Copy link

lubo commented Jan 27, 2020

@tianon Ping.

@tianon
Copy link
Member

tianon commented Jan 29, 2020

Sorry, yes, thank you for the detailed analysis. 👍

Unfortunately, this makes me even less excited about supporting this in either form -- the size of that .so file is fairly prohibitive and the number of image variants currently supported has a very tangible effect on our ability to get things like security releases out in a timely manner (docker-library/official-images#7332 for example was opened around 8am and wasn't mergeable for several hours due simply to test-building all supported variants).

In addition, this is not a variant that folks will run directly, but rather one they'll have to link their own software into in order to take advantage of.

If the desire is to have extremely up-to-date builds of PHP to link in other projects (such as an NGINX Unit image), I think the packages provided by the Debian Maintainer for PHP are probably a better option: https://deb.sury.org/#ubuntu-ppa and https://deb.sury.org/#debian-dpa -- inside, you'll find libphpX.Y-embed for a variety of OS versions, all kept very up-to-date with PHP upstream releases.

Alternatively, the changes required to rebuild this image with the added option are very minimal (just adding --enable-embed to PHP_EXTRA_CONFIGURE_ARGS on an appropriate variant like cli), so I don't think it's unreasonable to ask interested users to do so on their own (especially given that to use this, they'll have to be building their own image anyways).

In a perfect world, I'd love to see php, php-fpm, mod_php, etc all consume/use this same .so file (instead of embedding the shared PHP code in each executable directly), but I think that's probably a fairly large upstream refactor that isn't worth holding our breath for.

@lubo
Copy link

lubo commented Mar 28, 2020

So, what if we enabled SAPI in the CLI variants and in addition to that we'd apply suggestions from docker-library/python#448? We might end up with smaller images even after enabling SAPI. Would that make you more inclined to pick this up?

@thresheek
Copy link

Alternatively, the changes required to rebuild this image with the added option are very minimal (just adding --enable-embed to PHP_EXTRA_CONFIGURE_ARGS on an appropriate variant like cli), so I don't think it's unreasonable to ask interested users to do so on their own (especially given that to use this, they'll have to be building their own image anyways).

Hello @tianon!

So we're thinking about moving parts of NGINX Unit (https://hub.docker.com/r/nginx/unit /) images over from FROM debian:* to FROM php:cli and :ruby/:openjdk/:golang etc -- indeed, mainly to provide current versions of language runtimes -- but also to allow our users to utilize the familiar existing infrastructure and configurations.

I would think that adding --enable-embed to cli variant will work for us as proposed over at #510 or here. Having to duplicate efforts when maintaining forks of the PHP images is a bit meh...

@tianon
Copy link
Member

tianon commented Nov 25, 2020

@thresheek 👋❗

IMO this gets a lot more interesting when the official maintainers of the primary use case of it are interested in consuming it. 😄

In coming back to this, I think the question of whether to add this to the CLI variant boils down to the use case of the CLI variants, and whether they'll be substantially affected by a ~14MB increase in size (which in Alpine's case, is almost a 20% size increase).

Looking at just official images (to get some idea of how these get used), here's the relevant images:

$ bashbrew list --uniq php | grep cli | xargs -rt bashbrew children --uniq
bashbrew children --uniq php:8.0.0RC5-cli-buster php:8.0.0RC5-cli-alpine3.12 php:7.4.12-cli-buster php:7.4.12-cli-alpine3.12 php:7.4.12-cli-alpine3.11 php:7.3.24-cli-buster php:7.3.24-cli-stretch php:7.3.24-cli-alpine3.12 php:7.3.24-cli-alpine3.11 php:7.2.34-cli-buster php:7.2.34-cli-stretch php:7.2.34-cli-alpine3.12 php:7.2.34-cli-alpine3.11 
adminer:4.7.7-standalone
wordpress:cli-2.4.0
composer:2.0.7
composer:1.10.17
wordpress:cli-2.4.0-php7.3
wordpress:cli-2.4.0-php7.2

$ bashbrew list --uniq php | grep cli | xargs -r bashbrew children --uniq | xargs -rt bashbrew from --uniq
bashbrew from --uniq adminer:4.7.7-standalone wordpress:cli-2.4.0 composer:2.0.7 composer:1.10.17 wordpress:cli-2.4.0-php7.3 wordpress:cli-2.4.0-php7.2 
adminer:4.7.7-standalone: php:7.4-alpine
wordpress:cli-2.4.0: php:7.4-alpine
composer:2.0.7: php:7-alpine
composer:1.10.17: php:7-alpine
wordpress:cli-2.4.0-php7.3: php:7.3-alpine
wordpress:cli-2.4.0-php7.2: php:7.2-alpine

In all cases, these maintainers appear to simply be requesting a particular PHP version + Alpine Linux, and expecting the php CLI to be available.

So as an interim proposal, perhaps we start by adding this to the Debian-based CLI variants (so that the Alpine variants are still as small as is reasonably possible), and take it from there? It sounds like that would be sufficient for your use case at NGINX, @thresheek?

(We also have issues like #1076 that make me think Alpine's not really "officially" supported by PHP, so similar to what we do in the Go images, it probably makes sense to not overly encourage users to use Alpine-based variants, especially for building more complex things on top of like this .so is for.)

@thresheek
Copy link

It sounds like that would be sufficient for your use case at NGINX, @thresheek?

Oh yes, that will cover our use case 100%. We don't care much about alpine-based images at this point.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants