nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.
https://unit.nginx.org
Apache License 2.0
5.26k stars 322 forks source link

Add a Rust based Wasm language module that supports the component model #1122

Closed ac000 closed 4 months ago

ac000 commented 4 months ago

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-component as discussed here.

ac000 commented 4 months ago

Make it clear that you can use either the 'proxy' or 'reactor' adaptor.

$ git range-diff f35cf09e...bfe77292
1:  f35cf09e ! 1:  bfe77292 Wasm-wc: Allow to use the 'reactor' adaptor again
    @@ Commit message
         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 <joel.dice@fermyon.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
ac000 commented 4 months ago

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.

$ git range-diff bfe77292...2b6a624c
1:  a3177ae3 = 1:  09dac2b7 Wasm-wc: Upgrade to wasmtime 17
7:  bfe77292 = 2:  6733a579 Wasm-wc: Allow to use the 'reactor' adaptor again
2:  3b3a82a1 = 3:  ea1efcef Wasm-wc: Wire up the language module to the config system
3:  ac1e19c0 = 4:  9b3edc23 Fix alignment of wasm options text in auto/help
4:  82f0c74e = 5:  7ad8ff3e Wasm-wc: Wire it up to the build system
5:  2619c91e = 6:  30bae7b2 Wasm-wc: Add sample wasmtime Dockerfile
6:  df7a5302 = 7:  2b6a624c Wasm-wc: Dockerfile changed to pull sources from the right repository
ac000 commented 4 months ago

Eh, I suck at github interface.

The Docker part needs to be implemented as a target in pkg/docker/Makefile, similarly to 5ed7dd5

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 commented 4 months ago

Updated dockerfile module build commands

$ git range-diff 2b6a624c...73cc33ad
1:  2b6a624c ! 1:  73cc33ad Wasm-wc: Dockerfile changed to pull sources from the right repository
    @@ pkg/docker/Dockerfile.wasm-wasi-component: RUN set -ex \
          && NCPU="$(getconf _NPROCESSORS_ONLN)" \
          && DEB_HOST_MULTIARCH="$(dpkg-architecture -q DEB_HOST_MULTIARCH)" \
     @@ pkg/docker/Dockerfile.wasm-wasi-component: RUN set -ex \
    +     && cargo --version \
          && rustc --version \
          && ./configure $CONFIGURE_ARGS_MODULES --cc-opt="$CC_OPT" --modulesdir=/usr/lib/unit/modules \
    -     && make build/src/nxt_unit.o \
    +-    && make build/src/nxt_unit.o \
     -    && cargo build --release --manifest-path wasmtime/Cargo.toml \
     -    && install -pm755 wasmtime/target/release/libnxt_wasmtime.so /usr/lib/unit/modules/wasmtime.unit.so \
     -    && rm -rf wasmtime/target \
    -+    && cargo build --release --manifest-path src/wasm-wasi-component/Cargo.toml \
    -+    && install -pm755 src/wasm-wasi-component/target/release/libnxt_wasmtime.so /usr/lib/unit/modules/wasm_wasi_component.unit.so \
    -+    && rm -rf src/wasm-wasi-component/target \
    ++    && ./configure wasm-wasi-component \
    ++    && make wasm-wasi-component-install \
    ++    && cd \
          && rm -rf /usr/src/unit \
          && for f in /usr/sbin/unitd /usr/lib/unit/modules/*.unit.so; do \
              ldd $f | awk '/=>/{print $(NF-1)}' | while read n; do dpkg-query -S $n; done | sed 's/^\([^:]\+\):.*$/\1/' | sort | uniq >> /requirements.apt; \

@tippexs You might want to check I didn't break anything...

ac000 commented 4 months ago

Rebase with master for latest docker changes... (there's also a new commit, f9152882 Docker: Genericise MODULE_PREBUILD_wasm for re-use, which may or may not last)

$ git range-diff 73cc33ad...f9152882
 -:  -------- >  1:  9e986704 Configuration: Fix validation of "processes"
 -:  -------- >  2:  3a2687bb Packages: added Ubuntu 23.10 "mantic" support.
 -:  -------- >  3:  8ebe04fd contrib: Bump libunit-wasm to 0.3.0.
 -:  -------- >  4:  ca1bc062 contrib: updated njs to 0.8.2.
 -:  -------- >  5:  bad2c181 Packages: Added Fedora 39 support.
 -:  -------- >  6:  661e918a Docker: added python3.12 to versions
 -:  -------- >  7:  2b0d93d1 Docker: Generated Dockerfile for Unit 1.31.1.
 -:  -------- >  8:  fbeb2065 fix: Take options as well as requestListener (#1091)
 -:  -------- >  9:  c3af21e9 Docker: Switch to github
 -:  -------- > 10:  baff936b Packages: Move dist target to git archive
 -:  -------- > 11:  34b3a812 Add nxt_file_chown()
 -:  -------- > 12:  b500c36d Allow to set the permissions of the Unix domain control socket
 -:  -------- > 13:  2bd3b418 Docs: Update man page for new --control-* options
 -:  -------- > 14:  1dca8602 Tools: disambiguate unitc control socket detection
 1:  3bb4526b = 15:  d3ef175d Wasm-wc: Register a new Wasm component model language module type
 2:  89df7881 = 16:  54ea7dcb Wasm-wc: Add core configuration data structure
 3:  445b5d79 = 17:  8388b4c1 Wasm-wc: Core of initial Wasm component model language module support
 4:  1ea2537b = 18:  2edb2d39 Add a .rustfmt.toml file
 5:  63fa352e = 19:  dc5d786b Wasm-wc: Run src/lib.rs through rustfmt
 6:  6bf04a3a = 20:  8bdaf3d8 Wasm-wc: Improve request buffer handling
 7:  09dac2b7 = 21:  6979a259 Wasm-wc: Upgrade to wasmtime 17
 8:  6733a579 = 22:  243a5900 Wasm-wc: Allow to use the 'reactor' adaptor again
 9:  ea1efcef = 23:  514c18f1 Wasm-wc: Wire up the language module to the config system
10:  9b3edc23 = 24:  29cf5d2e Fix alignment of wasm options text in auto/help
11:  7ad8ff3e = 25:  d73a5673 Wasm-wc: Wire it up to the build system
12:  30bae7b2 = 26:  bf42a05c Wasm-wc: Add sample wasmtime Dockerfile
13:  73cc33ad = 27:  ef369cf3 Wasm-wc: Dockerfile changed to pull sources from the right repository
 -:  -------- > 28:  f9152882 Docker: Genericise MODULE_PREBUILD_wasm for re-use
ac000 commented 4 months ago
$ git range-diff f9152882...23db1cb1
-:  -------- > 1:  94ced44e Docker: Add a wasm_wasi_component target
-:  -------- > 2:  23db1cb1 Docker: Rename Dockerfile.wasm-wasi-component
ac000 commented 4 months ago

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...

callahad commented 4 months ago

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.

ac000 commented 4 months ago

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.

callahad commented 4 months ago

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.

callahad commented 4 months ago

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

callahad commented 4 months ago

Same goes for my tweaks to build.rs and auto/modules/wasm-wasi-component to be honest.

ac000 commented 4 months ago

Same goes for my tweaks to build.rs and auto/modules/wasm-wasi-component to be honest.

Did you figure out how to adjust the target name from build.rs? as last I looked it didn't seem possible...

callahad commented 4 months ago

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 .cargo/config.toml at the crate root, but no dice. I suspect we should let cargo spit out a dylib and then copy it as a .so as part of the make install target.

ac000 commented 4 months ago

Name the rust target more appropriately

$ git range-diff 23db1cb1...2fe88618
 1:  8388b4c1 !  1:  0a162f6a Wasm-wc: Core of initial Wasm component model language module support
    @@ Commit message

         It talks to Unit via automatically generated bindings.

    -    I've (Andrew) just made some minor tweaks to src/lib.rs and build.rs 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'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
    @@ src/wasm-wasi-component/.gitignore (new)
      ## src/wasm-wasi-component/Cargo.toml (new) ##
     @@
     +[package]
    -+name = "nxt-wasmtime"
    ++name = "wasm-wasi-component"
     +version = "0.1.0"
     +edition = "2021"
     +publish = false
 2:  2edb2d39 =  2:  74f6b5dc Add a .rustfmt.toml file
 3:  dc5d786b =  3:  58c17212 Wasm-wc: Run src/lib.rs through rustfmt
 4:  8bdaf3d8 =  4:  39f834a4 Wasm-wc: Improve request buffer handling
 5:  6979a259 =  5:  4cca6cd1 Wasm-wc: Upgrade to wasmtime 17
 6:  243a5900 =  6:  7654d304 Wasm-wc: Allow to use the 'reactor' adaptor again
 7:  514c18f1 =  7:  670139c7 Wasm-wc: Wire up the language module to the config system
 8:  29cf5d2e =  8:  28ee6f9d Fix alignment of wasm options text in auto/help
 9:  d73a5673 !  9:  3c2a65de Wasm-wc: Wire it up to the build system
    @@ Commit message
           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/libnxt_wasmtime.so \
    +      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 <a.clayton@nginx.com>
    @@ auto/modules/wasm-wasi-component (new)
     +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'"
    ++  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
    @@ auto/modules/wasm-wasi-component (new)
     +
     +${NXT_WCM_MODULE}-install: ${NXT_WCM_MODULE} install-check
     +  install -d \$(DESTDIR)$NXT_MODULESDIR
    -+  install -p src/wasm-wasi-component/target/release/libnxt_wasmtime.so \\
    ++  install -p src/wasm-wasi-component/target/release/libwasm_wasi_component.so \\
     +          \$(DESTDIR)$NXT_MODULESDIR/$NXT_WCM_MOD_NAME
     +
     +uninstall: ${NXT_WCM_MODULE}-uninstall
10:  bf42a05c = 10:  6566e862 Wasm-wc: Add sample wasmtime Dockerfile
11:  ef369cf3 = 11:  459af1d8 Wasm-wc: Dockerfile changed to pull sources from the right repository
12:  f9152882 = 12:  f1640dc6 Docker: Genericise MODULE_PREBUILD_wasm for re-use
13:  94ced44e = 13:  cb44d455 Docker: Add a wasm_wasi_component target
14:  23db1cb1 = 14:  2fe88618 Docker: Rename Dockerfile.wasm-wasi-component
callahad commented 4 months ago

Thoughts on this proposed patch?

Makes a plain cargo build work on macOS, handles the dylib->so renaming in the wasm-wasi-component-install target.

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"])
callahad commented 4 months ago

...module names would have to be adjusted to take into account your latest revision of the patch series, which I'm only just now seeing for some reason.

Edit: The above patch has been edited to account for the rename away from libnxt_wasmtime

ac000 commented 4 months ago

The thing I'm not so keen on is that we would have the macOS/other difference in two different places...

callahad commented 4 months ago

True, but without the change I cannot use normal tooling (cargo build / test / bench / etc) to develop and test the module. Seems like the appropriate separation of concerns: build.rs makes sure cargo can build the crate, autotools takes care of installing things into the correct destinations.

ac000 commented 4 months ago

On Tue, 20 Feb 2024 02:45:51 -0800 Dan Callahan @.***> wrote:

True, but without the change I cannot use normal tooling (cargo build / test / bench / etc) to develop and test the module. Seems like the appropriate separation of concerns: build.rs makes sure cargo can build the crate, autotools takes care of installing things into the correct destinations.

Our auto scripts are more than just about installing, they determine what gets built and how.

callahad commented 4 months ago

As far as I can tell, there are three remaining issues:

@ac000 How do you propose we move forward? Should we merge this series as-is and then tackle each of those independently, or would you prefer to hash everything out in this PR?

ac000 commented 4 months ago

As far as I can tell, there are three remaining issues:

* [ ]  Include `Cargo.lock` in Git for the wasm-wasi-component crate

I can add it now, if we understand what version we're adding and how to handle updates going forward.

* [ ]  Emit macOS linker options in `build.rs` for the wasm-wasi-component crate

I'd rather leave this for now.

* [ ]  Use `make` to generate the Dockerfile for the wasm-wasi-component module

Let's wait for @thresheek to review the latest docker changes.

ac000 commented 4 months ago
* [ ]  Include `Cargo.lock` in Git for the wasm-wasi-component crate

I can add it now, if we understand what version we're adding and how to handle updates going forward.

Although I'm not sure it matters what version we use as cargo will just overwrite it anyway...

callahad commented 4 months ago

Great. Let's commit the Cargo.lock, I'll open a separate issue to hash out the macOS linker question, and I'll take a closer look at the Docker changes.

I have a good grasp of how we're handling Docker; I'll take a look at that and see if we can get out ahead of @thresheek waking up for the day :)

ac000 commented 4 months ago

Add Cargo.lock

$ git range-diff 2fe88618...0a24f203
 -:  -------- >  1:  f00bb09a Wasm-wc: Add Cargo.lock
 1:  670139c7 =  2:  d30c86c1 Wasm-wc: Wire up the language module to the config system
 2:  28ee6f9d =  3:  0e5baa28 Fix alignment of wasm options text in auto/help
 3:  3c2a65de =  4:  f17da8ff Wasm-wc: Wire it up to the build system
 4:  6566e862 =  5:  a87a2cca Wasm-wc: Add sample wasmtime Dockerfile
 5:  459af1d8 =  6:  f809d2ef Wasm-wc: Dockerfile changed to pull sources from the right repository
 6:  f1640dc6 =  7:  90b83fad Docker: Genericise MODULE_PREBUILD_wasm for re-use
 7:  cb44d455 =  8:  0f94ba77 Docker: Add a wasm_wasi_component target
 8:  2fe88618 =  9:  0a24f203 Docker: Rename Dockerfile.wasm-wasi-component
ac000 commented 4 months ago

Obviously the git clone command needs updating for the --depth option and the correct repository/branch in the dockerfile...

callahad commented 4 months ago

I think what @thresheek is asking for is the ability to call:

cd pkg/docker/
rm Dockerfile.wasm_wasi_component
make Dockerfile.wasm_wasi_component

...and have git status report no changes.

ac000 commented 4 months ago

On Tue, 20 Feb 2024 08:21:08 -0800 Dan Callahan @.***> wrote:

What @thresheek is asking for is the ability to call:

cd pkg/docker/
rm Dockerfile.wasm_wasi_component
make Dockerfile.wasm_wasi_component

...and have git status report no changes.

There will (almost) always be a difference in the Unit version...

EDIT: Me fail English, that's unpossible!

ac000 commented 4 months ago

Though I see there other differences...

callahad commented 4 months ago

Looking more closely now, and $MODULE_PREBUILD_wasm_common is gnarly; I wouldn't trust myself to edit it and get all the escaping correct. Can we move it into its own shell script?

ac000 commented 4 months ago

Rebase with master and drop the commits related to the dockerfile a new commit will add it.

$ git range-diff 0a24f203...10ea18a7
 -:  -------- >  1:  756feafd Node.js: Use console.warn instead of stderr.write
 -:  -------- >  2:  ea239635 Docker: Switch python3.12 to using github
 -:  -------- >  3:  0c983530 Node.js: Build/install fix
 -:  -------- >  4:  30b410e4 Avoid a segfault in nxt_conn_io_sendbuf()
 -:  -------- >  5:  62894ae7 Var: Refactored nxt_var_ref_get()
 -:  -------- >  6:  01fd121c Var: Refactored nxt_http_unknown_var_ref()
 -:  -------- >  7:  63507c49 Var: Make nxt_var_cache_value() more general
 -:  -------- >  8:  46554015 Var: Introduced nxt_var_get()
 -:  -------- >  9:  63ad4deb NJS: Simplified nxt_js_call()
 -:  -------- > 10:  33c6c4d4 NJS: variable access support
 -:  -------- > 11:  183a1e9d Docker: redirect logs to stderr
 -:  -------- > 12:  5570d807 Packages: fixed a path to python 3.12 example app
 -:  -------- > 13:  53648ed5 Tools: Fix typo in tools/README.md
 -:  -------- > 14:  d24ae5a9 Add additional replace rules for node:* modules
 -:  -------- > 15:  bd0abdf0 Docker: Shallow clone the Unit repo
 -:  -------- > 16:  914cd4e3 .mailmap: Map some more personal addresses
 -:  -------- > 17:  d52a9361 Docker: Update versions of Go, Node, PHP, Ruby
 -:  -------- > 18:  2765522b Tests: NJS request variables
 -:  -------- > 19:  cca2c46e Tools: setup-unit: Use trap(1) to handle cleanup
 -:  -------- > 20:  d6ed0003 Tools: setup-unit: De-duplicate code
 -:  -------- > 21:  e9a0c49d Tools: setup-unit: Pass --fail-with-body to curl(1)
 -:  -------- > 22:  565a8ed0 Tools: setup-unit: ctl edit: Print file name on error
 -:  -------- > 23:  bc093ab3 Tools: setup-unit: Fix error message
 -:  -------- > 24:  6aa5ef63 Tools: setup-unit: ctl edit: Append suffix to tmp file name
 1:  d3ef175d = 25:  a37c87b6 Wasm-wc: Register a new Wasm component model language module type
 2:  54ea7dcb = 26:  6fe77b02 Wasm-wc: Add core configuration data structure
 3:  0a162f6a = 27:  82b394c4 Wasm-wc: Core of initial Wasm component model language module support
 4:  74f6b5dc = 28:  3ad3c9b4 Add a .rustfmt.toml file
 5:  58c17212 = 29:  a3bc4e6b Wasm-wc: Run src/lib.rs through rustfmt
 6:  39f834a4 = 30:  d9df0dba Wasm-wc: Improve request buffer handling
 7:  4cca6cd1 = 31:  eaf91c2e Wasm-wc: Upgrade to wasmtime 17
 8:  7654d304 = 32:  b1d37a93 Wasm-wc: Allow to use the 'reactor' adaptor again
 9:  f00bb09a = 33:  043e6b4f Wasm-wc: Add Cargo.lock
10:  d30c86c1 = 34:  ee6534fb Wasm-wc: Wire up the language module to the config system
11:  0e5baa28 = 35:  601b5042 Fix alignment of wasm options text in auto/help
12:  f17da8ff = 36:  05b4cd53 Wasm-wc: Wire it up to the build system
13:  a87a2cca <  -:  -------- Wasm-wc: Add sample wasmtime Dockerfile
14:  f809d2ef <  -:  -------- Wasm-wc: Dockerfile changed to pull sources from the right repository
15:  90b83fad = 37:  af71bd99 Docker: Genericise MODULE_PREBUILD_wasm for re-use
16:  0f94ba77 = 38:  10ea18a7 Docker: Add a wasm_wasi_component target
17:  0a24f203 <  -:  -------- Docker: Rename Dockerfile.wasm-wasi-component
ac000 commented 4 months ago

Add a pre-generated wasm_wasi_component dockerfile

$ git range-diff 10ea18a7...939121db
-:  -------- > 1:  939121db Docker: Add a pre-generated wasm_wasi_component dockerfile
callahad commented 4 months ago

Disregard comments about $MODULE_PREBUILD_wasm_common ; I'm tired and missed that this series didn't introduce that, so any changes I'd want to propose around that are a separate issue.

callahad commented 4 months ago

Docker: The common prebuild compiles wasmtime 11 from pkg/contrib, but we don't use it for this module, do we?

callahad commented 4 months ago

Also, building the docker image fails:

error: package `wasmtime v17.0.0` cannot be built because it requires rustc 1.73.0 or newer, while the currently active rustc version is 1.71.0
callahad commented 4 months ago

Building with RUST_VERSION=1.76.0... waiting for completion.

ac000 commented 4 months ago

Don't build and install the wasmtime C library for wasm-wasi-component

$ git range-diff 939121db...1227c0e7
-:  -------- > 1:  de388d48 Docker: Split building wasmtime out into a separate stage
1:  939121db ! 2:  1227c0e7 Docker: Add a pre-generated wasm_wasi_component dockerfile
    @@ pkg/docker/Dockerfile.wasm_wasi_component (new)
     +    && rustup --version \
     +    && cargo --version \
     +    && rustc --version \
    -+    && make -C pkg/contrib .wasmtime \
    -+    && install -pm 755 pkg/contrib/wasmtime/target/release/libwasmtime.so /usr/lib/$(dpkg-architecture -q DEB_HOST_MULTIARCH)/ \
     +    && ./configure $CONFIGURE_ARGS_MODULES --cc-opt="$CC_OPT" --modulesdir=/usr/lib/unit/debug-modules --debug \
     +    && ./configure wasm-wasi-component \
     +    && make -j $NCPU wasm-wasi-component-install \

Verified this produces same output as before

For Dockerfile.wasm

$ diff -u Dockerfile.wasm.bak Dockerfile.wasm
--- Dockerfile.wasm.bak 2024-02-20 18:39:38.030127093 +0000
+++ Dockerfile.wasm     2024-02-20 19:03:42.885981895 +0000
@@ -6,7 +6,7 @@
 LABEL org.opencontainers.image.source="https://github.com/nginx/unit"
 LABEL org.opencontainers.image.documentation="https://unit.nginx.org/installation/#docker-images"
 LABEL org.opencontainers.image.vendor="NGINX Docker Maintainers <docker-maint@nginx.com>"
-LABEL org.opencontainers.image.version="1.31.1"
+LABEL org.opencontainers.image.version="1.32.0"

 RUN set -ex \
     && savedAptMark="$(apt-mark showmanual)" \
@@ -15,7 +15,7 @@
     && mkdir -p /usr/lib/unit/modules /usr/lib/unit/debug-modules \
     && mkdir -p /usr/src/unit \
     && cd /usr/src/unit \
-    && git clone --depth 1 -b 1.31.1-1 https://github.com/nginx/unit \
+    && git clone --depth 1 -b 1.32.0-1 https://github.com/nginx/unit \
     && cd unit \
     && NCPU="$(getconf _NPROCESSORS_ONLN)" \
     && DEB_HOST_MULTIARCH="$(dpkg-architecture -q DEB_HOST_MULTIARCH)" \

The only difference is the Unit version.

For Dockerfile.wasm_wasi_component

$ diff -u Dockerfile.wasm_wasi_component.bak Dockerfile.wasm_wasi_component
--- Dockerfile.wasm_wasi_component.bak  2024-02-20 18:39:52.327218450 +0000
+++ Dockerfile.wasm_wasi_component      2024-02-20 19:04:29.074429279 +0000
@@ -64,8 +64,6 @@
     && rustup --version \
     && cargo --version \
     && rustc --version \
-    && make -C pkg/contrib .wasmtime \
-    && install -pm 755 pkg/contrib/wasmtime/target/release/libwasmtime.so /usr/lib/$(dpkg-architecture -q DEB_HOST_MULTIARCH)/ \
     && ./configure $CONFIGURE_ARGS_MODULES --cc-opt="$CC_OPT" --modulesdir=/usr/lib/unit/debug-modules --debug \
     && ./configure wasm-wasi-component \
     && make -j $NCPU wasm-wasi-component-install \

The only difference is we no longer build and install the wasmtime C library.

ac000 commented 4 months ago

I was going with 1.73.0, but I guess no point not using the latest version...

callahad commented 4 months ago

Heh, the Linux kernel already has a patch series targeting Rust 1.77. We're falling behind!

Builds now fail because libclang-dev isn't present

Unable to find libclang: "couldn't find any valid shared libraries matching: ['libclang.so', 'libclang-*.so', 'libclang.so.*', 'libclang-*.so.*'], set the `LIBCLANG_PATH` environment variable to a path where one of these files can be found (invalid: [])"
ac000 commented 4 months ago

Actually 1.76.0 is what's currently in Fedora, so I know that works...

ac000 commented 4 months ago

Bump rust to 1.76.0

$ git range-diff 1227c0e7...09584a04
-:  -------- > 1:  43671755 Docker: Bump rust version to 1.76.0
1:  1227c0e7 = 2:  09584a04 Docker: Add a pre-generated wasm_wasi_component dockerfile
callahad commented 4 months ago

Here's a dumb patch that I'm waiting to finish building: https://gist.github.com/callahad/65ddbf3405da791cf682c4dc3f4fc2a1

callahad commented 4 months ago

Patch in the gist works

ac000 commented 4 months ago

That patch is quite invasive. My alternative would be to simply install that package in MODULE_PREBUILD_wasm_common and live with the fact that it's not technically required for the wasm module, unless it might be at some point because wasmtime 17 and not some other reason... which it probably is... but maybe not...

callahad commented 4 months ago

I'm happy enough with adding it to the prebuild; go for it.

ac000 commented 4 months ago

Or is it for compiling the newer versions of rust it's needed? I don't have that equivalent package installed on Fedora.

thresheek commented 4 months ago

Yep, MODULEPREBUILD* is a correct place to preinstall the dependency.

ac000 commented 4 months ago
$ git range-diff 09584a04...b18f0bec
1:  43671755 ! 1:  906a12d4 Docker: Bump rust version to 1.76.0
    @@ Commit message

         But no point not using the latest version.

    +    This also now needs the libclang-dev package installed, we install this
    +    via MODULE_PREBUILD_wasm_common.
    +
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

      ## pkg/docker/Makefile ##
    @@ pkg/docker/Makefile: RUN_wasm_wasi_component ?=          /bin/true

      define MODULE_PREBUILD_wasm_common
     -export RUST_VERSION=1.71.0 \\\n \
    -+export RUST_VERSION=1.76.0 \\\n \
    ++apt-get install --no-install-recommends --no-install-suggests -y libclang-dev \\\n \
    ++\ \ \ \&\& export RUST_VERSION=1.76.0 \\\n \
      \ \ \ \&\& export RUSTUP_HOME=/usr/src/unit/rustup \\\n \
      \ \ \ \&\& export CARGO_HOME=/usr/src/unit/cargo \\\n    \
      \ \ \ \&\& export PATH=/usr/src/unit/cargo/bin:\$$PATH \\\n \
2:  09584a04 ! 2:  acc1f9c3 Docker: Add a pre-generated wasm_wasi_component dockerfile
    @@ pkg/docker/Dockerfile.wasm_wasi_component (new)
     +    && make -j $NCPU unitd \
     +    && install -pm755 build/sbin/unitd /usr/sbin/unitd \
     +    && make clean \
    -+    && export RUST_VERSION=1.71.0 \
    ++    && apt-get install --no-install-recommends --no-install-suggests -y libclang-dev \
    ++    && export RUST_VERSION=1.76.0 \
     +    && export RUSTUP_HOME=/usr/src/unit/rustup \
     +    && export CARGO_HOME=/usr/src/unit/cargo \
     +    && export PATH=/usr/src/unit/cargo/bin:$PATH \
-:  -------- > 3:  b18f0bec Docker: Update Dockerfile.wasm for rust 1.76.0
callahad commented 4 months ago

Without libclang-dev, the error occurs when cargo build attempts to run our custom build.rs. Looks like it's required for bindgen. Full output:

error: failed to run custom build command for `wasm-wasi-component v0.1.0 (/usr/src/unit/unit/src/wasm-wasi-component)`

Caused by:
  process didn't exit successfully: `/usr/src/unit/unit/src/wasm-wasi-component/target/release/build/wasm-wasi-component-970591f24f7a8dbb/build-script-build` (exit status: 101)
  --- stdout
  cargo:rerun-if-changed=wrapper.h
  cargo:rerun-if-env-changed=TARGET
  cargo:rerun-if-env-changed=BINDGEN_EXTRA_CLANG_ARGS_aarch64-unknown-linux-gnu
  cargo:rerun-if-env-changed=BINDGEN_EXTRA_CLANG_ARGS_aarch64_unknown_linux_gnu
  cargo:rerun-if-env-changed=BINDGEN_EXTRA_CLANG_ARGS

  --- stderr
  thread 'main' panicked at /usr/src/unit/cargo/registry/src/index.crates.io-6f17d22bba15001f/bindgen-0.68.1/lib.rs:611:31:
  Unable to find libclang: "couldn't find any valid shared libraries matching: ['libclang.so', 'libclang-*.so', 'libclang.so.*', 'libclang-*.so.*'], set the `LIBCLANG_PATH` environment variable to a path where one of these files can be found (invalid: [])"
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
make: *** [build/Makefile:1945: build/lib/unit/modules/wasm_wasi_component.unit.so] Error 101
Error: building at STEP "RUN set -ex     && savedAptMark="$(apt-mark showmanual)"     && apt-get update     && apt-get install --no-install-recommends --no-install-suggests -y ca-certificates git build-essential libssl-dev libpcre2-dev curl pkg-config     && mkdir -p /usr/lib/unit/modules /usr/lib/unit/debug-modules     && mkdir -p /usr/src/unit     && cd /usr/src/unit     && git clone --depth 1 -b wasm-wasi-component https://github.com/ac000/unit     && cd unit     && NCPU="$(getconf _NPROCESSORS_ONLN)"     && DEB_HOST_MULTIARCH="$(dpkg-architecture -q DEB_HOST_MULTIARCH)"     && CC_OPT="$(DEB_BUILD_MAINT_OPTIONS="hardening=+all,-pie" DEB_CFLAGS_MAINT_APPEND="-Wp,-D_FORTIFY_SOURCE=2 -fPIC" dpkg-buildflags --get CFLAGS)"     && LD_OPT="$(DEB_BUILD_MAINT_OPTIONS="hardening=+all,-pie" DEB_LDFLAGS_MAINT_APPEND="-Wl,--as-needed -pie" dpkg-buildflags --get LDFLAGS)"     && CONFIGURE_ARGS_MODULES="--prefix=/usr                 --statedir=/var/lib/unit                 --control=unix:/var/run/control.unit.sock                 --runstatedir=/var/run                 --pid=/var/run/unit.pid                 --logdir=/var/log                 --log=/var/log/unit.log                 --tmpdir=/var/tmp                 --user=unit                 --group=unit                 --openssl                 --libdir=/usr/lib/$DEB_HOST_MULTIARCH"     && CONFIGURE_ARGS="$CONFIGURE_ARGS_MODULES                 --njs"     && make -j $NCPU -C pkg/contrib .njs     && export PKG_CONFIG_PATH=$(pwd)/pkg/contrib/njs/build     && ./configure $CONFIGURE_ARGS --cc-opt="$CC_OPT" --ld-opt="$LD_OPT" --modulesdir=/usr/lib/unit/debug-modules --debug     && make -j $NCPU unitd     && install -pm755 build/sbin/unitd /usr/sbin/unitd-debug     && make clean     && ./configure $CONFIGURE_ARGS --cc-opt="$CC_OPT" --ld-opt="$LD_OPT" --modulesdir=/usr/lib/unit/modules     && make -j $NCPU unitd     && install -pm755 build/sbin/unitd /usr/sbin/unitd     && make clean     && export RUST_VERSION=1.76.0     && export RUSTUP_HOME=/usr/src/unit/rustup     && export CARGO_HOME=/usr/src/unit/cargo     && export PATH=/usr/src/unit/cargo/bin:$PATH     && dpkgArch="$(dpkg --print-architecture)"     && case "${dpkgArch##*-}" in        amd64) rustArch="x86_64-unknown-linux-gnu"; rustupSha256="0b2f6c8f85a3d02fde2efc0ced4657869d73fccfce59defb4e8d29233116e6db" ;;        arm64) rustArch="aarch64-unknown-linux-gnu"; rustupSha256="673e336c81c65e6b16dcdede33f4cc9ed0f08bde1dbe7a935f113605292dc800" ;;        *) echo >&2 "unsupported architecture: ${dpkgArch}"; exit 1 ;;     esac     && url="https://static.rust-lang.org/rustup/archive/1.26.0/${rustArch}/rustup-init"     && curl -L -O "$url"     && echo "${rustupSha256} *rustup-init" | sha256sum -c -     && chmod +x rustup-init     && ./rustup-init -y --no-modify-path --profile minimal --default-toolchain $RUST_VERSION --default-host ${rustArch}     && rm rustup-init     && rustup --version     && cargo --version     && rustc --version     && ./configure $CONFIGURE_ARGS_MODULES --cc-opt="$CC_OPT" --modulesdir=/usr/lib/unit/debug-modules --debug     && ./configure wasm-wasi-component     && make -j $NCPU wasm-wasi-component-install     && make clean     && ./configure $CONFIGURE_ARGS_MODULES --cc-opt="$CC_OPT" --modulesdir=/usr/lib/unit/modules     && ./configure wasm-wasi-component     && make -j $NCPU wasm-wasi-component-install     && cd     && rm -rf /usr/src/unit     && for f in /usr/sbin/unitd /usr/lib/unit/modules/*.unit.so; do         ldd $f | awk '/=>/{print $(NF-1)}' | while read n; do dpkg-query -S $n; done | sed 's/^\([^:]\+\):.*$/\1/' | sort | uniq >> /requirements.apt;        done     && apt-mark showmanual | xargs apt-mark auto > /dev/null     && { [ -z "$savedAptMark" ] || apt-mark manual $savedAptMark; }     && /bin/true     && mkdir -p /var/lib/unit/     && mkdir -p /docker-entrypoint.d/     && groupadd --gid 999 unit     && useradd          --uid 999          --gid unit          --no-create-home          --home /nonexistent          --comment "unit user"          --shell /bin/false          unit     && apt-get update     && apt-get --no-install-recommends --no-install-suggests -y install curl $(cat /requirements.apt)     && apt-get purge -y --auto-remove build-essential     && rm -rf /var/lib/apt/lists/*     && rm -f /requirements.apt     && ln -sf /dev/stderr /var/log/unit.log": while running runtime: exit status 2

make: *** [Makefile:155: build-wasm_wasi_component] Error 2

Edit: Yep, from the bindgen docs

bindgen leverages libclang to preprocess, parse, and type check C and C++ header files. It is required to use Clang 5.0 or greater.

ac000 commented 4 months ago

Hmm, I guess clang-libs on Fedora contains enough without needing the -devel package...