Per-target rust testsuite #114
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wingo
force-pushed
the
rust-multi-target
branch
2 times, most recently
from
fc573cd to
3e885d1
Compare
As in WebAssembly#112, we reorganize the directory structure and change to a python build script, with the goal of making space for wasip3. Before, it was: - tests/rust/: Root of cargo package - tests/rust/src: Rust source files - tests/rust/testsuite: JSON test files, fs-tests.dir; built .wasm files dumped here Now it is: - tests/rust/wasm32-wasip1: Root of cargo package for wasip1 builds - tests/rust/wasm32-wasip1/src: Rust source, but also JSON and fs-tests.dir - tests/rust/testsuite/: Removed; instead it is a built dir - tests/rust/testsuite/wasm32-wasip1: All wasm and json files copied here
wingo
force-pushed
the
rust-multi-target
branch
from
3e885d1 to
e63e727
Compare
loganek
reviewed
Aug 22, 2025
| return f"{system}-{version}" | ||
|
|
||
| def compute_build_target(system, version): | ||
| if version == 'wasip3': |
There was a problem hiding this comment.
not sure why do we need this check? Looks like we never attempt to call this function for wasip3
loganek
reviewed
Aug 22, 2025
| def compute_build_target(system, version): | ||
| if version == 'wasip3': | ||
| # wasm32-wasip3 triple not yet supported. | ||
| return compute_target(system, 'wasip2') |
There was a problem hiding this comment.
if wasm32-wasip3 is not supported, then we should throw an error, instead of using another version. But as I stated in my previous comment, we could perhaps remove this if statement completely.
loganek
reviewed
Aug 22, 2025
Comment on lines
+12
to
+14
| parser.add_argument("--dry-run", action="store_true") | ||
| parser.add_argument("--verbose", action="store_true") | ||
| parser.add_argument("--release", action="store_true") |
There was a problem hiding this comment.
Consider adding help for each of those, not all the parameters are self-explanatory
loganek
reviewed
Aug 22, 2025
| return compute_target(system, 'wasip2') | ||
| return compute_target(system, version) | ||
|
|
||
| BASE_DIR = Path(__file__).parent |
There was a problem hiding this comment.
That could be a cli argument with the default value set to Path(__file__).parent for local executions.
loganek
reviewed
Aug 22, 2025
Comment on lines
+36
to
+38
| r = subprocess.run(argv) | ||
| if r.returncode != 0: | ||
| sys.exit(r.returncode) |
There was a problem hiding this comment.
This should have similar effect
Suggested change
| r = subprocess.run(argv) | |
| if r.returncode != 0: | |
| sys.exit(r.returncode) | |
| r = subprocess.run(argv, check=True) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As in #112, we reorganize the directory structure and change to a python build script, with the goal of making space for wasip3.
Before, it was:
tests/rust/: Root of cargo package
tests/rust/src: Rust source files
tests/rust/testsuite: JSON test files, fs-tests.dir; built .wasm files dumped here
Now it is:
tests/rust/wasm32-wasip1: Root of cargo package for wasip1 builds
tests/rust/wasm32-wasip1/src: Rust source, but also JSON and fs-tests.dir
tests/rust/testsuite/: Removed; instead it is a built dir
tests/rust/testsuite/wasm32-wasip1: All wasm and json files copied here
Note, the workflows are not yet updated.