Fixing build regression [BISON COMPILE] #871
Conversation
seanmorris
changed the title
seanmorris
added
[Type] Bug
|
I tried running the updated build script and ran into the following errors: I expected the build to stop at this point, but it kept going. |
seanmorris
changed the title
Thats why we need to maintain a compile step. We can't guarantee that we know what architecture the user will be building on. I've gone ahead and restored the compilation, and made sure it only builds Bison twice, once for v2.7 and again for v3. I've confirmed the full build works on both |
| # Must be 2.7 for PHP <= 7.3 | ||
| # Must be 3 for PHP >= 7.4 | ||
| ARG BISON_VERSION | ||
| FROM playground-php-wasm:bison${BISON_VERSION} |
There was a problem hiding this comment.
Is there any way to compute the bison version in this Dockerfile here instead of in build.js?
There was a problem hiding this comment.
@adamziel If we do that, we can't cache it and we'll be forced to rebuild on every iteration.
I think we should move as much of this calculation into the build JS as possible, then think about re-ordering the dockerfile with the implicit layer-caching mechanism in mind.
If we can cache the identical steps between different builds, and re-use them, we should be able to get the build to run a lot faster.
There was a problem hiding this comment.
If that logic lives in build.js, then the Dockerfile is no longer self-contained – you need prior knowledge about the mapping between specific PHP versions and Bison versions to be able to use it. Is there a way to keep that mapping inside the Dockerfile and still benefit from caching?
There was a problem hiding this comment.
@adamziel Nope. If Docker considers a PHP_VERSION arg that is different from that of a previous build, then the layer-hash will differ and the cache will not be used for that build.
I think that the value we gain from a shorter build is greater than the value we lose from compromising a self-contained dockerfile. Our build system architecture is already relatively highly coupled, and the existing dockerfile already has dependencies it cannot invoke on its own.
I think this still represents a step forward.
Edit: I can put some error-checking logic in to clarify things a bit.
There was a problem hiding this comment.
Errors will print in red if incompatible versions are provided:
wordpress-playground/packages/php-wasm/compile/php/Dockerfile
Lines 24 to 39 in 1ef26cc
There was a problem hiding this comment.
If Docker considers a PHP_VERSION arg that is different from that of a previous build, then the layer-hash will differ and the cache will not be used for that build.
Using the --download-only flag for apt would do the trick here, though. Both Bison 2.7 and Bison 3.0 may then exist in the docker cache as static files, and then a bash if could just plug-in the correct version depending on the PHP_VERSION.
the value we gain from a shorter build
How much shorter is it, though? Are we talking about minutes or seconds?
Our build system architecture is already relatively highly coupled, and the existing dockerfile already has dependencies it cannot invoke on its own.
Oh, interesting, I haven't thought of it like that. What would be an example of such a dependency?
|
It seems like the bison change may require some more iterations. Would you mind splitting this PR in two? One PR would only fix the paths, as that fix seems urgent. The bison update would make more sense as a separate PR, as it blocks fixing the build regression and is only a quality of life improvement. |
seanmorris
changed the title
|
About this PR's description – it's not really fixing a build regression, right? That's the other PR with the same title. This one seems to be splitting Bison 3 into a separate Dockerfile for speed gains. What is the order of magnitude of the speed increase? Is this one worth pursuing? |
This fixes Bison 2.7. If thats broken we can't build PHP <=7.3. |
|
Is Bison 2.7 broken in any way without this PR, though? |
@adamziel Without it, I'm getting the error below for Fails on: Works on: |
|
Similar error on my Debian laptop, but I need to checkout #887 to get to this point: Git Commit: |
seanmorris
changed the title
@adamziel This PR fixes the issue a little more simply. Let me know what you think: #892 |
Couldn't agree more. "Accidentally working" is my biggest fear in software. |
## What is this PR doing? This corrects some paths in the docker file used to copy libraries into the build image. ## What problem is it solving? Newer versions of Docker will ignore leading `../` that lead outside of the build context, but older versions will throw an error in that case. ## How is the problem addressed? This removes the extra `../` at the start of the paths in question. Newer versions of docker will ignore `../`s that lead outside of the build context, but older versions will throw an error. The following paths will work on later versions but fail on earlier ones: ``` # Bring in the libraries RUN mkdir -p /root/lib/include /root/lib/lib /libs COPY ../../../../../../../../../../bison2.7/dist/ /libs/bison2.7 COPY ../../../../../../../../../../libedit/dist/root/lib /root/lib COPY ../../../../../../../../../../libiconv/dist/root/lib /root/lib COPY ../../../../../../../../../../libncurses/dist/root/lib /root/lib COPY ../../../../../../../../../../libopenssl/dist/root/lib /root/lib COPY ../../../../../../../../../../libpng16/dist/root/lib /root/lib COPY ../../../../../../../../../../libsqlite3/dist/root/lib /root/lib COPY ../../../../../../../../../../libxml2/dist/root/lib /root/lib COPY ../../../../../../../../../../libz/dist/root/lib /root/lib COPY ../../../../../../../../../../libzip/dist/1.2.0/root/lib /libs/libzip/1.2.0 COPY ../../../../../../../../../../libzip/dist/1.9.2/root/lib /libs/libzip/1.9.2 COPY ../../../../../../../../../../oniguruma/dist/root/lib /root/lib ``` ## Testing Instructions Checkout the branch and pull dependencies: ``` git checkout sm-correcting-build-paths npm install ``` Build the libraries: ``` cd packages/php-wasm/compile make clean make all ``` In a new terminal, build PHP 7.4 - 8.3: (PHP <=7.3 is fixed in #871) ``` npm run recompile:php:web:light:8.3 npm run recompile:php:web:light:8.2 npm run recompile:php:web:light:8.1 npm run recompile:php:web:light:8.0 npm run recompile:php:web:light:7.4 ``` ``` npm run recompile:php:web:kitchen-sink:8.3 npm run recompile:php:web:kitchen-sink:8.2 npm run recompile:php:web:kitchen-sink:8.1 npm run recompile:php:web:kitchen-sink:8.0 npm run recompile:php:web:kitchen-sink:7.4 ``` ``` npm run recompile:php:node:8.3 npm run recompile:php:node:8.2 npm run recompile:php:node:8.1 npm run recompile:php:node:8.0 npm run recompile:php:node:7.4 ``` Ensure the build works: ``` npm run build npm run dev ``` Navigate to the following URLs: * http://localhost:5400/website-server/?plugin=web-stories&php-extension-bundle=kitchen-sink&php=8.3 * http://localhost:5400/website-server/?plugin=web-stories&php-extension-bundle=kitchen-sink&php=8.2 * http://localhost:5400/website-server/?plugin=web-stories&php-extension-bundle=kitchen-sink&php=8.1 * http://localhost:5400/website-server/?plugin=web-stories&php-extension-bundle=kitchen-sink&php=8.0 * http://localhost:5400/website-server/?plugin=web-stories&php-extension-bundle=kitchen-sink&php=7.4
## What is this PR doing? This PR corrects the same Bison2.7 issue as #871, in a much simpler fashion. ## What problem is it solving? Bison2.7 will not be built in some cases, blocking the build of PHP <= 7.3. ## How is the problem addressed? This invokes dependency builds before building PHP. ## Testing Instructions Checkout the branch & install the dependencies: ``` git checkout sm-make-deps-before-compile npm install ``` In a separate terminal, clear the artifacts: ```bash docker system prune -af cd packages/php-wasm/compile make clean ``` Run the build and ensure it succeeds: ```bash npm run recompile:php:node npm run recompile:php:web ``` Start the dev server: ```bash npm run dev ``` Then, navigate to: * http://localhost:5400/website-server/?php=8.3 * http://localhost:5400/website-server/?php=8.2 * http://localhost:5400/website-server/?php=8.1 * http://localhost:5400/website-server/?php=8.0 * http://localhost:5400/website-server/?php=7.4 * http://localhost:5400/website-server/?php=7.3 * http://localhost:5400/website-server/?php=7.2 * http://localhost:5400/website-server/?php=7.1 * http://localhost:5400/website-server/?php=7.0 ... and ensure that wordpress works as-expected in each environment.
|
Just a note: I ran into this issue for PHP 7.3, and it turns out it was for a very good reason. My Mac is currently intel-based, and we are using a binary for ARM. It would probably be good to fix this at some point, but not being able to build PHP 7.3 is not currently a blocker for me. |
|
Ah, I guess the architecture mismatch with the prebuilt binary is the known problem. |
|
@adamziel, I was able to get the PHP 7.3 light build succeeding with this change: It doesn't cache the build output across PHP builds, but it seemed reasonable as an initial fix since builds for PHP 7.3 and below appear to be completely broken for intel right now. Is this the kind of fix you had in mind? A few of notes: This has not been tested on an arm64 device because I do not have one handy, and I have not tested the build output yet because the I took hints from the bison 2.7 Dockerfile here but don't know the reason for changing the libiconv prefix here: Do you recall why this path setting was important or necessary? Also related to the bison 2.7 Dockerfile, the URL was adjusted to use HTTPS instead of HTTP to reduce the possibility of MITM attacks. If we are still using the original Dockerfile, we should probably fix it to use HTTPS as well. |
@brandonpayton I think that's reasonable! I'm happy to test on an arm machine – can I just apply that patch and run the build process?
I can't remember either. If it runs without that prefix change, it's most likely not needed.
❤️ |
@adamziel, great! Yep, just apply the patch and I went ahead and removed the libiconv prefix and didn't see any build problems. I also tested loading Playground with some PHP versions 7.3 and below, and there didn't appear to be problems loading WP. (Actually, there was a problem loading WP for PHP 7.1, but I experienced the same when switching to |
|
Superseded by #1115 |
What is this PR doing?
This installs the necessary version of Bison in a separate docker image and uses that image as a base image for the PHP build. This allows us to only build Bison twice: once for v2.7 and again for v3.
What problem is it solving?
Currently Bison 2.7 isn't installing correctly and thus we cannot build PHP <=7.3.
wordpress-playground/packages/php-wasm/compile/php/Dockerfile
Lines 4 to 5 in a4c3ad0
How is the problem addressed?
Testing Instructions
Checkout the branch & install the dependencies:
Run the build and ensure it succeeds:
Start the dev server:
Then, navigate to:
... and ensure that wordpress works as-expected in each environment.