feat(build) add bazel rules to build ngx-wasm-rs and ngx-rust #392
Conversation
|
@fffonion Are you prepared to be responsible for changes in the Bazel structure as development of the module itself continues? I don't feel very good about merging a whole new build system that we aren't comfortable maintaining, so someone has to be prepared to help us maintain it. Are you? |
| run: bazel build :ngx_rust :ngx_wasm_rs_static :ngx_wasm_rs_shared --verbose_failures | ||
|
|
||
| - name: Display artifacts | ||
| run: ls -lh bazel-bin/ |
There was a problem hiding this comment.
On second thought, we will need this to be a reusable workflow that we can invoke from ci.yml and ci-large.yml. Please follow the example with job-unit-tests.yml (or any of the job-*.yml reusable workflows we already have).
There was a problem hiding this comment.
I will leave this as a TODO for this PR once I finish the bazel part of it.
|
@thibaultcha This PR only intended to build the rust part of this project, but not all of the Nginx build system. And it won't replace the default cargo based build when using outside of Kong (CI, vanilla nginx). So it would need minimal maintainance once merged, the most part of work will be at Kong side. I'm happy to setup knowledge sessions with wasm team folks on this : ) |
Pull Request Test Coverage Report for Build 5947539505
💛 - Coveralls |
| @@ -102,7 +102,7 @@ if [ "$ngx_wasm_cargo_lib_name" = ngx_wasm_rs ]; then | |||
| fi | |||
| fi | |||
|
|
|||
| if echo -n "$ngx_wasm_runtime_libs\n$NGX_LD_OPT" | grep -q "ngx-wasm-rs"; then | |||
There was a problem hiding this comment.
This greps for the path ngx-wasm-rs as it is specified as a path, not a library name, you will have to find another way.
There was a problem hiding this comment.
The path name doesn't necessarily contain ngx-wasm-rs (for example the rust rule automatically export the file in lib under build dependency https://github.com/Kong/kong-ee/pull/6346/files#diff-5c8ca1cd9c76393e97326f7857dd2be6cd741552bab8e4413dbcad5b652a94ccR29). Well, it's probably not reliable to either test the path or the module name, as it may fasly match some other part in NGX_LD_OPT; how do you feel?
There was a problem hiding this comment.
The path name doesn't necessarily contain
ngx-wasm-rs
It does when using the local development environment and when using ./configure with a source release of this module, hence why keeping it that way. For Bazel there will have to be another way of detecting this. Checking for both ngx-wasm-rs and ngx_wasm_rs is good enough too if it works.
|
Latest remnants of CI flakiness were fixed in |
|
Hmmm the PR comes from your fork, it seems we still have to adjust for that use-case. I hope #400 should fix it. |
|
Thank you @thibaultcha ! I will make another rebase. I may continue to work on this PR on the TODO items shortly. |
|
Hmmm that last failure is what I was afraid of, that the ghcr.io tag would also fetch upstream instead of just finding it locally... So that means we have to use a different tag when the |
|
@thibaultcha Is there anything I can setup on my fork to fix it? I understand secrets of GHA for fork is such a headache. |
|
@fffonion Give it another rebase since the last update I put in there yesterday, it should work as I tested it, short of something specific I didn't foresee. |
fffonion
force-pushed
the
bazel-cargo
branch
from
e879a30 to
4873378
Compare
| @@ -8,6 +8,10 @@ edition = "2018" | |||
| ngx-wasm-wat = { path = "lib/wat", optional = true } | |||
| ngx-wasm-backtrace = { path = "lib/backtrace" } | |||
|
|
|||
| # from backtrace | |||
| wasmparser = "0.102" | |||
| rustc-demangle = "0.1" | |||
There was a problem hiding this comment.
Why are these dependencies duplicated here? Cargo resolves hierarchical dependencies nicely, no need to specify this twice.
There was a problem hiding this comment.
These are the workaround for bazel's rust rule, it doesn't use cargo cli directly and doesn't resolve hierachical dependecies like cargo did 😢 .
| @@ -102,7 +102,7 @@ if [ "$ngx_wasm_cargo_lib_name" = ngx_wasm_rs ]; then | |||
| fi | |||
| fi | |||
|
|
|||
| if echo -n "$ngx_wasm_runtime_libs\n$NGX_LD_OPT" | grep -q "ngx-wasm-rs"; then | |||
| if echo -n "$ngx_wasm_runtime_libs\n$NGX_LD_OPT" | grep -q "ngx_wasm_rs"; then | |||
There was a problem hiding this comment.
| if echo -n "$ngx_wasm_runtime_libs\n$NGX_LD_OPT" | grep -q "ngx_wasm_rs"; then | |
| if echo -n "$ngx_wasm_runtime_libs\n$NGX_LD_OPT" | grep -E -q "ngx_wasm_rs|ngx-wasm-rs"; then |
This PR adds Bazel build rules to compile ngx-wasm-rs and ngx-rust that can be imported and used by Kong,
as directly calling
cargomight break cross-compilation.The
rules_rustdoesn't seem to parse the local redirection of depenendencies in Cargo.toml well so thatI have to manually add those dependencies. And it seems the secondary dependencies will also needs to
be in the top level Cargo.toml to make build happy. Let me know how do you feel about this workaround.
What's missing at present: