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

dynamic_modules: scaffolds Rust SDK #35914

Merged

Conversation

mathetake
Copy link
Member

@mathetake mathetake commented Aug 29, 2024

Commit Message: dynamic_modules: scaffolds Rust SDK
Additional Description:
This scaffolds the Rust SDK of dynamic modules. The same set of
unit tests is passing for Rust SDK like raw C test programs.

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #35914 was opened by mathetake.

see: more, trace.

Signed-off-by: Takeshi Yoneda <[email protected]>

Currently, the ABI binding ([src/abi.rs](./src/abi.rs)) is manually generated by [`bindgen`](https://github.com/rust-lang/rust-bindgen) for the ABI header. Ideally, we should be able to do the bindgen in the build.rs file.

TODO(@mathetake): figure out how to properly setup the bindgen in build.rs with rules_rust. The most recommended way is to use [crate_universe](https://bazelbuild.github.io/rules_rust/crate_universe.html#setup) and use bindgen as a dev-dependency. However, it seems that crate_universe tries to use the underlying gcc system linker which we cannot assume always available.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: i tried so hard (like spent the entire two days) to get the crate_universe working in our envoy build settings but no luck. basically it is officially recommended way to deal with cargo dependencies but it seems not that compatible with our settings. So I had to choose to do the manual bindgen for now - which of course not ideal so I left this TODO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use crate universe here with bazel: https://github.com/bitdriftlabs/capture-sdk. Not sure if there is anything to be learned from that.

Copy link
Member Author

@mathetake mathetake Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I identified the root cause: rules_rust repo declared in Envoy is a bit old and patched version (used by the comptime option Wasmtime dependency and tests for Wasm extensions. They will never be able to use the vanilla rules_rust), hence crate_universe_dependencies requires bootstrap = True argument due to the reason descrbied in the crate_universe doc:

Note that if the current version of rules_rust is not a release artifact, you may need to set additional flags such as bootstrap = True on the crate_universe_dependencies call above or crates_repository::generator_urls in uses of crates_repository.

This results in rebuilding cargo-bazel which in turns tries to use the system gcc linker. Since we cannot assume (at least for the current build container and CI settings) gcc exists in the system and set in the PATH, that results in the build failure.

I think we have a few options here:

  1. Nukes Wasmtime and Proxy-Wasm Rust SDK dependency in Envoy Wasm sources/tests. This will allows us to use the latest vanilla rules_rust.
    • For Wasmtime, I believe nobody uses it because in anyway it's build time option. If ever there's a user, they can build Envoy by themselves. And for Proxy-Wasm Rust SDK, it's only used in tests, so easy to justify.
    • Even simply updating rules_rust is not impossible due to the usage of old APIs by Proxy-Wasm C++ host.
  2. Declare the another rules_rust with the non-patched latest version as rules_rust_2 or whatever different repo name.
  3. Install gcc in the build containers and CI env - but this will break the existing dev environment - we need to inform everyone to install gcc which is obviously not good. Also, this will break for Windows for sure.

what do you think? I think 2. is the easiest one but to be honest, that could be the beginning of a mess. If it's ok, I would go for 1., but not sure either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #35323 for discussion of the deps clusterfu

i think the solution is a fix in rules_rust (bazelbuild/rules_rust#2665)

cc @martijneken

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok cool, more complex than I expected - I think for now we should go with as-is while waiting for the @phlax's PR got merged. I will migrate here to use crate_universe once it's done

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well i got an idea

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok no luck - i will revert the commits trying to use crate_universe

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heads up - rules_rust is about to be updated

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake mathetake marked this pull request as ready for review August 29, 2024 22:44
@mathetake
Copy link
Member Author

cc @marc-barry

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with small nit.

/wait


Currently, the ABI binding ([src/abi.rs](./src/abi.rs)) is manually generated by [`bindgen`](https://github.com/rust-lang/rust-bindgen) for the ABI header. Ideally, we should be able to do the bindgen in the build.rs file.

TODO(@mathetake): figure out how to properly setup the bindgen in build.rs with rules_rust. The most recommended way is to use [crate_universe](https://bazelbuild.github.io/rules_rust/crate_universe.html#setup) and use bindgen as a dev-dependency. However, it seems that crate_universe tries to use the underlying gcc system linker which we cannot assume always available.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use crate universe here with bazel: https://github.com/bitdriftlabs/capture-sdk. Not sure if there is anything to be learned from that.

test/extensions/dynamic_modules/test_data/rust/no_op.rs Outdated Show resolved Hide resolved
@repokitteh-read-only repokitteh-read-only bot added deps Approval required for changes to Envoy's external dependencies and removed waiting labels Sep 3, 2024
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: #35914 was synchronize by mathetake.

see: more, trace.

@mathetake
Copy link
Member Author

/retest

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
LazyLock is stabilized in 1.80 (LazyOnce is in 1.79), but somehoe x64 CI fails with rust version upgrade

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
This reverts commit 6923b75.

Signed-off-by: Takeshi Yoneda <[email protected]>
This reverts commit fff449c.

Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
Copy link
Member Author

added a bunch of commits, but at the end of the day, resulted in only a few lines of change to use atomic: 21053c5...ef12250

@mattklein123 mattklein123 merged commit 08bf286 into envoyproxy:main Sep 4, 2024
39 checks passed
@mathetake mathetake deleted the dynamicmodulerustsdkscaffolds2 branch September 4, 2024 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants