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

Refactor test-programs to build modules and components #6385

Merged
merged 27 commits into from
May 17, 2023
Merged

Conversation

pchickey
Copy link
Contributor

@pchickey pchickey commented May 16, 2023

This is task 2 in #6370

On the tails of #6374, this adds support for test-programs to build components out of preview 1 modules.

But thats really just the side story for this PR, which rewrites the entire way that test-programs works. Over in preview2-prototyping, I got tired of doing a bunch of code generation of all the #[test] functions, which makes it tests to read and hard to modify.

So, I trimmed code generation in test-programs/build.rs down to the bare minimum: it calls out to cargo to build the test program crates, and then we have a function module_rs() which writes a source file {test-crate}_modules.rs into OUT_DIR. This source file defines a function get_module(&str) -> Module which gives the user the module corresponding to the binary name.

For test program crates which can be built into components, you can invoke component_rs() which creates {test-crate}_components.rs in OUT_DIR, and you use get_component(&str) -> wasmtime::component::Component. One extra argument is required: a built adapter, which is the command build of wasi-preview1-component-adapter from this tree.

As part of this rewrite, we generate the components for the test crates, but we don't actually execute them yet, because we need a host implementation of preview 2 to do so. Sit tight - that is coming in the very next PR!

@pchickey pchickey force-pushed the pch/test_programs branch from 336e465 to cbd5c0f Compare May 16, 2023 03:45
@pchickey pchickey force-pushed the pch/test_programs branch from f44643f to 41e99f7 Compare May 16, 2023 19:31
@pchickey pchickey marked this pull request as ready for review May 16, 2023 19:31
@pchickey pchickey requested review from a team as code owners May 16, 2023 19:31
@pchickey pchickey requested review from jameysharp and removed request for a team May 16, 2023 19:31
@pchickey
Copy link
Contributor Author

I am applying a temporary exception to deny.toml for duplicate dependencies on wasm-encoder, wasmparser, and wit-parser. The wasmtime project versions have fallen behind the latest wit-bindgen, and @itsrainy is going to update those shortly with their core dumps work.

I would typically just wait for that sort of work to finish up to land my PR, but I want to move along quickly so we can change the canonical home of the preview 2 work to this repository, which will unblock several other people.

@pchickey pchickey requested review from alexcrichton and removed request for jameysharp May 16, 2023 19:33
@pchickey pchickey force-pushed the pch/test_programs branch from 41e99f7 to 4745cab Compare May 16, 2023 19:37
crates/test-programs/Cargo.toml Outdated Show resolved Hide resolved

[dev-dependencies]
anyhow = { workspace = true }
tempfile = "3.1.0"
test-log = { version = "0.2", default-features = false, features = ["trace"] }
Copy link
Member

Choose a reason for hiding this comment

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

Oh? I've not seen this dependency before, what does it do?

Copy link
Contributor Author

@pchickey pchickey May 16, 2023

Choose a reason for hiding this comment

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

It is provides a wrapper over #[test] (or e.g. #[tokio::test] if you pass it in as an argument) which takes care of installing tracing-subscriber with an env filter before starting your test. It gets rid of a Once cell we used to do that manually.

crates/test-programs/wasi-http-tests/Cargo.toml Outdated Show resolved Hide resolved
version = "0.3.16"
version = "0.3.19"
Copy link
Member

Choose a reason for hiding this comment

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

Could this diff be audited to avoid adding a new exemption? (assuming that this diff is a relatively small one hopefully)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could, I just figured that if we have an exemption I'd keep bumping it. I've been thinking about our exemptions of the tokio/hyper family (which this is a part of) is a wildcard audit for carl and sean.

Copy link
Member

Choose a reason for hiding this comment

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

Ok sounds reasonable 👍

@@ -641,6 +637,10 @@ criteria = "safe-to-deploy"
version = "0.2.13"
criteria = "safe-to-deploy"

[[exemptions.redox_syscall]]
version = "0.3.5"
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 check to see if 0.2.13, which is exempted, is a small or big diff from 0.3.5? If small mind adding an audit for that? If big though it's a bit of a bummer but we can probably live with an exemption.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to do any sort of audit on this because it is the redox syscall layer, an OS I don't know anything about and don't think we care about in the context of this project. It just happens to be a transitive dep in some crates.

Copy link
Member

Choose a reason for hiding this comment

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

Ok sounds reasonable 👍

crates/test-programs/tests/wasi-tokio.rs Show resolved Hide resolved
@pchickey pchickey requested a review from a team as a code owner May 16, 2023 20:09
@pchickey pchickey requested review from abrown and removed request for a team and abrown May 16, 2023 20:09
Alex asked me to do thsi for wit-component and wit-bindgen, and I found
a few more (cfg-if, tempfile, filecheck, anyhow...

I also reorganized the workspace dependencies section to make the ones
our team maintains more clearly separated from our external
dependencies.
@pchickey pchickey force-pushed the pch/test_programs branch from 7c4fe4a to 4c29127 Compare May 16, 2023 20:32
@pchickey pchickey enabled auto-merge May 16, 2023 20:32
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label May 16, 2023
@pchickey pchickey disabled auto-merge May 17, 2023 02:57
@pchickey
Copy link
Contributor Author

#6385 (comment) This change was backed out as part of merging with main after #6394 landed. Thank you @itsrainy.

Unfortunately, even with those fixes, there were still multiple versions of wasm-tools crates in the workspace, via the transitive dependencies of wit-bindgen. Alex and I decided that, since wit-bindgen is only used to build component tests, we will add a deny exception for its dependency tree.

@pchickey pchickey enabled auto-merge May 17, 2023 17:51
@pchickey pchickey added this pull request to the merge queue May 17, 2023
Merged via the queue into main with commit 338b535 May 17, 2023
@pchickey pchickey deleted the pch/test_programs branch May 17, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants