-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
chore(ci): add wasm to release builds #11039
Conversation
5162537
to
9bd5881
Compare
1c3f5fd
to
a6317f4
Compare
a6317f4
to
cbb59e6
Compare
@@ -197,6 +234,12 @@ CONFIGURE_OPTIONS = [ | |||
}) + select({ | |||
"@kong//:wasmx_flag": [ | |||
"--add-module=$$EXT_BUILD_ROOT$$/external/ngx_wasm_module", | |||
# this is a workaround for rhel/centos builds |
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.
FYI: @curiositycasualty @fffonion
I don't know enough to judge the level of risk/impact of this change. If necessary I'm sure it could be limited to just centos/rhel-based builds, but I opted to start with the simple approach for 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, just a few nitpicks
Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com>
Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com>
Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com>
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. The build is green and everything seems OK. I am not pressing the merge button right now because the build is failing on my local environment (unrelated to the *dylib
change). I am trying to find out if it's my environment that is broken or not.
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.
Still looking for the root-cause, but @gszr confirmed this change breaks the build on macos.
the commented line breaks the build for macos, probably the flag is not supported by clang
* chore(ci): add wasm to release builds * chore(*): apply review suggestions Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com> * chore(*): apply review suggestions Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com> * chore(*): apply review suggestions Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com> * chore(build): change the v8 runtime URL * chore(build): workaround for macos the commented line breaks the build for macos, probably the flag is not supported by clang --------- Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com>
* chore(ci): add wasm to release builds * chore(*): apply review suggestions Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com> * chore(*): apply review suggestions Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com> * chore(*): apply review suggestions Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com> * chore(build): change the v8 runtime URL * chore(build): workaround for macos the commented line breaks the build for macos, probably the flag is not supported by clang --------- Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com>
* chore(ci): add wasm to release builds * chore(*): apply review suggestions Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com> * chore(*): apply review suggestions Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com> * chore(*): apply review suggestions Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com> * chore(build): change the v8 runtime URL * chore(build): workaround for macos the commented line breaks the build for macos, probably the flag is not supported by clang --------- Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com>
This turns on
ngx_wasm_module
by default, adding it to all of our amd64 builds in the release workflow.Right now we only build and package with
wasmtime
, because that is what is planned to ship, but there is partial support for wasmer and v8 in here as well, should we decide to make them available in the future.Additionally, with this change the wasm runtime is now statically linked to
ngx_wasm_module
, so there are fewer shared object files that need to be copied around into our common lib directory at build time.