docker-library / php

Docker Official Image packaging for PHP
https://php.net
MIT License
3.82k stars 2k forks source link

Restore phpdbg in fpm images #1260

Open MinDBreaK opened 2 years ago

MinDBreaK commented 2 years ago

Hello,

I just came across the PR #1259 and the issue #1257 after wondering why the phpdbg has suddenly disappeared from images like php:7.4-fpm-alpine3.14.

While I understand the point of "It is not suitable to debug fpm processes" the images can still be used with the PHP CLI, and thus, having the need for the phpdbg component. In our case we use the production build to run all tests and generate code coverage, and only install dev dependecies on start of the container. We also use the fpm image to run async workers without fpm started)

The use of phpdbg allows faster tests and not having to instal xdebug specifically for this (which would slow drastically any process).

Leaving it will not impact any existing user as it does not provide (AFAIK) any slowness when not used and will save people time, headache, and allow easier testing.

Thanks

tianon commented 2 years ago

Hmm, I'm inclined to suggest something like the following for users who still want/need phpdbg in images which no longer include it (to put another way, where it's not useful/relevant for the primary use case of the variant):

FROM php:7.4-fpm-alpine3.14
COPY --from=php:7.4-cli-alpine3.14 /usr/local/bin/phpdbg /usr/local/bin/
MinDBreaK commented 2 years ago

Thank you for the tip of the import from the CLI image.

However I still believe it should be included because while this is working, this force the user to download two images to use a very small binary. (IRRC from your PR). This result in higher build time, higher number of pulls, higher traffic and higher disk usage for IMHO, a very small benefit.

I hope you'll reconsider the removal of phpdbg

fieg commented 2 years ago

Some of our build pipelines are breaking because suddenly phpdbg is no longer present in older images. I agree with @MinDBreaK and also believe this change should be reverted. If it is really needed to remove phpdbg from the fpm image, I think it would be best to only do it for new images, not existing ones.

4c0n commented 2 years ago

I would like to see it restored as well. It's an unnecessary amount of hassle to change our builds and production setups, just because of the half reason that it's not useful for FPM. You mind as well remove the CLI from that container image entirely then, as that is exactly what we're using it with... What is the big deal with having it included? It's a tiny binary AFAIK and it doesn't seem to pose any security threats.

In short there seems little benefit in removing it and it's causing a lot of inconvience for everyone that was using it.

4c0n commented 2 years ago

Also arguments are not supported in the --from argument, so we cannot do this:

ARG PHP_VERSION=8.0
FROM php:${PHP_VERSION}-fpm-alpine
COPY --from=php:${PHP_VERSION}-cli-alpine /usr/local/bin/phpdbg /usr/local/bin/

making it even more inconvienent...

tianon commented 2 years ago

I agree that the workaround for that issue isn't ideal, but it's not too complicated:

ARG PHP_VERSION=8.0
FROM php:${PHP_VERSION}-cli-alpine AS php-cli
FROM php:${PHP_VERSION}-fpm-alpine
COPY --from=php-cli /usr/local/bin/phpdbg /usr/local/bin/

As for size, it's just shy of 20MiB, which is a non-trivial percentage of the total size of /usr/local in our images (which is ~80MiB in the CLI images which contain phpdbg).

4c0n commented 2 years ago

I agree that the workaround for that issue isn't ideal, but it's not too complicated:

ARG PHP_VERSION=8.0
FROM php:${PHP_VERSION}-cli-alpine AS php-cli
FROM php:${PHP_VERSION}-fpm-alpine
COPY --from=php-cli /usr/local/bin/phpdbg /usr/local/bin/

Sure we can make it a multistage build I suppose...

As for size, it's just shy of 20MiB, which is a non-trivial percentage of the total size of /usr/local in our images (which is ~80MiB in the CLI images which contain phpdbg). I guess that's a matter of perspective. I'd say that maybe a big percentage, but not a significant difference in size

Point is that a lot of us are using the CLI inside the FPM image (it is included after all). Just imagine a standard development setup using docker-compose, in most situations we will want to do something like docker-compose exec php [SOME_DEV_TOOL]. Like for example use phpdbg to run phpunit so that we can measure our code coverage. Sure we can do that using a CLI container as well, however most of us currently are not doing it that way. As has been pointed out above, this change requires all of us to pull in an extra image either in our build or seperately in order te be able to do that. It costs us more disk space, bandwidth and increases our build times, which totally diminishes or even counter acts the decrease in size of just one image by a very small amount of MiB in today's standards.

So for me this just makes no sense. If a product in a store is priced at 80 cents and you get a 25% discount making it only 60 cents, you're not going to be telling all your friends about the great deal you got.

tianon commented 2 years ago

80 cents vs 60 cents isn't really a fair comparison here though.

Imagine we have a Docker image that's downloaded 1 million times (which isn't exactly an exaggeration here). That 20MiB translates to roughly 20,000,000MiB of bandwidth in this scenario. I doubt we have anywhere near 25% of those 1m users who actually need phpdbg, so an extra 80MiB for even 10% of users (which I think is still an overestimate) is still only 8,000,000MiB.

I'm not completely opposed to putting it back in, but I think we're going to need a more compelling argument than just bandwidth and/or convenience.

tristanpemble commented 2 years ago

regardless of the reasons to introduce this; this was not a backward compatible change, which broke end users' pipelines, and further erodes trust in your ecosystem.

dercoder commented 1 year ago

For php-zts version the command

COPY --from=php-cli /usr/local/bin/phpdbg /usr/local/bin/

will not work as workaround

It would be very helpful to enable phpdbg in the ZTS version

SimonMacIntyre commented 1 year ago

in most situations we will want to do something like docker-compose exec php [SOME_DEV_TOOL]. Like for example use phpdbg to run phpunit so that we can measure our code coverage. Sure we can do that using a CLI container as well, however most of us currently are not doing it that way.

Same thing we are doing which is now annoyingly broken.

We rely on coverage in CI normally, but it's nice to be able to take a really quick look with phpdbg/pcov locally, for example when developing a new feature, double-checking your test coverage of it as you work through test writing.

What was once a very simple one-liner, is now an entirely new dockerfile since you can't just docker run from php-cli as now that image will be missing so many other extensions needed to run your tests etc. We use docker compose which references pre-built images, for our development environment.

So now we need a new Dockerfile.coverage just for the sake of running local coverage. 💩

SimonMacIntyre commented 1 year ago

For php-zts version the command

COPY --from=php-cli /usr/local/bin/phpdbg /usr/local/bin/

will not work as workaround

It would be very helpful to enable phpdbg in the ZTS version

@dercoder A couple things

dercoder commented 1 year ago

@SimonMacIntyre Thx but this will not work. All the extensions can not be loaded => NTS

SimonMacIntyre commented 1 year ago

Funny after working through this for a while, and updating recently to phpunit10 & php8.2 I found this: https://github.com/sebastianbergmann/phpunit/issues/5173#issuecomment-1418253018

Suddenly and without warning, I no longer have a horse in this race :D