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

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

Merged
merged 7 commits into from
Mar 12, 2024

Conversation

alexcrichton
Copy link
Member

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

rustbot commented Mar 5, 2024

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 rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 5, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 5, 2024

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.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Mar 5, 2024

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

oli-obk commented Mar 5, 2024

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent suggestion, extracted to #122108

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

please land the following on master as separate PRs:

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

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 6, 2024
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`.
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 6, 2024
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 added a commit to alexcrichton/rust that referenced this pull request Mar 6, 2024
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`.
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 6, 2024
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 added a commit to alexcrichton/rust that referenced this pull request Mar 6, 2024
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
Copy link
Member Author

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.

@rust-log-analyzer

This comment has been minimized.

.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());
Copy link
Member

Choose a reason for hiding this comment

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

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()?

return;
}

rustc().arg("foo.rs").arg("--target=wasm32-wasip1").arg("-O").run();
Copy link
Member

@jieyouxu jieyouxu Mar 6, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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) 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

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?

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 6, 2024
…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.
jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 11, 2024
…, 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
Copy link
Contributor

bors commented Mar 11, 2024

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

jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 11, 2024
…, 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`.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2024
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
Copy link
Member Author

@rustbot ready

@oli-obk
Copy link
Contributor

oli-obk commented Mar 11, 2024

Thanks for the PR and commit splits!

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

@bors
Copy link
Contributor

bors commented Mar 11, 2024

📌 Commit cf6d605 has been approved by oli-obk

It is now in the queue for this repository.

@bors 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
Copy link
Contributor

bors commented Mar 12, 2024

⌛ Testing commit cf6d605 with merge dc2ffa4...

@bors
Copy link
Contributor

bors commented Mar 12, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 12, 2024
@bors bors merged commit dc2ffa4 into rust-lang:master Mar 12, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 12, 2024
@alexcrichton alexcrichton deleted the test-wasm-with-wasi branch March 12, 2024 02:04
@rust-timer
Copy link
Collaborator

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%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants