Add a Rust based Wasm language module that supports the component model #1122
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ac000
force-pushed
the
wasm-wasi-component
branch
from
f35cf09
to
bfe7729
Compare
|
Make it clear that you can use either the 'proxy' or 'reactor' adaptor. |
Merged
ac000
force-pushed
the
wasm-wasi-component
branch
from
bfe7729
to
2b6a624
Compare
|
Move the re-enablement of the 'reactor' adaptor patch to before wiring the module up to the config system so the all the module changes are in before it's actually usable. |
hongzhidao
approved these changes
Feb 19, 2024
Right, I'll see if I can hack that in. But we still want the Dockerfile right? (Yes, it also needs some updating in terms of configuring/building/installing the module). |
ac000
force-pushed
the
wasm-wasi-component
branch
from
2b6a624
to
73cc33a
Compare
|
Updated dockerfile module build commands @tippexs You might want to check I didn't break anything... |
ac000
force-pushed
the
wasm-wasi-component
branch
from
73cc33a
to
f915288
Compare
|
Rebase with master for latest docker changes... (there's also a new commit, f915288 Docker: Genericise MODULE_PREBUILD_wasm for re-use, which may or may not last) |
|
callahad
reviewed
Feb 19, 2024
| @@ -76,7 +76,12 @@ cat << END | |||
| java OPTIONS configure Java module | |||
| run "./configure java --help" to see available options | |||
|
|
|||
| wasm OPTIONS configure WebAssembly module | |||
| run "./configure wasm --help" to see available options | |||
| wasm OPTIONS configure WebAssembly module | |||
There was a problem hiding this comment.
99% of our users will want wasm-wasi-component... I wonder if we can signpost this better?
Suggested change
| wasm OPTIONS configure WebAssembly module | |
| wasm OPTIONS configure raw (not Component Model) WebAssembly module |
Hm. I don't like that either. Food for thought if you have something better that immediately comes to mind.
Not blocking on this feedback.
There was a problem hiding this comment.
Users should be consuming it via packages... If they're building from source they hopefully have some idea of what they're doing and then there's documentation...
callahad
requested changes
Feb 19, 2024
auto/modules/wasm-wasi-component
Outdated
Comment on lines
83
to
91
| NXT_OS=$(uname -o) | ||
|
|
||
| if [ $NXT_OS = "Darwin" ]; then | ||
| NXT_CARGO_CMD="cargo rustc --release --manifest-path src/wasm-wasi-component/Cargo.toml -- --emit link=target/release/libnxt_wasmtime.so -C link-args='-undefined dynamic_lookup'" | ||
| else | ||
| NXT_CARGO_CMD="cargo build --release --manifest-path src/wasm-wasi-component/Cargo.toml" | ||
| fi | ||
|
|
||
|
|
There was a problem hiding this comment.
Let's handle -link-args in build.rs instead of auto/modules/wasm-wasi-component
I'll provide a patch tonight after I figure out if there's an easy way to duplicate the function of --emit during a normal cargo build
src/wasm-wasi-component/.gitignore
Outdated
| @@ -0,0 +1,3 @@ | |||
| Cargo.lock | |||
There was a problem hiding this comment.
I suspect we do want the Cargo.lock committed, to ensure we can deterministically build this module.
There was a problem hiding this comment.
I've also generally read that you should ignore Cargo.lock for libraries. E.g rust-lang/cargo#315 (comment)
|
On Mon, 19 Feb 2024 09:17:20 -0800 Dan Callahan ***@***.***> wrote:
+Cargo.lock
I suspect we _do_ want the `Cargo.lock` committed, to ensure we can deterministically build this module.
This thing constantly changes...
|
|
re: Cargo.lock, was there any other discussion about whether we should include it or not that I'm missing / contradicting? The general recommendation for the past decade has been "commit the lockfile for binaries, ignore it for libraries." Last year, the guidance changed to be more nuanced, but to default to tracking the lockfile even for libraries. The Cargo Book has a FAQ entry, "Why have Cargo.lock in version control?", which goes into more detail about the benefits of tracking the lockfile by default, but even under the old definition, I'd argue we should track the lockfile as our crate is not intended to be consumed by other crates; it's a leaf node producing a binary artifact. |
|
I'd say unless there's a very good reason to include the lock file, ignore it... so far I haven't seen ignoring it having ill effect. |
|
The guidance and FAQ linked above discuss reasons to include the lockfile, and inclusion has always been the default recommendation for binary crates. Omitting the lockfile is the exceptional behavior, and thus should have exceptional justification for why we're deviating from standard practices. |
|
If you don't want the faff, I'm happy to get us to merge these patches as-is (it's already a large series), and we can add the lockfile in a followup |
|
Same goes for my tweaks to |
Did you figure out how to adjust the target name from build.rs? as last I looked it didn't seem possible... |
Nope. Was thinking there might be a way to do it via a |
ac000
force-pushed
the
wasm-wasi-component
branch
from
23db1cb
to
2fe8861
Compare
|
Name the rust target more appropriately |
|
Thoughts on this proposed patch? Makes a plain diff --git a/auto/modules/wasm-wasi-component b/auto/modules/wasm-wasi-component
index 1ec5841c..e054401c 100644
--- a/auto/modules/wasm-wasi-component
+++ b/auto/modules/wasm-wasi-component
@@ -5,6 +5,11 @@
NXT_WCM_MODULE=wasm-wasi-component
NXT_WCM_MOD_NAME=`echo $NXT_WCM_MODULE | tr '-' '_'`.unit.so
+case $(uname -o) in
+ "Darwin") NXT_WCM_MOD_EXT="dylib" ;;
+ *) NXT_WCM_MOD_EXT="so" ;;
+esac
+
shift
@@ -80,15 +85,6 @@ fi
$echo " + $NXT_WCM_MODULE module: $NXT_WCM_MOD_NAME"
-NXT_OS=$(uname -o)
-
-if [ $NXT_OS = "Darwin" ]; then
- NXT_CARGO_CMD="cargo rustc --release --manifest-path src/wasm-wasi-component/Cargo.toml -- --emit link=target/release/libwasm_wasi_component.so -C link-args='-undefined dynamic_lookup'"
-else
- NXT_CARGO_CMD="cargo build --release --manifest-path src/wasm-wasi-component/Cargo.toml"
-fi
-
-
cat << END >> $NXT_MAKEFILE
.PHONY: ${NXT_WCM_MODULE}
@@ -101,13 +97,13 @@ ${NXT_WCM_MODULE}: $NXT_BUILD_DIR/lib/unit/modules/$NXT_WCM_MOD_NAME
$NXT_BUILD_DIR/lib/unit/modules/$NXT_WCM_MOD_NAME:
make build/src/nxt_unit.o
- $NXT_CARGO_CMD
+ cargo build --release --manifest-path src/wasm-wasi-component/Cargo.toml
install: ${NXT_WCM_MODULE}-install
${NXT_WCM_MODULE}-install: ${NXT_WCM_MODULE} install-check
install -d \$(DESTDIR)$NXT_MODULESDIR
- install -p src/wasm-wasi-component/target/release/libwasm_wasi_component.so \\
+ install -p src/wasm-wasi-component/target/release/libwasm_wasi_component.$NXT_WCM_MOD_EXT \\
\$(DESTDIR)$NXT_MODULESDIR/$NXT_WCM_MOD_NAME
uninstall: ${NXT_WCM_MODULE}-uninstall
diff --git a/src/wasm-wasi-component/build.rs b/src/wasm-wasi-component/build.rs
index 5ea74f17..49b904aa 100644
--- a/src/wasm-wasi-component/build.rs
+++ b/src/wasm-wasi-component/build.rs
@@ -5,6 +5,13 @@ fn main() {
// Tell cargo to invalidate the built crate whenever the wrapper changes
println!("cargo:rerun-if-changed=wrapper.h");
+ // Building fails on recent macOS due to changes in its linker.
+ // Passing `-undefined dynamic_lookup` works and matches prior behavior.
+ if cfg!(target_os = "macos") {
+ println!("cargo:rustc-link-arg=-undefined");
+ println!("cargo:rustc-link-arg=dynamic_lookup");
+ }
+
let bindings = bindgen::Builder::default()
.clang_args(["-I", "../"])
.clang_args(["-I", "../../build/include"]) |
|
Edit: The above patch has been edited to account for the rename away from libnxt_wasmtime |
|
Combine the wasm and wasm-wasi-component model docker targets |
|
Looks like the Dockerfile is still pointing at 1.32.0; could you please re-generate them with VERSION=1.31.1? |
|
That was on purpose Otherwise the thing will fail anyway... |
|
Ah, fair enough. We had discussed doing 1.31.1 upthread in #1122 (comment), but either way is fine since neither way will work until we release anyway :) I think we're good to merge, now? |
callahad
dismissed
thresheek’s stale review
Reached agreement in discussions above
|
One final run on BB... |
|
Let's get this sucker merged... |
ac000
force-pushed
the
wasm-wasi-component
branch
from
3243598
to
d88048d
Compare
|
Rebase with master |
This is the first commit in adding WebAssembly Component Model language module support. This just adds a new NXT_APP_WASM_WC type, required by subsequent commits. The WC stands for WASI_COMPONENT This new module will have a type of 'wasm-wasi-component'. Link: <nginx#1098> Signed-off-by: Andrew Clayton <[email protected]>
This is required to actually _build_ the 'wasm-wasi-componet' language module. The nxt_wasm_wc_app_conf_t structure consists of the component name, e.g my_component.wasm, this is required. It also consists of an object to store the directories that are allowed access to by the component, this is optional. The bulk of the configuration infrastructure will be added in a subsequent commit. Signed-off-by: Andrew Clayton <[email protected]>
This is the work of Alex Crichton. This is written in Rust. The problem is that there is currently no support on the C side of things for the component model, which is the point of this module. It talks to Unit via automatically generated bindings. I've (Andrew) just made some minor tweaks to src/lib.rs, build.rs & Cargo.toml to adjust some paths, adjust where we get the language module config from and the module name and where it's located in the source tree, I also removed and disabled the tracking of the Cargo.lock file, this is constantly changing and not tracking it seems right for 'libraries' and dropped the README's... Other than that I have tried to leave his work intact, subsequent commits will make some larger changes, but I didn't want to intermix them with Alex's work. One such commit will update the module to use wasmtime 17 which brings WASI 0.2.0 support. Signed-off-by: Andrew Clayton <[email protected]>
This is used by the rustfmt program to format Rust code according to the rules contained in this file. Currently we just set the line width limit to 80 characters to match our C code. Signed-off-by: Andrew Clayton <[email protected]>
Run from the repository root like $ rustfmt --edition 2021 src/wasm-wasi-component/src/lib.rs Also manually fix up some overly long comments. Signed-off-by: Andrew Clayton <[email protected]>
When Unit receives a request, if the body of that request is greater than a certain amount (16KiB by default) then it is written to a temporary file. When a language module goes to read the request body in such situations it will end up using read(2). The wasm-wasi-component language module was failing to properly read request bodies of around 2GiB or more. This is because (on Linux at least) read(2) (and other related system calls) will only read (or write) at most 0x7ffff000 (2,147,479,552) bytes, this is the case for both 32 and 64-bit systems. Regardless, it's probably not a good idea doing IO in such large chunks anyway. This patch changes the wasm-wasi-component language module to read the request buffer in 32MiB chunks (this matches the original 'wasm' language module). We are still limited to a 4GiB address space and can only upload files a little under 4GiB. Signed-off-by: Andrew Clayton <[email protected]>
This brings WASI 0.2.0 support. Link: <https://github.com/bytecodealliance/wasmtime/releases/tag/v17.0.0> Signed-off-by: Andrew Clayton <[email protected]>
With the initial port to wasmtime 17 we could no longer use the 'reactor' adaptor but had to switch to the more restrictive 'proxy' adaptor. This meant amongst other things (probably) we could no longer access the filesystem. Thanks to Joel Dice for pointing out the fix. With this we can go back to using the 'reactor' adaptor again and things are back to working as before. It's worth noting that you can use either the 'proxy' or 'reactor' adaptor depending on your requirements. Cc: Joel Dice <[email protected]> Signed-off-by: Andrew Clayton <[email protected]>
It seems we do want to track this thing. This is just the latest version that cargo had generated for me. Cc: Dan Callahan <[email protected]> Signed-off-by: Andrew Clayton <[email protected]>
This exposes the various WebAssembly Component Model language module
specific options.
The application type is "wasm-wasi-component".
There is a "component" option that is required, this specifies the full
path to the WebAssembly component to be run. This component should be in
binary format, i.e a .wasm file.
There is also currently one optional option
"access"
Due to the sandboxed nature of WebAssembly, by default Wasm
modules/components don't have any access to the underlying filesystem.
There is however a capabilities based mechanism[0] for allowing such
access.
This adds a config option to the 'wasm-wasi-component' application type
(same as for 'wasm');
'access.filesystem' which takes an array of
directory paths that are then made available to the wasm
module/component. This access works recursively, i.e everything under a
specific path is allowed access to.
Example config might look like
"applications": {
"my-wasm-component": {
"type": "wasm-wasi-component",
"component": "/path/to/component.wasm",
"access" {
"filesystem": [
"/tmp",
"/var/tmp"
]
}
}
}
The actual mechanism used allows directories to be mapped differently in
the guest. But at the moment we don't support that and just map say /tmp
to /tmp. This can be revisited if it's something users clamour for.
[0]: <https://github.com/bytecodealliance/wasmtime/blob/main/docs/WASI-capabilities.md>
Signed-off-by: Andrew Clayton <[email protected]>
The indentation uses spaces and not TABs. Signed-off-by: Andrew Clayton <[email protected]>
Et voila...
$ ./configure wasm-wasi-component
configuring wasm-wasi-component module
Looking for rust compiler ... found.
Looking for cargo ... found.
+ wasm-wasi-component module: wasm_wasi_component.unit.so
$ make install
test -d /opt/unit/sbin || install -d /opt/unit/sbin
install -p build/sbin/unitd /opt/unit/sbin/
test -d /opt/unit/state || install -d /opt/unit/state
test -d /opt/unit || install -d /opt/unit
test -d /opt/unit || install -d /opt/unit
test -d /opt/unit/share/man/man8 || install -d /opt/unit/sh
man/man8
install -p -m644 build/share/man/man8/unitd.8 /opt/unit/share/ma
n8/
make build/src/nxt_unit.o
make[1]: Entering directory '/home/andrew/src/unit'
make[1]: 'build/src/nxt_unit.o' is up to date.
make[1]: Leaving directory '/home/andrew/src/unit'
cargo build --release --manifest-path src/wasm-wasi-component/Cargo.toml
Finished release [optimized] target(s) in 0.55s
install -d /opt/unit/modules
install -p src/wasm-wasi-component/target/release/libwasm_wasi_component.so \
/opt/unit/modules/wasm_wasi_component.unit.so
Signed-off-by: Andrew Clayton <[email protected]>
The minimum version required to build wasmtime 17 which is required by wasm-wasi-component is 1.73.0 But no point not using the latest version. This also now needs the libclang-dev package installed, we install this via MODULE_PREBUILD_wasm. Signed-off-by: Andrew Clayton <[email protected]>
Thus $ make build-wasm will build _both_ the 'wasm' & 'wasm-wasi-component' modules. Signed-off-by: Andrew Clayton <[email protected]>
This now includes support for the 'wasm-wasi-component' module. This targets the upcoming 1.32.0 release which is required by wasm-wasi-component. However of course the 1.32.0 tag doesn't exist yet, so there will be a small window where this image won't build. Signed-off-by: Andrew Clayton <[email protected]>
ac000
force-pushed
the
wasm-wasi-component
branch
from
d88048d
to
4c55869
Compare
|
Rebase with master |
Thanks! This one felt of my radar! I will make sure we adding this to the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first thing that probably stands out here is: 'Rust'!
Indeed, this new language module is written in Rust, not ideal, but unfortunately there is currently no component model support on the runtime side of things in the C toolchain (you can write components themselves in C however...)
We need component model support to run WASI 0.2.0 components. Components are the way ahead.
This module doesn't replace the existing 'wasm' module and the two can happily co-exist. The exiting module currently has some advantages including the ability to transfer in and out files/data > ~4GiB.
This new language module was written by Alex Crichton, I have tried to keep his work intact and separate from extra changes to the module we make, such as updating to wasmtime 17.
The Rust code is limited purely to the language module and if you don't build it then there is no Rust involved.
The language module talks to the rest of Unit via automatically generated bindings at module build time.
This new module registers itself as a type of
wasm-wasi-componentas discussed here.