Kong / ngx_wasm_module

Nginx + WebAssembly
Apache License 2.0
80 stars 7 forks source link

feat(build) Add bazel rules to build ngx-wasm-rs and ngx-rust #450

Closed curiositycasualty closed 9 months ago

curiositycasualty commented 9 months ago

"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 cargo might break cross-compilation.

The rules_rust doesn't seem to parse the local redirection of depenendencies in Cargo.toml well so that I 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:

~ https://github.com/Kong/ngx_wasm_module/pull/392

This PR is based on a copy of the @fffonion fork branch that has been pushed to the @Kong repo, duplicating https://github.com/Kong/ngx_wasm_module/pull/392 while allowing for CI in this repo to run.

curiositycasualty commented 9 months ago

Rebased on main.

codecov[bot] commented 9 months ago

Codecov Report

Merging #450 (3b6ce1f) into main (db88b15) will not change coverage. Report is 1 commits behind head on main. The diff coverage is n/a.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/450/graphs/tree.svg?width=650&height=150&src=pr&token=T0PT2Q9IAN&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong)](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) ```diff @@ Coverage Diff @@ ## main #450 +/- ## ============================================= Coverage 90.16374% 90.16374% ============================================= Files 46 46 Lines 8489 8489 ============================================= Hits 7654 7654 Misses 835 835 ```
thibaultcha commented 9 months ago

Hey thanks. We were wondering recently if we should let this go or push for merging it. We landed on "if Gateway won't use it we would rather not maintain it"; I too am curious what @fffonion thinks (I saw the earlier Slack thread).

fffonion commented 9 months ago

That will lead to the question of if we are going to ship the two other runtimes (wasmer and v8) that need either ngx_wasm_rs or ngx_rust, other than the existing wasmtime, into kong or kong-ee. Which from a dicussion from my previous PR a while ago, I assume the answer is yes.

If the plan changes and that we aren't planning to ship the two other runtimes with kong in near future, we can hold this progress for now. But if we do, I would suggest a go with this PR. On this repo, we will only ensure the two rust dependencies are build-able from bazel; there's no need to switch the whole CI of ngx-wasm-module to build upon bazel. Additional functional tests will then need to be added on gateway side to cover the two other runtimes.

thibaultcha commented 9 months ago

Hmm ok. We are probably not shipping any other runtime anytime soon, but maybe we could still have this. Heads up though there will be quite some work before this can be merged based on this current state.

there's no need to switch the whole CI of ngx-wasm-module to build upon bazel.

Oh well yeah that has always been totally out of the question anyway. If we add bazel it's just for fun (CONTRIBUTING.md note, like a moonshot if another Nginx embedder wishes to use the module through bazel) or for a specific Gateway purpose that has some maintainability benefit (considering we probably will only ever ship with Wasmtime).

thibaultcha commented 9 months ago

Since it seems we only need this because of how the Gateway wishes to build our Rust libs, and given how invasive it is (a ton of files we don't care for on a day-to-day basis), could we move it to a repository of its own instead? We can keep the CI in this repository, and the bazel rules in a new one like Kong/ngx_wasm_module_bazel or something...

curiositycasualty commented 9 months ago

could we move it to a repository of its own instead?

If we come back around on needing this functionality, than that can be the approach we take. Closing per KAG-2682.

thibaultcha commented 9 months ago

FYI I'll be deleting this branch and storing it in another fork of ngx_wasm_module; if you or @fffonion wish to keep a copy of the branch as well, make sure to keep a fork around.