Skip to content
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(build) Add bazel rules to build ngx-wasm-rs and ngx-rust #450

Closed
wants to merge 3 commits into from

Conversation

curiositycasualty
Copy link

@curiositycasualty curiositycasualty commented Nov 27, 2023

"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:

  • wat feature (currently not working due to "missing CMAKE_ROOT")
  • readme
  • reusable workflow"

~ #392

This PR is based on a copy of the @fffonion fork branch that has been pushed to the @Kong repo, duplicating #392 while allowing for CI in this repo to run.

@curiositycasualty
Copy link
Author

Rebased on main.

@curiositycasualty curiositycasualty changed the title feat(build): Add bazel rules to build ngx-wasm-rs and ngx-rust feat(build) Add bazel rules to build ngx-wasm-rs and ngx-rust Nov 27, 2023
config Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 27, 2023

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

@@              Coverage Diff              @@
##                main        #450   +/-   ##
=============================================
  Coverage   90.16374%   90.16374%           
=============================================
  Files             46          46           
  Lines           8489        8489           
=============================================
  Hits            7654        7654           
  Misses           835         835           

@thibaultcha
Copy link
Member

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
Copy link

fffonion commented Nov 28, 2023

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
Copy link
Member

thibaultcha commented Nov 28, 2023

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
Copy link
Member

thibaultcha commented Nov 29, 2023

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
Copy link
Author

curiositycasualty commented Nov 29, 2023

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
Copy link
Member

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.

@thibaultcha thibaultcha deleted the bazel-cargo branch December 2, 2023 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants