Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions .github/workflows/compile-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,22 @@ jobs:
- name: Check out repository code
uses: actions/checkout@v3

- name: Install wasm32-wasip1 target for Rust
- name: Install nightly wasm32-wasip2 target for Rust
uses: actions-rust-lang/setup-rust-toolchain@v1
with:
toolchain: nightly
target: wasm32-wasip2

- name: Install stable wasm32-wasip1 target for Rust
uses: actions-rust-lang/setup-rust-toolchain@v1
with:
toolchain: stable
target: wasm32-wasip1

- name: Build tests
working-directory: tests/rust
run: |
./build.py
./build.py --toolchain=wasm32-wasip3:nightly

- name: Upload precompiled tests
if: matrix.os == 'ubuntu-latest'
Expand Down
25 changes: 21 additions & 4 deletions tests/rust/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,27 @@
help="print commands to be executed")
parser.add_argument("--release", action="store_true",
help="build tests in release mode")
parser.add_argument("--toolchain", action="append",
help="TARGET:TOOLCHAIN, pass +TOOLCHAIN to cargo for TARGET")

args = parser.parse_args()
if args.dry_run:
args.verbose = True

TOOLCHAINS={}
if args.toolchain is not None:
for toolchain_arg in args.toolchain:
match toolchain_arg.split(':'):
case (target, toolchain):
TOOLCHAINS[target] = toolchain
case arg:
print(f"expected --toolchain=TARGET:TOOLCHAIN, got {toolchain_arg}",
file=sys.stderr)
sys.exit(1)

CARGO = ['cargo']
SYSTEMS = ['wasm32']
VERSIONS = ['wasip1'] # + ['wasip2', 'wasip3']
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you mean wasip2? I see in the compile-tests.yml workflow you configure wasip2 target, not wasip3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's right as-is: the wasip3 interfaces and toolchain are still under development. There will be no wasm32-wasip3 toolchain until the release is blessed. Until then, one compiles with the wasm32-wasip2 toolchain. But they are indeed wasip3 tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is quite confusing and perhaps requires a bit of re-thinking how we define test cases overall.
Perhaps in the test suite metadata we should specify the minimum / maximum version of the WASI version that the test is compatible with, and then the test runner can select an appropriate set of tests to run based on parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed with regards to having a shortcut to run e.g. "just the wasip1 tests". But from a directory structure / test suite definition perspective, I don't know how to be more clear: the test sources are in tests/rust/wasm32-wasip3, and the binaries get written to tests/rust/testsuite/wasm32-wasip3. They test wasip3 APIs. That they are compiled with a toolchain named "wasip2" is a temporary detail that will go away once the wasip3 release is out.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wingo I agree that its clear when you lay it out that way - can you add text to the readme that describes how the tests for wasi are in tests//wasi, and a comment here in this build script that says wasip2 is an implementation detail until wasip3 ships in rustc?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious what's your long-term plan for adding more tests? Let's say we'll have wasip4 at some point of time, would they go to the wasip4 folder? But then what about tests in wasip3 - are they expected to be copied to wasip4, or is there assumption that all wasip(N-1) work with wasip(N)?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, there's no assumption that tests from wasip(N-1) work in wasip(N) - wasi revisions are where we are allowed to make breaking changes, whether because the prior interfaces were flawed or because new component model features allow us to change. So, when its time to add wasip4 (or wasi 1.0 or etc) we'd probably start by making a copy of the prior tests and then making whatever necessary changes according to what changed in the spec.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not fully convinced to the idea of having copies of tests for each version as I'm worried about the overhead this will cause whenever we have to update the tests. I'd be interested if there are any other reasonable alternatives. Having said that, I'm ok with unblocking the progress on wasip3 but really want us to re-think how those tests should be developed going forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I get the desire to not copy needlessly, I think here we have a need to do so.

  • The different WASI versions will have different WIT interfaces. Different APIs will need different tests.
  • Different runtimes will support different versions of WASI, and you might need different versions of the same engine to run different versions; e.g. I can imagine an engine wanting to drop support for wasip3 in 18 months and instead relying on 2to3-style migrators.
  • WASIp3 is churning and not out yet. WASI itself is churning and not 1.0. There is a necessary amount of change and we should just roll with it I think.

If we were testing something higher-level like libc, then I agree, there is no need to make per-version copies. But here we're testing low-level interfaces, so while I understand the distaste, I don't see another way to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not fully convinced to the idea of having copies of tests for each version as I'm worried about the overhead this will cause whenever we have to update the tests.

Our goal should be that, for older revisions of wasi, the tests are not updated except to clarify what the behavior of a certain revision should be. Whether a given clarification applies to more than one revision applies on a case by case basis.

@wingo has been working on two engineering activities in parallel, and I think we should distinguish between them. One effort, here, is to create new tests for wasip3. A second effort, is to clarify the spec of wasip1, in e.g. #118 #119 #124. Of those, only #119 has any applicability at all to wasip3, and when it does, it will be rendered with completely different source code because the wasip3 interfaces for expressing fdflags sync are distinct from wasip1. The other two PRs are for aspects of the wasip1 interfaces that we deliberately removed in wasip2, so there is no possible way to share code between those tests.

VERSIONS = ['wasip1', 'wasip3']

def compute_target(system, version):
return f"{system}-{version}"
Expand Down Expand Up @@ -74,10 +88,13 @@ def mkdir_p(path):
target = compute_target(system, version)
build_target = compute_build_target(system, version)
build_mode = "release" if args.release else "debug"
toolchain = [f"+{TOOLCHAINS[target]}"] if target in TOOLCHAINS else []

build_args = ["cargo", "build",
f"--manifest-path={BASE_DIR / target / 'Cargo.toml'}",
f"--target={build_target}"]
build_args = CARGO + toolchain + [
"build",
f"--manifest-path={BASE_DIR / target / 'Cargo.toml'}",
f"--target={build_target}"
]
if args.release:
build_args.append("--release")
run(build_args)
Expand Down
1 change: 1 addition & 0 deletions tests/rust/wasm32-wasip3/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
target/
Loading
Loading