Test wasm32-wasip1 in CI, not wasm32-unknown-unknown #122036

alexcrichton · 2024-03-05T17:47:54Z

This commit changes CI to no longer test the wasm32-unknown-unknown target and instead test the wasm32-wasip1 target. There was some discussion of this in a Zulip thread, and the motivations for this PR are:

Runtime failures on wasm32-unknown-unknown print nothing, meaning all you get is "something failed". In contrast wasm32-wasip1 can print to stdout/stderr.
The unknown-unknown target is missing lots of pieces of libstd, and while wasm32-wasip1 is also missing some pieces (e.g. threads) it's missing fewer pieces. This means that many more tests can be run.

Overall my hope is to improve the debuggability of wasm failures on CI and ideally be a bit less of a maintenance burden.

This commit specifically removes the testing of wasm32-unknown-unknown and replaces it with testing of wasm32-wasip1. Along the way there were a number of other archiectural changes made as well, including:

A new target.*.runtool option can now be specified in config.toml which is passed as --runtool to compiletest. This is used to reimplement execution of WebAssembly in a less-wasm-specific fashion.
The default value for runtool is an ambiently located WebAssembly runtime found on the system, if any. I've implemented logic for Wasmtime.
Existing testing support for wasm32-unknown-unknown and Emscripten has been removed. I'm not aware of Emscripten testing being run any time recently and otherwise wasm32-wasip1 is in theory the focus now.
I've added a new //@ needs-threads directive for compiletest and classified a bunch of wasm-ignored tests as needing threads. In theory these tests can run on wasm32-wasi-preview1-threads, for example.
I've tried to audit all existing tests that are either ignore-emscripten or ignore-wasm*. Many now run on wasm32-wasip1 due to being able to emit error messages, for example. Many are updated with comments as to why they can't run as well.
The compiletest output matching for wasm32-wasip1 automatically uses "match a subset" mode implemented in compiletest. This is because WebAssembly runtimes often add extra information on failure, such as the unreachable instruction in panic!, which isn't able to be matched against the golden output from native platforms.
I've ported most existing run-make tests that use custom Node.js wrapper scripts to the new run-make-based-in-Rust infrastructure. To do this I added wasmparser as a dependency of run-make-support for the various wasm tests to use that parse wasm files. The one test that executed WebAssembly now uses wasmtime-the-CLI to execute the test instead. I have not ported over an exception-handling test as Wasmtime doesn't implement this yet.
I've updated the test crate to print out timing information for WASI targets as it can do that (gets a previously ignored test now passing).
The test-various image now builds a WASI sysroot for the WASI target and additionally downloads a fixed release of Wasmtime, currently the latest one at 18.0.2, and uses that for testing.

rustbot · 2024-03-05T17:48:03Z

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-03-05T17:48:05Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

This PR modifies config.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

rustbot · 2024-03-05T17:53:29Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

This PR modifies config.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

oli-obk · 2024-03-05T17:58:10Z

please land the following on master as separate PRs:

I've added a new //@ needs-threads directive for compiletest and classified a bunch of wasm-ignored tests as needing threads. In theory these tests can run on wasm32-wasi-preview1-threads, for example.

A new target.*.runtool option can now be specified in config.toml which is passed as --runtool to compiletest. This is used to reimplement execution of WebAssembly in a less-wasm-specific fashion.

Existing testing support for [..] Emscripten has been removed. I'm not aware of Emscripten testing being run any time recently

bjorn3 · 2024-03-05T18:13:53Z

config.example.toml

+# This configuration is a space-separated list of arguments so `foo bar` would
+# execute the program `foo` with the first argument as `bar` and the second
+# argument as the test binary.
+#runtime = <none> (string)


Could you mirror the cargo naming (runner/test runner)? This options seems universally useful, not just for wasm.

Excellent suggestion, extracted to #122108

bjorn3 · 2024-03-05T18:15:50Z

tests/run-make/wasm-abi/rmake.rs

+        _ => false,
+    };
+    if !has_wasmtime {
+        println!("skipping test, wasmtime isn't available");


This will be entirely hidden when running ./x.py test. It will not even be marked as ignored test.

True! I don't know of a great way to manage that, but I suppose an alternative would be to require a RUNNER style env var and to consult that?

True! I don't know of a great way to manage that, but I suppose an alternative would be to require a RUNNER style env var and to consult that?

I'm a complete idiot and forgor that when implementing support for rmake.rs, compiletest directives are supported in rmake.rs! So this could be actually implemented in compiletest as a //@ needs-wasmtime header

alexcrichton · 2024-03-05T18:28:46Z

please land the following on master as separate PRs:

Can do! I'll do that over the next few days.

This commit adds a `runner` field configuration to `config.toml` for specifying a wrapper executable when executing binaries for a target. This is pulled out of rust-lang#122036 where a WebAssembly runtime is used, for example, to execute tests for `wasm32-wasip1`. The name "runner" here is chosen to match Cargo's `CARGO_*_RUNNER` configuration, and to make things a bit more consistent this additionally renames compiletest's `--runtool` argument to `--runner`.

This commit is extracted from rust-lang#122036 and adds a new directive to the `compiletest` test runner, `//@ needs-threads`. This is intended to capture the need that a target must implement threading to execute a specific test, typically one that uses `std::thread`. This is primarily done for WebAssembly targets which currently do not have threads by default. This enables transitioning a lot of `//@ ignore-wasm*`-style ignores into a more self-documenting `//@ needs-threads` directive. Additionally the `wasm32-wasi-preview1-threads` target, for example, does actually have threads, but isn't tested in CI at this time. This change enables running these tests for that target, but not other wasm targets.

This commit adds a `runner` field configuration to `config.toml` for specifying a wrapper executable when executing binaries for a target. This is pulled out of rust-lang#122036 where a WebAssembly runtime is used, for example, to execute tests for `wasm32-wasip1`. The name "runner" here is chosen to match Cargo's `CARGO_*_RUNNER` configuration, and to make things a bit more consistent this additionally renames compiletest's `--runtool` argument to `--runner`.

This commit is extracted from rust-lang#122036 and adds a new directive to the `compiletest` test runner, `//@ needs-threads`. This is intended to capture the need that a target must implement threading to execute a specific test, typically one that uses `std::thread`. This is primarily done for WebAssembly targets which currently do not have threads by default. This enables transitioning a lot of `//@ ignore-wasm*`-style ignores into a more self-documenting `//@ needs-threads` directive. Additionally the `wasm32-wasi-preview1-threads` target, for example, does actually have threads, but isn't tested in CI at this time. This change enables running these tests for that target, but not other wasm targets.

alexcrichton · 2024-03-06T20:49:18Z

I've split out #122108 and #122109 and then rebased this PR on top of them (that's the first two commits). I've split the remaining commit into smaller chunks as well with this commit being the one that removes some Emscripten-specific bits. I'd like to confirm, but would you like me to still separate out that to its own PR? Happy to do so, but the change would be quite small since the wasm32-unknown-unknown bits would need to stick around until this PR lands.

jieyouxu · 2024-03-06T21:22:24Z

src/tools/run-make-support/src/lib.rs

-        .arg(env::var("TMPDIR").unwrap())
-        .arg("-L")
-        .arg(env::var("TMPDIR").unwrap());
+    cmd.arg("--out-dir").arg(out_dir()).arg("-L").arg(env::var("TMPDIR").unwrap());


Suggested change

cmd.arg("--out-dir").arg(out_dir()).arg("-L").arg(env::var("TMPDIR").unwrap());

cmd.arg("--out-dir").arg(out_dir()).arg("-L").arg(out_dir());

Think this can also use out_dir()?

src/tools/run-make-support/src/lib.rs

jieyouxu · 2024-03-06T21:41:32Z

tests/run-make/wasm-custom-sections-opt/rmake.rs

+        return;
+    }
+
+    rustc().arg("foo.rs").arg("--target=wasm32-wasip1").arg("-O").run();


Could be a good idea to have some helper methods to be more semantically meaningful, like have an alias input()/input_file() that takes a Path instead of a plain &str. Might be overkill though.

EDIT: I meant input not output

Do you mean something like an output_file() on the return value from run()?

If so, that may be somewhat difficult as the precise output file name depends on the target being used, for example it'd have to parse --target passed here and realize that the output file extension is *.wasm.

Do you mean something like an output_file() on the return value from run()?

Sorry I meant for the input file, predicting the output file name is indeed a massive pain and shouldn't be done (just let the linker et al. do that) 😆

Ah ok I see, but at least in my experience you rarely want to just take &Path when it's common to take string literals, which typically leads to something like path: impl AsRef<Path>.

Either that or put another way the boilerplate of requiring Path::new("some literal") probably won't necessarily make it too much harder to shoot yourself in the foot with paths (vs just passing "some literal").

From above, though, could you elaborate on:

because some tools/executables properly normalize/convert a Unix-y input path so they work on Windows properly, but other tools just give up

AFAIK native Windows APIs, which anything going through Rust uses (e.g. the run_make_support crate), will never normalize anything. Instead Windows allows both / and \ as path separators which is why c:\a/b\c works. I believe that "verbatim paths" which IIRC are something like \\?\... (or something like that sorry I forget the specifics) require that the path separator is \, not /, but AFAIK that's pretty rare.

That's all to say, while I know that mingw/cygwin/etc can cause confusion with paths I've rarely run into / not working as a path separator except in the rare case of verbatim paths.

That's all to say, while I know that mingw/cygwin/etc can cause confusion with paths I've rarely run into / not working as a path separator except in the rare case of verbatim paths.

Yeah it's probably fine, I think I may have run into some strange path issues before.

jieyouxu

I left some very minor nitpicks for the run-make-support library changes, but otherwise the support library changes look good to me.

Ditto on supporting "ignored" test status in addition to pass/fail, but also haven't thought of a good way to handle that yet (since we probably want to support this for run-make tests in general). Unless we also replicate some of the compiletest directives in run-make...?

Also leaving a comment here as a remark (not limited/specific to this PR): for run-make tests in general, since they don't do error annotations and such, we can now run rustfmt on them now, right? Unless something is dependent on pretty-printing / specific formatting?

…reads, r=workingjubilee compiletest: Add a `//@ needs-threads` directive This commit is extracted from rust-lang#122036 and adds a new directive to the `compiletest` test runner, `//@ needs-threads`. This is intended to capture the need that a target must implement threading to execute a specific test, typically one that uses `std::thread`. This is primarily done for WebAssembly targets which currently do not have threads by default. This enables transitioning a lot of `//@ ignore-wasm*`-style ignores into a more self-documenting `//@ needs-threads` directive. Additionally the `wasm32-wasi-preview1-threads` target, for example, does actually have threads, but isn't tested in CI at this time. This change enables running these tests for that target, but not other wasm targets.

This commit adds a `runner` field configuration to `config.toml` for specifying a wrapper executable when executing binaries for a target. This is pulled out of rust-lang#122036 where a WebAssembly runtime is used, for example, to execute tests for `wasm32-wasip1`. The name "runner" here is chosen to match Cargo's `CARGO_*_RUNNER` configuration, and to make things a bit more consistent this additionally renames compiletest's `--runtool` argument to `--runner`.

…, r=WaffleLapkin Add `target.*.runner` configuration for targets This commit adds a `runner` field configuration to `config.toml` for specifying a wrapper executable when executing binaries for a target. This is pulled out of rust-lang#122036 where a WebAssembly runtime is used, for example, to execute tests for `wasm32-wasip1`. The name "runner" here is chosen to match Cargo's `CARGO_*_RUNNER` configuration, and to make things a bit more consistent this additionally renames compiletest's `--runtool` argument to `--runner`.

bors · 2024-03-11T07:30:24Z

☔ The latest upstream changes (presumably #122050) made this pull request unmergeable. Please resolve the merge conflicts.

…, r=WaffleLapkin Add `target.*.runner` configuration for targets This commit adds a `runner` field configuration to `config.toml` for specifying a wrapper executable when executing binaries for a target. This is pulled out of rust-lang#122036 where a WebAssembly runtime is used, for example, to execute tests for `wasm32-wasip1`. The name "runner" here is chosen to match Cargo's `CARGO_*_RUNNER` configuration, and to make things a bit more consistent this additionally renames compiletest's `--runtool` argument to `--runner`.

Rollup merge of rust-lang#122108 - alexcrichton:target-config-runtool, r=WaffleLapkin Add `target.*.runner` configuration for targets This commit adds a `runner` field configuration to `config.toml` for specifying a wrapper executable when executing binaries for a target. This is pulled out of rust-lang#122036 where a WebAssembly runtime is used, for example, to execute tests for `wasm32-wasip1`. The name "runner" here is chosen to match Cargo's `CARGO_*_RUNNER` configuration, and to make things a bit more consistent this additionally renames compiletest's `--runtool` argument to `--runner`.

This commit rewrites a number of `run-make` tests centered around wasm to instead use `rmake.rs` and additionally use the `wasm32-wasip1` target instead of `wasm32-unknown-unknown`. Testing no longer requires Node.js and additionally uses the `wasmparser` crate from crates.io to parse outputs and power assertions.

This commit updates the libtest conditionals to use `std::time::Instant` on WASI targets where it's implemented. Previously all wasm targets wouldn't use this type.

This commit removes the `wasm32-shim.js` file, for example, and deletes old support for Emscripten which hasn't been exercised in some time.

If one is not explicitly configured look in the system environment to try and find one. For now just probing for `wasmtime` is implemented.

Drop testing of `wasm32-unknown-unknown` and instead only test a WASI target which enables more debugging utilities such as printing.

This commit updates compiletest to automatically compare test output with subsets if a `--runner` argument is configured. Runners might inject extra information on failures, for example a WebAssembly runtime printing a wasm stack trace, which won't be in the output of a native runtime. The output with a `--runner` argument, however, should still have all the native output present.

* The WASI targets deal with the `main` symbol a bit differently than native so some `codegen` and `assembly` tests have been ignored. * All `ignore-emscripten` directives have been updated to `ignore-wasm32` to be more clear that all wasm targets are ignored and it's not just Emscripten. * Most `ignore-wasm32-bare` directives are now gone. * Some ignore directives for wasm were switched to `needs-unwind` instead. * Many `ignore-wasm32*` directives are removed as the tests work with WASI as opposed to `wasm32-unknown-unknown`.

alexcrichton · 2024-03-11T16:36:53Z

@rustbot ready

oli-obk · 2024-03-11T16:47:43Z

Thanks for the PR and commit splits!

@bors r+ rollup=never (let's not mix CI changes with other PRs)

bors · 2024-03-11T16:47:45Z

📌 Commit cf6d605 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2024-03-12T00:03:57Z

⌛ Testing commit cf6d605 with merge dc2ffa4...

bors · 2024-03-12T02:01:25Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing dc2ffa4 to master...

rust-timer · 2024-03-12T03:16:59Z

Finished benchmarking commit (dc2ffa4): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.7%	[2.7%, 2.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.2%	[-2.2%, -0.1%]	2
Improvements ✅ (secondary)	-3.6%	[-5.2%, -2.0%]	2
All ❌✅ (primary)	0.1%	[-2.2%, 2.7%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.7%, -2.4%]	2
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 671.842s -> 672.134s (0.04%)
Artifact size: 310.08 MiB -> 310.00 MiB (-0.03%)

rustbot assigned oli-obk Mar 5, 2024

This comment has been minimized.

Sign in to view

bjorn3 reviewed Mar 5, 2024

View reviewed changes

alexcrichton mentioned this pull request Mar 6, 2024

Add target.*.runner configuration for targets #122108

Merged

alexcrichton mentioned this pull request Mar 6, 2024

compiletest: Add a //@ needs-threads directive #122109

Merged

alexcrichton force-pushed the test-wasm-with-wasi branch from d7bdb30 to 284297b Compare March 6, 2024 20:47

This comment has been minimized.

Sign in to view

jieyouxu reviewed Mar 6, 2024

View reviewed changes

src/tools/run-make-support/src/lib.rs Show resolved Hide resolved

jieyouxu reviewed Mar 6, 2024

View reviewed changes

alexcrichton force-pushed the test-wasm-with-wasi branch from 6acca73 to aeb51f4 Compare March 11, 2024 16:33

alexcrichton added 7 commits March 11, 2024 09:36

libtest: Print timing information on WASI

8fcc009

This commit updates the libtest conditionals to use `std::time::Instant` on WASI targets where it's implemented. Previously all wasm targets wouldn't use this type.

Remove old support for emscripten/wasm32-u-u

7d9690a

This commit removes the `wasm32-shim.js` file, for example, and deletes old support for Emscripten which hasn't been exercised in some time.

Configure a default runner for WASI targets

341215c

If one is not explicitly configured look in the system environment to try and find one. For now just probing for `wasmtime` is implemented.

Update test-various docker image to test wasm32-wasip1

fc746c8

Drop testing of `wasm32-unknown-unknown` and instead only test a WASI target which enables more debugging utilities such as printing.

alexcrichton force-pushed the test-wasm-with-wasi branch from aeb51f4 to cf6d605 Compare March 11, 2024 16:36

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 12, 2024

bors merged commit dc2ffa4 into rust-lang:master Mar 12, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Mar 12, 2024

This was referenced Mar 12, 2024

Move alloc tests to rmake #121918

Open

Port exit-code run-make test to use rust #121884

Merged

alexcrichton deleted the test-wasm-with-wasi branch March 12, 2024 02:04

bors mentioned this pull request Mar 12, 2024

Support test output postprocessing by a child process. #122145

Closed

jieyouxu mentioned this pull request Jun 9, 2024

Tracking Issue for porting run-make tests to use Rust #121876

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test wasm32-wasip1 in CI, not wasm32-unknown-unknown #122036

Test wasm32-wasip1 in CI, not wasm32-unknown-unknown #122036

alexcrichton commented Mar 5, 2024

rustbot commented Mar 5, 2024

rustbot commented Mar 5, 2024

This comment has been minimized.

rustbot commented Mar 5, 2024

oli-obk commented Mar 5, 2024

bjorn3 Mar 5, 2024

alexcrichton Mar 6, 2024

bjorn3 Mar 5, 2024

alexcrichton Mar 6, 2024

jieyouxu Mar 27, 2024

alexcrichton commented Mar 5, 2024

alexcrichton commented Mar 6, 2024

This comment has been minimized.

jieyouxu Mar 6, 2024

jieyouxu Mar 6, 2024 •

edited

Loading

alexcrichton Mar 7, 2024

jieyouxu Mar 7, 2024

alexcrichton Mar 8, 2024

jieyouxu Mar 8, 2024

jieyouxu left a comment •

edited

Loading

bors commented Mar 11, 2024

alexcrichton commented Mar 11, 2024

oli-obk commented Mar 11, 2024

bors commented Mar 11, 2024

bors commented Mar 12, 2024

bors commented Mar 12, 2024

rust-timer commented Mar 12, 2024

	cmd.arg("--out-dir").arg(out_dir()).arg("-L").arg(env::var("TMPDIR").unwrap());
	cmd.arg("--out-dir").arg(out_dir()).arg("-L").arg(out_dir());

Test wasm32-wasip1 in CI, not wasm32-unknown-unknown #122036

Test wasm32-wasip1 in CI, not wasm32-unknown-unknown #122036

Conversation

alexcrichton commented Mar 5, 2024

rustbot commented Mar 5, 2024

rustbot commented Mar 5, 2024

This comment has been minimized.

rustbot commented Mar 5, 2024

oli-obk commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Mar 5, 2024

alexcrichton commented Mar 6, 2024

This comment has been minimized.

Choose a reason for hiding this comment

jieyouxu Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu left a comment • edited Loading

Choose a reason for hiding this comment

bors commented Mar 11, 2024

alexcrichton commented Mar 11, 2024

oli-obk commented Mar 11, 2024

bors commented Mar 11, 2024

bors commented Mar 12, 2024

bors commented Mar 12, 2024

rust-timer commented Mar 12, 2024

Overall result: no relevant changes - no action needed

jieyouxu Mar 6, 2024 •

edited

Loading

jieyouxu left a comment •

edited

Loading