Fixing Build Regression [BISON] #892

seanmorris · 2023-12-22T01:22:04Z

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:

docker system prune -af
cd packages/php-wasm/compile
make clean

Run the build and ensure it succeeds:

npm run recompile:php:node
npm run recompile:php:web

Start the dev server:

npm run dev

Then, navigate to:

... and ensure that wordpress works as-expected in each environment.

dmsnell · 2023-12-25T19:31:14Z

Hm. I was trying to test this but npm install and npm ci both fail when attempting to compile vips, claiming that libarchive isn't available. I have node@21.1.0 and npm@8.11.0, and although I don't think it should matter, I actually have libvips and livarchive installed on my system via Homebrew.

npm ERR! Package libarchive was not found in the pkg-config search path.
npm ERR! Perhaps you should add the directory containing `libarchive.pc'
npm ERR! to the PKG_CONFIG_PATH environment variable
npm ERR! Package 'libarchive', required by 'vips', not found

I reviewed the project README and I'm not seeing any obvious missing dependency. Clearing out node_modules didn't help either. Do you have any advice on this? @seanmorris

dmsnell · 2023-12-25T19:34:10Z

Do you have any advice on this? @seanmorris

Interestingly, I removed vips from my machine and the dependencies built fine. Not sure why, but that's not fun. Created #904 to note it.

dmsnell · 2023-12-25T23:08:43Z

Ran some smoke tests and everything seemed to be working across various PHP versions.

The node build took 24 min to complete
The web build took around 28 min to complete

dmsnell · 2023-12-25T23:11:41Z

packages/php-wasm/compile/build.js

@@ -167,7 +167,10 @@ if (!requestedVersion || requestedVersion === 'undefined') {
 const sourceDir = path.dirname(new URL(import.meta.url).pathname);

 // Build the base image
-await asyncSpawn('make', ['base-image'], { cwd: sourceDir, stdio: 'inherit' });
+await asyncSpawn('make', ['all', 'base-image'], {


does the base-image depend on the all target or is that incidental here?

otherwise it seems like the Makefile would be the place to list dependencies, wouldn't it? why aren't we putting it there?

@dmsnell The base-image target does not depend on the all target.

wordpress-playground/packages/php-wasm/compile/Makefile

Lines 1 to 2 in db34c27

base-image: base-image/.ready

base-image/.ready:

(Its generally poor practice to use a PHONY target as a dependency because that means anything downstream from that target would be rebuilt on every invocation.)

@dmsnell Actually its fine so long as phony targets are used as dependencies ONLY of other phony targets:

wordpress-playground/packages/php-wasm/compile/Makefile

Lines 9 to 13 in c4cfd1e

.PHONY: all libz libzip libpng16 libxml2 libopenssl libsqlite3 libiconv libncurses libedit bison2.7 oniguruma base-image clean

all: libz libzip libpng16 libxml2 libopenssl libsqlite3 libiconv libncurses libedit bison2.7 oniguruma base-image

clean:

@seanmorris I think we might have jumped a little too quickly here. I wasn't asking if base-image currently is encoded as depending on all, but rather was asking about your change, which seemed to be encoding a dependency outside of the Makefile. previously we asked for base-image, but with your change we were also asking for all. this communicated to me that all is necessary to build base-image, and if that were actually true then I think it would be best to leave the dependencies inside make where they are handled robustly.

as for .PHONY I think there may also be some misunderstandings. we should use them wherever they are part of a dependency hierarchy, whether or not they are a dependency of another .PHONY or not. the distinction is purely whether the .PHONY targets need to be rebuilt every time make runs. for example, it could be that some build target needs to download a configuration from the internet. that download would be a candidate .PHONY target, but that doesn't force other targets to rebuild because we could make those other targets depend on the downloaded configuration file directly, and simply not update it if the download is the same as it previously was.

.PHONY: fetch-config fetch-config: curl https://my.site/config.json -o /tmp/config.json diff -q -s /tmp/config.json ./config.json if [ $? -ne 0 ]; then mv /tmp/config.json ./config.json; fi libsomething: ./config.json build-lib-something

making all a .PHONY target is common because people generally don't want to put sub-dependencies on the all target, and even if all is .PHONY and runs every time, if the dependencies of the dependencies haven't changed than nothing much will run.

so we want to be judicious and thoughtful when applying .PHONY, but it's also not something we want to try and avoid; we only want to understand its purpose and value and use it where appropriate.

I can see that we have a number of ways we could improve the Makefile, but those don't need to occur in this PR.

dmsnell · 2023-12-26T22:00:16Z

packages/php-wasm/compile/Makefile

+
+# If you write a rule whose recipe will not create the target file,
+# the recipe will be executed every time the target comes up for remaking.
+# https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html


Generally I love comments like this, but I also think it's okay if we don't add it, because .PHONY is a pretty standard feature of Makefile syntax and there should be ample documentation available for someone to understand it. We don't document the other parts of the Makefile syntax here, and it's probably okay to let the PHONY exist as is.

dmsnell · 2023-12-26T22:03:13Z

packages/php-wasm/compile/Makefile

+# If you write a rule whose recipe will not create the target file,
+# the recipe will be executed every time the target comes up for remaking.
+# https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html
+.PHONY: all libz libzip libpng16 libxml2 libopenssl libsqlite3 libiconv libncurses libedit bison2.7 oniguruma base-image clean


do all of these need to be .PHONY? I wouldn't ask other than it seems like this patch is now adding that property to the targets, and it wasn't there before.

of course, adding .PHONY will ensure they're built every time we run make, but particularly for the libraries like libpng16 and libzip it seems like we could be more efficient in general if we based the build on some deterministic element like a version number, build architecture, or source files, etc…

I guess the important question is whether you found something in this PR that was broken because dependencies didn't get rebuilt when they should have. if that's the case then .PHONY seems like a fair addition; if not, maybe we let it stay the way it was and address that separately.

The Makefile should have originally listed these targets as .PHONY because the names being used are the same as the directories in which the actual artifacts lie. Unintentionally, if that directory has a modification date newer than its dependencies, it would then INCORRECTLY be marked as cached and skipped. Adding the .PHONY target instructs Make to skip looking at the directory modification time and instead look at the listed dependencies only.

Great. This makes more sense now, as I didn't realize that the directories of the same name as the phony targets existed (because I hadn't looked that deeply outside of the diff).

Let's make sure to add this in the commit description, that this fixes a bug we already had, wherein the .PHONY should have always been there.

dmsnell · 2023-12-26T22:03:54Z

packages/php-wasm/compile/Makefile

+	rm -rf ./libedit/dist
+	rm -rf ./bison2.7/dist
+	rm -rf ./oniguruma/dist
+


these don't need to appear first. without any arguments, make will run the first target, which is why all is a common first target. clean doesn't need to move unless there's a reason to do it.

dmsnell · 2023-12-26T22:05:18Z

packages/php-wasm/compile/build.js

@@ -167,7 +167,10 @@ if (!requestedVersion || requestedVersion === 'undefined') {
 const sourceDir = path.dirname(new URL(import.meta.url).pathname);

 // Build the base image
-await asyncSpawn('make', ['base-image'], { cwd: sourceDir, stdio: 'inherit' });
+await asyncSpawn('make', [], {


it would be nice here to leave some explicit intention in the code. although it happens to work with the changes you made to the Makefile, we've lost the linkage between asking for what we want (with the pre-existing base-image) and running make and hoping it does what we need (because no more target is listed).

adding base-image here would be helpful, or all, or anything to communicate to our future selves what our purupose was in running make.

dmsnell · 2023-12-26T22:20:09Z

packages/php-wasm/compile/build.js

@@ -167,7 +167,10 @@ if (!requestedVersion || requestedVersion === 'undefined') {
 const sourceDir = path.dirname(new URL(import.meta.url).pathname);

 // Build the base image
-await asyncSpawn('make', ['base-image'], { cwd: sourceDir, stdio: 'inherit' });
+await asyncSpawn('make', ['all', 'base-image'], {


@seanmorris I think we might have jumped a little too quickly here. I wasn't asking if base-image currently is encoded as depending on all, but rather was asking about your change, which seemed to be encoding a dependency outside of the Makefile. previously we asked for base-image, but with your change we were also asking for all. this communicated to me that all is necessary to build base-image, and if that were actually true then I think it would be best to leave the dependencies inside make where they are handled robustly.

as for .PHONY I think there may also be some misunderstandings. we should use them wherever they are part of a dependency hierarchy, whether or not they are a dependency of another .PHONY or not. the distinction is purely whether the .PHONY targets need to be rebuilt every time make runs. for example, it could be that some build target needs to download a configuration from the internet. that download would be a candidate .PHONY target, but that doesn't force other targets to rebuild because we could make those other targets depend on the downloaded configuration file directly, and simply not update it if the download is the same as it previously was.

.PHONY: fetch-config fetch-config: curl https://my.site/config.json -o /tmp/config.json diff -q -s /tmp/config.json ./config.json if [ $? -ne 0 ]; then mv /tmp/config.json ./config.json; fi libsomething: ./config.json build-lib-something

making all a .PHONY target is common because people generally don't want to put sub-dependencies on the all target, and even if all is .PHONY and runs every time, if the dependencies of the dependencies haven't changed than nothing much will run.

so we want to be judicious and thoughtful when applying .PHONY, but it's also not something we want to try and avoid; we only want to understand its purpose and value and use it where appropriate.

I can see that we have a number of ways we could improve the Makefile, but those don't need to occur in this PR.

dmsnell

Thanks @seanmorris - looks good in the merge.

adamziel · 2023-12-27T08:10:04Z

Just to confirm — does this PR:

Only rebuilds bison and only if needed (e.g. missing pre-built library or an architecture mismatch)?
Reuses the pre-built wasm artifacts shipped with the repo for libpng and other libraries?

Or does it always trigger a full rebuild? It’s not apparent to me from the description and I’d like to avoid a full rebuild each time.

if this Pr only rebuilds bison, we’re good. However, if it does trigger a full rebuild, let’s revert and explore what I suggested in the other PR: #871 (comment)

dmsnell · 2023-12-28T16:20:11Z

sorry @adamziel for missing the comment you linked; I may have misunderstood this PR's goal, which I took from the PR's description and chats with @seanmorris.

as he and I discussed, I think there definitely are mislabeled or missing dependencies, and it sounds like your suggestion would address that more directly.

if there's a problem occurring because we're not building bison, this PR fixes that problem bluntly by making sure it's built while building the base image.

I'll let @seanmorris share his thoughts before I do any reverts, as I think we can still move the two ideas together first by making the build slow but reliable (if this does that) and then by optimizing the Makefile.

Reverts #892 Context: #892 (comment)

Run make all before compiling base image.

db34c27

seanmorris changed the title ~~Fixing Build Regression [Part II - ALT]~~ Fixing Build Regression [BISON] Dec 22, 2023

seanmorris self-assigned this Dec 22, 2023

seanmorris requested a review from adamziel December 22, 2023 01:27

This was referenced Dec 22, 2023

Fixing build regression [BISON COMPILE] #871

Closed

Fixing build regression [PATHS] #887

Merged

seanmorris added [Type] Bug An existing feature does not function as intended [Feature] PHP.wasm labels Dec 22, 2023

seanmorris mentioned this pull request Dec 22, 2023

Correcting OOB & Prevent Crash on Saving Large Post #870

Closed

dmsnell reviewed Dec 25, 2023

View reviewed changes

seanmorris added 5 commits December 26, 2023 13:59

Simplifying Makefile.

1a80342

Reverting artifacts.

8b8d60b

Reverting artifacts.

9df50ac

Merge branch 'trunk' into sm-make-deps-before-compile

3a0548f

Reverting artifacts.

c4cfd1e

seanmorris requested a review from dmsnell December 26, 2023 20:11

dmsnell reviewed Dec 26, 2023

View reviewed changes

seanmorris added 4 commits December 26, 2023 17:53

Makefile comment.

fcb65d3

Redacting

c8574a8

Make all.

5601da8

Resorting makefile.

15cca11

dmsnell approved these changes Dec 26, 2023

View reviewed changes

seanmorris merged commit 1ef5bf6 into trunk Dec 26, 2023
5 checks passed

seanmorris deleted the sm-make-deps-before-compile branch December 26, 2023 23:41

bgrgicak mentioned this pull request Jan 10, 2024

Revert "Fixing Build Regression [BISON]" #929

Merged

adamziel pushed a commit that referenced this pull request Jan 10, 2024

Revert "Fixing Build Regression [BISON]" (#929)

93f2fbc

Reverts #892 Context: #892 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing Build Regression [BISON] #892

Fixing Build Regression [BISON] #892

seanmorris commented Dec 22, 2023 •

edited

Loading

dmsnell commented Dec 25, 2023

dmsnell commented Dec 25, 2023 •

edited

Loading

dmsnell commented Dec 25, 2023

dmsnell Dec 25, 2023

seanmorris Dec 26, 2023 •

edited

Loading

seanmorris Dec 26, 2023 •

edited

Loading

dmsnell Dec 26, 2023

dmsnell Dec 26, 2023

dmsnell Dec 26, 2023

seanmorris Dec 26, 2023 •

edited

Loading

dmsnell Dec 26, 2023

dmsnell Dec 26, 2023

dmsnell Dec 26, 2023

dmsnell Dec 26, 2023

dmsnell left a comment

adamziel commented Dec 27, 2023 •

edited

Loading

dmsnell commented Dec 28, 2023

	.PHONY: all libz libzip libpng16 libxml2 libopenssl libsqlite3 libiconv libncurses libedit bison2.7 oniguruma base-image clean

	all: libz libzip libpng16 libxml2 libopenssl libsqlite3 libiconv libncurses libedit bison2.7 oniguruma base-image

	clean:

Fixing Build Regression [BISON] #892

Fixing Build Regression [BISON] #892

Conversation

seanmorris commented Dec 22, 2023 • edited Loading

What is this PR doing?

What problem is it solving?

How is the problem addressed?

Testing Instructions

dmsnell commented Dec 25, 2023

dmsnell commented Dec 25, 2023 • edited Loading

dmsnell commented Dec 25, 2023

Choose a reason for hiding this comment

seanmorris Dec 26, 2023 • edited Loading

Choose a reason for hiding this comment

seanmorris Dec 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmorris Dec 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell left a comment

Choose a reason for hiding this comment

adamziel commented Dec 27, 2023 • edited Loading

dmsnell commented Dec 28, 2023

seanmorris commented Dec 22, 2023 •

edited

Loading

dmsnell commented Dec 25, 2023 •

edited

Loading

seanmorris Dec 26, 2023 •

edited

Loading

seanmorris Dec 26, 2023 •

edited

Loading

seanmorris Dec 26, 2023 •

edited

Loading

adamziel commented Dec 27, 2023 •

edited

Loading