Skip to content
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.

Sign up for GitHub

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

Fixing Build Regression [BISON] #892

Merged
merged 10 commits into from
Dec 26, 2023
+10 −3
Merged

Fixing Build Regression [BISON] #892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
db34c27
Run make all before compiling base image.
seanmorris Dec 22, 2023
1a80342
Simplifying Makefile.
seanmorris Dec 26, 2023
8b8d60b
Reverting artifacts.
seanmorris Dec 26, 2023
9df50ac
Reverting artifacts.
seanmorris Dec 26, 2023
3a0548f
Merge branch 'trunk' into sm-make-deps-before-compile
seanmorris Dec 26, 2023
c4cfd1e
Reverting artifacts.
seanmorris Dec 26, 2023
fcb65d3
Makefile comment.
seanmorris Dec 26, 2023
c8574a8
Redacting
seanmorris Dec 26, 2023
5601da8
Make all.
seanmorris Dec 26, 2023
15cca11
Resorting makefile.
seanmorris Dec 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/php-wasm/compile/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
.PHONY: all libz libzip libpng16 libxml2 libopenssl libsqlite3 libiconv libncurses libedit bison2.7 oniguruma base-image clean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@seanmorris seanmorris Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


base-image: base-image/.ready

base-image/.ready:
cd base-image; docker build -f ./Dockerfile -t playground-php-wasm:base .

Expand Down Expand Up @@ -28,7 +31,7 @@ libzip/dist/1.9.2/root/lib/lib/libzip.a: base-image libz
libpng16: libpng16/dist/root/lib/lib/libpng16.a
libpng16/dist/root/lib/lib/libpng16.a: base-image libz
mkdir -p ./libpng16/dist/root/lib
docker build -f ./libpng16/Dockerfile -t playground-php-wasm:libpng .
docker build -f ./libpng16/Dockerfile -t playground-php-wasm:libpng .
docker cp $$(docker create playground-php-wasm:libpng):/root/install/lib ./libpng16/dist/root/lib
docker cp $$(docker create playground-php-wasm:libpng):/root/install/include ./libpng16/dist/root/lib

Expand Down Expand Up @@ -88,7 +91,8 @@ oniguruma/dist/root/lib/lib/libonig.a: base-image
docker cp $$(docker create playground-php-wasm:oniguruma):/root/lib/lib ./oniguruma/dist/root/lib/
docker cp $$(docker create playground-php-wasm:oniguruma):/root/lib/include ./oniguruma/dist/root/lib

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

clean:
rm -rf ./libz/dist
rm -rf ./libzip/dist
Expand Down
5 changes: 4 additions & 1 deletion packages/php-wasm/compile/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'], {
cwd: sourceDir,
stdio: 'inherit',
});

await asyncSpawn(
'docker',
Expand Down
Loading