Feat: parallel build multiple php #4
Conversation
On OSX, with Docker Desktop, this wouldn't build in parallel Until I ran the following command docker buildx create --name parallel-builder --use --driver docker-container --bootstrap Once that was done, I switched the target default, to group default with targets And ran in the parallel-builder! docker buildx bake --builder=parallel-builder Note: this builds upon a dockerfile edit to remove some win32 garbage from php5.x
|
Feel free to close if not something you are interested in |
| rm -f /local/src/php-src/main/php_glob.c && \ | ||
| touch /local/src/php-src/main/php_glob.c && \ |
There was a problem hiding this comment.
This takes out some nonsense win32 php_glob code, which is only present in 8.5.x (alphas) so far
There was a problem hiding this comment.
Does it hurt if it stays? It shouldn't include if it's only for win32 in the compile stage either. In any case, it requires a comment.
There was a problem hiding this comment.
It refuses to compile; but I am open to other fixes
There was a problem hiding this comment.
This is only for php 8.5, as no other versions use this code, and it looks like the wasm version does not either.
There was a problem hiding this comment.
Right, but why would it even want to compile that file? What are the errors during compilation? Perhaps we can fix it instead.
There was a problem hiding this comment.
I have that information here php/php-src#19152 along with a few things I tried and their output.
iluuu1994 blamed your repo build... I'm fairly certain that this is a php-src issue.
From my perspective the php-src build should not require explicit setting of low-level compiler defines by consumers, instead focusing on the configure script. This seems to be true of 8.0, 8.1, 8.2, 8.3 and 8.4 with this workaround only for 8.5. The files do not exist in any of the other minors for 8.x major.
The Dockerfile at the point I've forked it contains no defines, and although I did try some CFLAGS and some LDFLAGS to work around the source gen, and even tried tinkering with the files themselves (which works if I remove reallocarray redefinitions). I Feel the right place for this kind of tinkering is in core repo; and that their clearly communicated desire is not to solve for problems introduced internally, but to outsource that to consumers of the php-src repo.
I did not try to set any LDFLAGS in the PR, as I am uncertain how that would interact with emscripten; you may know more than me, have more time or just be bold enough to try and suppose more things; I'd welcome the input; but all I can offer is removing 8.5 alpha at this point.
I'm also not terribly invested in fighting a battle with the core to solve issues they introduced to their own codebase.
There was a problem hiding this comment.
It seems to be an emscripten issue: emscripten-core/emscripten#24771
There was a problem hiding this comment.
I've read that thread and notice it is a totally different error from any I've seen
32.04 /local/src/php-src/main/php_glob.c:207:1: error: static declaration of 'reallocarray' follows non-static declaration
32.04 207 | reallocarray(void *optr, size_t nmemb, size_t size)
32.04 | ^
32.04 /local/src/emsdk/upstream/emscripten/cache/sysroot/include/stdlib.h:150:7: note: previous declaration is here
32.04 150 | void *reallocarray (void *, size_t, size_t);
32.04 | ^
32.04 1 error generated.
32.05 make: *** [Makefile:1720: main/php_glob.lo] Error 1
32.05 make: *** Waiting for unfinished jobs....
33.67 emmake: error: 'make -j4' failed (returned 2)
Did you follow different steps to get the error? We're both within a Dockerfile, so I'm a little confused to why you have different output. Has PHP changed that file since I opened the issue?
|
|
||
| target "php-8-0" { | ||
| output = ["type=local,dest=./builds/auto/build-8.0.x"] | ||
| tags = ["php-wasm"] |
There was a problem hiding this comment.
There is something wonky going on here with whitespace. Can you make it consistent (with preferably tabs)?
There was a problem hiding this comment.
Would you mind if I add an editorconfig to the repo? That should be understood by most editors, and should make fixing faster and easier; and transferrable to all other commits.
There was a problem hiding this comment.
I don't know what these are. What would that entail?
There was a problem hiding this comment.
A .editorconfig is just a file which describes for supporting IDEs; rules, which can help the IDE know tabs vs spaces and how many spaces to substitute a tab for. I'm unsure if the tabs come from the original or lack of an editorconfig led to the inconsistent spacing; but it would certainly help to make rules around files and editing explicit.
Edit: to be clear you are saying you prefer tabs in this file right?
| rm -f /local/src/php-src/main/php_glob.c && \ | ||
| touch /local/src/php-src/main/php_glob.c && \ |
There was a problem hiding this comment.
Does it hurt if it stays? It shouldn't include if it's only for win32 in the compile stage either. In any case, it requires a comment.
|
Huh? It's exactly the same error. What are you talking about?
|
I see now, it is in there at the top. I'd gone down and was seeing a linker bug in the post; whereas I was getting an error before linking. I'm even more confused (which is fine) that I get a working build from the 8.5 compile in this, but you've got a linker error... I'll chalk this up to me likely not understanding the problem as deeply as you do, and just being annoyed that something compiles and works with files edited under PHP, but not without. (which apparently has a deeper root cause, that an upstream accepts as their fault) |
|
I am going to close this specific PR, as there are a bunch of different unrelated things in it. Can you split out an editor config change into its own PR, and the multiple versions one? For now, until emscripten-core/emscripten#24775 is fixed and released, the build for PHP 8.5.x will just not work. |
|
The build is live and working Derick, Thank you for your time and for this repo, but I do think beyond submitting this patch; I'll leave it open and in whatever license you have or do not have. If anyone wants a unified patch, github has a feature I'm using in this link unified patch |
I don't expect you to want or to merge this, but just in-case anyone else wonders how to use the docker buildx
See commits for details on how to get parallel builds working on Docker-desktop of OSX