eduardomourar edited PR #7167:
This will fix the error one would get while trying to run the wasmtime CLI with
-S http
because the async functions were being added to the linker. Additionally, we will allow shadowing in linker when using WASI HTTP in order to prevent the following error:import of
wasi:clocks/wall-clock
defined twiceAlso, to prevent it from breaking in the future, a simple handcrafted test has been added to the CLI test suite.
eduardomourar has marked PR #7167 as ready for review.
eduardomourar requested wasmtime-core-reviewers for a review on PR #7167.
eduardomourar requested alexcrichton for a review on PR #7167.
eduardomourar updated PR #7167.
eduardomourar updated PR #7167.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Thanks for adding this! This is unfortunately not really possible to change though and without the source used to generate this we can't feasibly change this over time. I think the best solution is to enable the
cli-tests
suite here to use programs from thetest-programs
crate. That's a larger refactoring though and probably best done in a separate PR, so it's ok to skip a test for this PR specifically and rely on local testing.
alexcrichton created PR review comment:
I think it would be best if
allow_shadowing
was disabled because that's otherwise easy to accidentally run into issues.There's unfortunately not a great solution for the error you're running into here, so in the meantime I think it would be best to expose individual
add_to_linker
functions per-interface and only invoke the wasi-http-related ones here.
eduardomourar submitted PR review.
eduardomourar created PR review comment:
i actually built the component out of the original core module
wasi-http
(which was handcrafted before in order to make it more maintainable).
eduardomourar submitted PR review.
eduardomourar created PR review comment:
i agree it will be very hard to keep it up-to-date while we are still iterating over WASI. maybe we can keep the original core module and build a component on the fly with wasm-tools?
eduardomourar submitted PR review.
eduardomourar created PR review comment:
alright, i can do that
eduardomourar updated PR #7167.
eduardomourar submitted PR review.
eduardomourar created PR review comment:
done. as it is only temporary, i exposed function only in sync module and kept out of docs.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Yes that makes sense but it's not clear to me, for example, how to regenerate this test. This means that if there's an update it would be quite difficult to update this. Even the core module will change because the wasi-http interfaces might change.
The "real fix" here is to use source code rather than compiled wat or wasm, but that would require further refactorings to provide the
test-programs
programs to the WASI CLI tests here. I think it's best to have no tests until that's done, and I don't think it would be too large of a refactoring so I can work on it after this PR lands.
eduardomourar submitted PR review.
eduardomourar created PR review comment:
there is no real source code because it was done by hand. it is not hard to make it, though. i just wanted to keep it very simple without a server-side in the cli as it takes longer to run.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Yes that's all fine, but again I would personally prefer not to check in a large
*.wat
file which we don't know how to regenerate and change. I don't doubt that you've generated this from source, but there's no instructions in-tree about what the generation process is or what the source is. I think it's best to leave this out, manually test this PR, and I'll follow up with a better testing strategy to re-land this.
eduardomourar updated PR #7167.
eduardomourar submitted PR review.
eduardomourar created PR review comment:
just added a test to
test-programs
calledoutbound_request_response_build
that does not require a server. i just don't know how to find the path to reference in the wasmtime cli tests file.
eduardomourar updated PR #7167.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Hm ok so in my opinion this PR can't land with this
*.wat
test. This is even if it's generated from the test you've added in-tree. This is local knowledge on this PR's comment which will be lost basically as soon as the PR lands, meaning that if you or I aren't the next person to touch this file, which is quite likely, then they'll not know what to do. To solve this dilemma one of a few things need to happen:
- This
*.wat
test should be deleted, temporarily. I'll send a follow-up PR to enable referencing wasms fromtest-programs
here in this test suite.- You should, in this PR, delete this
*.wat
and implement the previous bullet in this PR.- Block this PR on me doing the above refactor, then rebase on it and reference the
test-programs
-generated wasm file.Personally I prefer the first option because, as you pointed out, we'll want to backport this. In that sense I'd rather not block this PR. I'm happy to defer to you though on which route you'd prefer.
eduardomourar updated PR #7167.
eduardomourar submitted PR review.
eduardomourar created PR review comment:
in think option 1 is the best one too.
eduardomourar submitted PR review.
eduardomourar created PR review comment:
i have removed the wat file and ignored that particular test case. let me know and i can help re-enabling the test back when ready
eduardomourar edited PR #7167:
This will fix the error one would get while trying to run the wasmtime CLI with
-S http
because the async functions were being added to the linker. Additionally, we will expose only the necessary functions toallow shadowing inlinker when using WASI HTTP in order to prevent the following error:import of
wasi:clocks/wall-clock
defined twice
Also, to prevent it from breaking in the future, a simple handcrafted test has been added to the CLI test suite.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
For the future this isn't necessary as
println!
should work in guests. If it doesn't that's a bug we should document and fix.
eduardomourar submitted PR review.
eduardomourar created PR review comment:
If I use println, it gets translated to wasi preview1 but I wanted to exercise the preview2 functions
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Ah that's sort of right but also not quite accurate. It's correct that
println!
uses the Rust standard library which uses wasi preview1, but that's adapted to preview2 with the adapter, meaning thatprintln!
ends up calling the same preview2 function this one calls.
eduardomourar submitted PR review.
eduardomourar created PR review comment:
But not in the core module, right?
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Sorry I'm not exactly sure what you mean, but when using
println!
the core module produced by rustc will refer towasi_snapshot_preview1::fd_write
if that's what you mean. That core import is then hooked up to the adapter, and the adatper imports preview2 instead.
alexcrichton merged PR #7167.
alexcrichton created PR review comment:
To follow up here, https://github.com/bytecodealliance/wasmtime/pull/7182 is the refactoring I had in mind and this is the un-ignore of the test
alexcrichton submitted PR review.
Last updated: Dec 23 2024 at 12:05 UTC