Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 COMPILE] #871
Fixing build regression [BISON COMPILE] #871
Changes from all commits
3a6e9d3
fafa9e6
eebb534
2f00b16
cfeca7a
080d9b7
be40eae
2651f82
7bc8d83
fec45de
09c6329
03ee0b0
a4c3ad0
e202c14
69388f7
8371941
6744e0c
a53d842
cefc9fa
ae2d9f4
0c4fa2e
1ef26cc
18b1884
677e85a
bf645ec
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 bashif
could just plug-in the correct version depending on the PHP_VERSION.How much shorter is it, though? Are we talking about minutes or seconds?
Oh, interesting, I haven't thought of it like that. What would be an example of such a dependency?