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
Merged

Fixing Build Regression [BISON] #892

Changes from 1 commit
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
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', 'base-image'], {
Copy link
Member

Choose a reason for hiding this comment

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

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?

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.

@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.)

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.

@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:

Copy link
Member

Choose a reason for hiding this comment

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

@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.

cwd: sourceDir,
stdio: 'inherit',
});

await asyncSpawn(
'docker',
Expand Down
Loading