Skip to content

Revert to compiling PHP from source #43

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 98 commits into from
Jan 29, 2023
Merged

Revert to compiling PHP from source #43

merged 98 commits into from
Jan 29, 2023

Conversation

mnapoli
Copy link
Member

@mnapoli mnapoli commented Jan 13, 2023

Because of #42 and remicollet/remirepo#228

Fixes #42

TODO left:

  • Build bref/fpm-internal-src
  • Cleanup lib-check
  • Adapt the release scripts
  • Update README

BTW we could use libedit for readline, but it appears to be a secondary alternative, and not without issues (e.g. docker-library/php#1187).

Let's stick to readline.
Both options seem to be acceptable, but we get readline out of the box on lambda, and there seems to be issues with libedit anyway (docker-library/php#1187)
We don't need that variable anymore since we set it when compiling PHP
@mnapoli
Copy link
Member Author

mnapoli commented Jan 22, 2023

Does the base-level image have a use outside of being effectively a build cache? Is there a reason it's shipped as an image rather than pushing the cache layers to a registry?

Indeed. Unless I'm missing something, it's now no longer pushed anywhere, just built for the build process. (it used to be published to Docker Hub)

Maybe out of scope for this PR but the caching approach could have a bit of an overhaul with use of --no-cache-filter and some stage refactoring to make specific parts of the build cache-bustable but enable the builds to resume later

I had a look at that option, but that would only cache less things so I'm not sure how that would improve things here?

There's significant overlap between the Dockerfiles of different architectures, having everything split out makes this a bit tricky to see what's going on.

Good point, thanks! The base-devel images had no reason to be split up anymore. If we indeed need to have different builds in the future, we can split them up again.

But I pushed a commit where I merged the 2 files together, so that's a win!

Now I need to figure out why the ARM builds have started failing with the cryptic errors:

buildx bake failed with: ERROR: failed to solve: failed to copy: invalid status response 403 Server failed to authenticate the request. Make sure the value of Authorization header is formed correctly including the signature. for https://vth0acprodeus2file0.blob.core.windows.net/daf72de42ddf4a22bb30b289c092b345/edc875780696ed11bf7a14cb652f465e?sv=2019-07-07&sr=b&sig=H1DVEU9wjTKeu03F8GMeV5vgP37AF9joAj4IF505jMA%3D&se=2023-01-21T12%3A32%3A57Z&sp=r&rscl=x-e2eid-f69ae48d-47ec417e-9d28c90a-d03bf09a

Maybe the build is taking too long and the credentials for uploading stuff to GitHub Actions cache have expired… Maybe there are too many requests. Maybe it's something else 🤷

@mnapoli
Copy link
Member Author

mnapoli commented Jan 22, 2023

Good point, thanks! The base-devel images had no reason to be split up anymore. If we indeed need to have different builds in the future, we can split them up again.

But I pushed a commit where I merged the 2 files together, so that's a win!

🤦 that was actually also the case for all the other Dockerfiles. I merged all of them together and reduced the number of lines of code drastically, thank you!

image

This is to avoid having the dangling base-devel-xx Docker images that are used only to cache internal steps (common throughout PHP versions).

Since Depot caches all intermediary layers very aggressively, that intermediary image that serves as a cache becomes useless.
This will help to solve build dependency issues and image visibility with Depot.dev. The added complexity (duplicated code) is very minor, and Docker will cache the build steps anyway, so I find that acceptable.
@deleugpn
Copy link
Member

deleugpn commented Jan 28, 2023

I know this won't be super relevant, but I just wanted to point out why I chose two different files. Variables makes things harder to mentally parse. You need to know possible values upfront and beginners unaware of how the system works are much less likely to know/understand what these variables will be. They optimize for writing code while we spent a lot more cumulative time reading it (think several developers reading/understanding the same code).

At the time when we were not compiling PHP anymore, I felt that it was a lot more useful to have static/duplicated code than code that has variables spread around it and having to understand the many places these variables flow around. In the end, we've spent 30+ years with a single CPU architecture and today we have 2. Chances are that we won't need more than 2 arch for another decade or so. Variables bring more value when they help reuse the code significantly more than just 2 times.

Part of this is moot point with compiling though, as it appears the entire Dockerfile is practically the same. It does make the Makefile a bit more complex to reason about, but everything is about trade-offs.

@mnapoli
Copy link
Member Author

mnapoli commented Jan 29, 2023

It's all working now, builds are fast (see #44), I'm very happy with the results and:

image

we switched to a more complex build while not exploding in complexity 🎉

Thanks a lot @ciaranmcnulty for the advice, and @deleugpn for adding context (indeed we're iterating a lot on the structure as the context drastically changes every time, the previous build had completely different Dockerfiles for different architectures).

Anyway, I gave the new layers a try and it's all working, tests are passing, let's get this merged and benchmark it!

@mnapoli mnapoli merged commit 955f907 into main Jan 29, 2023
@mnapoli mnapoli deleted the compile-php branch January 29, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add support for HTTP2 requests in Curl
3 participants