-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
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
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)
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?
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:
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 🤷 |
🤦 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! |
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.
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 |
It's all working now, builds are fast (see #44), I'm very happy with the results and: 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! |
Because of #42 and remicollet/remirepo#228
Fixes #42
TODO left:
bref/fpm-internal-src