Skip to content

Use RC5 image #74

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

Merged
merged 3 commits into from
Oct 31, 2022
Merged

Use RC5 image #74

merged 3 commits into from
Oct 31, 2022

Conversation

withinboredom
Copy link
Member

@withinboredom withinboredom commented Oct 31, 2022

This migrates to the new RC5 image instead of cloning master of php-src.

New image sizes:

  • alpine: 60.14 MiB
  • debian: 190.31 MiB

@withinboredom withinboredom marked this pull request as ready for review October 31, 2022 07:45
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@withinboredom
Copy link
Member Author

During this, I discovered that the alpine image doesn't have --enable-embed in the php Dockerfile. I left a comment on docker-library/php#1331 (review), but this will likely need an issue on that repo to track it.

@withinboredom withinboredom merged commit 2b7e649 into main Oct 31, 2022
@withinboredom withinboredom deleted the update-dockerfile branch October 31, 2022 10:13
@back-2-95
Copy link
Contributor

@withinboredom That docker-library/php#1331 is merged

@back-2-95
Copy link
Contributor

@withinboredom Few questions:

  • If we now have no need to clone any php-src branch why we need to build php in php-base stage?
  • If final stage extends from php-base, it has all the dev stuff needed for building in it. Extra weight?

In other words, if we're now happy with official PHP RC5 as it is, we should just build FrankenPHP and then combine it with clean PHP RC5 ? Just trying to understand what I'm reading from the Dockerfile diff.

@withinboredom
Copy link
Member Author

If we now have no need to clone any php-src branch why we need to build php in php-base stage?

It's pretty much explicitly needed for alpine (#74 (comment)), and I left it in for both platforms so we can control the build of PHP, which is desirable since it appears getting "bugfix" PRs merged to the base image can take awhile. This gives us some independence from their release cycle and keeps things consistent.

If final stage extends from php-base, it has all the dev stuff needed for building in it. Extra weight?

I think this is normal? The "dev stuff" is required to build extensions, right? There shouldn't be anything not already in the original image (except for libphp in the alpine image).

I suspect the build of PHP does contribute to some image size increase (I'm not 100% sure how Docker does deduping -- especially without a zfs backend); however, that should be negligible once you start piling extensions on the image. For example, one of my internal images adds another 200mb of extensions and several hundred megs in assets. So the cost of storage is negligible when compared to a final image.

@back-2-95
Copy link
Contributor

First one 👍

Second, the "dev stuff": it it's needed for the final product (to eg. run some operations) then it's argumented.
If some packages were installed just for the time being (building stuff), then it should be either removed or use multistage (where you don't need to worry about cleaning up as you just copy the outcome = build stuff).

Still, your image sizes seem quite small already so I'm not totally sure if something happened already (cleaning).

@back-2-95
Copy link
Contributor

Alpine docker file has at least these:

  • docker-php-source delete
  • apk del --no-network .build-deps

Bullseye has some savedAptMark with a comment reset apt-mark's "manual" list so that "purge --auto-remove" will remove all build dependencies

so there is cleaning 👍

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.

3 participants