-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: add common build environment #14
Conversation
cdb90a0
to
6cec5f0
Compare
f98b874
to
8ac25fb
Compare
1387121
to
d4a80a5
Compare
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.
The PR looks amazing @ereslibre! This simplifies the build a lot and it ensure the builds are consistent across different environments 👏 .
Added a few minor comments before approving it. Thanks!
2. You can find `php-cgi` in `$WASMLABS_BUILD_OUTPUT/bin/php-cgi` | ||
|
||
# Running a script with php-cgi | ||
## Running a script with php-cgi |
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.
Even though this PR simplifies build a lot, I would still keep some information in the README file about the commands to build PHP. Even it's a simple make php-X
. In that way, I don't need to check all the files in the folder, look for a "build" system, notice the Makefile
file and read it before knowing the command.
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.
Yeah, you are right. I was a bit dubious about adding it because it's easy to forget updating the README when we update the versions.
However I think we can add automation to fix that problem. I have some ideas we can discuss :)
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 that fine now?
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.
LGTM! 👏
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.
That's a great step forward!
|
||
.PHONY: wasi-builder-16 | ||
wasi-builder-16: | ||
docker build --build-arg WASI_SDK_VERSION=16 -f ${BUILDER_ROOT_DIR}/Dockerfile.wasi-builder -t ghcr.io/vmware-labs/wasi-builder:16 ${BUILDER_ROOT_DIR} |
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.
Shouldn't this be tagged in projects.registry.vmware.com/wasmlabs/containers instead of ghcr.io ?
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.
I realized with this work that we can publish on ghcr.io already (https://github.com/orgs/vmware-labs/packages/container/package/wasi-builder) and (https://github.com/orgs/vmware-labs/packages/container/package/php-builder).
I think we might want to discuss it. If we are going to work with GitHub workflows and automate stuff on GitHub I think it might be more convenient to deploy assets on the GitHub registry, given it makes authentication & authorization easier and more integrated.
66cd689
to
6b56531
Compare
Add a build environment based on container images that can reproduce the build in multiple environments. Signed-off-by: Rafael Fernández López <[email protected]>
6b56531
to
92a8c55
Compare
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.
LGTM! Amazing automation jod 😄
Fixes: #6
Fixes: #10
Add a build environment based on container images that can reproduce the build in multiple environments. To be completed: