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

Restore phpdbg in fpm images #1260

Open
MinDBreaK opened this issue Mar 3, 2022 · 14 comments
Open

Restore phpdbg in fpm images #1260

MinDBreaK opened this issue Mar 3, 2022 · 14 comments

Comments

@MinDBreaK
Copy link

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
Copy link
Member

tianon commented Mar 3, 2022

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
Copy link
Author

MinDBreaK commented Mar 10, 2022

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
Copy link

fieg commented Mar 16, 2022

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
Copy link

4c0n commented Mar 24, 2022

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
Copy link

4c0n commented Mar 24, 2022

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
Copy link
Member

tianon commented Mar 24, 2022

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
Copy link

4c0n commented Mar 25, 2022

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
Copy link
Member

tianon commented Mar 25, 2022

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
Copy link

tristanpemble commented Aug 12, 2022

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
Copy link

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
Copy link

SimonMacIntyre commented Mar 3, 2023

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
Copy link

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

  • Should be --from=php:cli or --from=php:cli-alpine or similar.
  • You will have issues if you try to use that binary from php:cli on an image that is from php:cli-alpine for example. Make sure you match up where you are copying it from, to the proper alignment with your source image(matching linux).

@dercoder
Copy link

dercoder commented Mar 3, 2023

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

@SimonMacIntyre
Copy link

Funny after working through this for a while, and updating recently to phpunit10 & php8.2 I found this: sebastianbergmann/phpunit#5173 (comment)

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

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

No branches or pull requests

7 participants