Make wasi-common self-contained, deprecate exports from wasmtime-wasi#7881
Merged
Make wasi-common self-contained, deprecate exports from wasmtime-wasi#7881
Conversation
not a very modular design, but at this point wasi-common and wasi-threads are forever wed
and remove libc dep because rustix provides the same constant
since this is the sole place in the codebase its used
pchickey
requested review from
alexcrichton,
fitzgen and
sunfishcode
and removed request for
a team and
fitzgen
github-actions
bot
added
wasi
Subscribe to Label Actioncc @peterhuene DetailsThis issue or pull request has been labeled: "wasi", "wasmtime:c-api"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
sunfishcode
approved these changes
Feb 9, 2024
Member
sunfishcode
left a comment
sunfishcode
left a comment
There was a problem hiding this comment.
This looks great! Much simpler.
Could you add a high-level summary to the top of wasi-common's top-level doc comment in src/lib.rs that mentions it contains the WASI 0.1 implementation?
Contributor
Author
|
Good reminder - done! |
…si => wasi_common
pchickey
force-pushed
the
pch/wasi-common-self-contained
branch
from
0cac87a to
9697614
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
pchickey
force-pushed
the
pch/wasi-common-self-contained
branch
from
61b2834 to
fd9f520
Compare
prtest:full
pchickey
force-pushed
the
pch/wasi-common-self-contained
branch
from
fd9f520 to
19ea4a1
Compare
added 6 commits
February 12, 2024 10:45
pchickey
pushed a commit
that referenced
this pull request
Feb 13, 2024
…#7881) * WIP: try to make wasi-common self contained. * rebase: cargo.lock * remove all dependencies between wasi-common and wasmtime-wasi * use wasi-common directly throughout tests, benches, examples, cli run * wasi-threads: use wasi-common's maybe_exit_on_error in spawned thread not a very modular design, but at this point wasi-common and wasi-threads are forever wed * fix wasmtime's docs * re-introduce wasmtime-wasi's exports of wasi-common definitions behind deprecated * factor out determining i32 process exit code and remove libc dep because rustix provides the same constant * commands/run: inline the logic about aborting on trap since this is the sole place in the codebase its used * Add high-level summary to wasi-common's top-level doc comment. * c-api: fix use of wasi_cap_std_sync => wasi_common::sync, wasmtime_wasi => wasi_common * fix tokio example * think better of combining downcast and masking into one method * fix references to wasmtime_wasi in docs prtest:full * benches: use wasi-common * cfg-if around use of rustix::process because that doesnt exist on windows * wasi-common: include tests, caught by verify-publish * fix another bench * exit requires wasmtime dep. caught by verify-publish.
pchickey
pushed a commit
that referenced
this pull request
Feb 13, 2024
…#7881) * WIP: try to make wasi-common self contained. * rebase: cargo.lock * remove all dependencies between wasi-common and wasmtime-wasi * use wasi-common directly throughout tests, benches, examples, cli run * wasi-threads: use wasi-common's maybe_exit_on_error in spawned thread not a very modular design, but at this point wasi-common and wasi-threads are forever wed * fix wasmtime's docs * re-introduce wasmtime-wasi's exports of wasi-common definitions behind deprecated * factor out determining i32 process exit code and remove libc dep because rustix provides the same constant * commands/run: inline the logic about aborting on trap since this is the sole place in the codebase its used * Add high-level summary to wasi-common's top-level doc comment. * c-api: fix use of wasi_cap_std_sync => wasi_common::sync, wasmtime_wasi => wasi_common * fix tokio example * think better of combining downcast and masking into one method * fix references to wasmtime_wasi in docs prtest:full * benches: use wasi-common * cfg-if around use of rustix::process because that doesnt exist on windows * wasi-common: include tests, caught by verify-publish * fix another bench * exit requires wasmtime dep. caught by verify-publish.
pchickey
pushed a commit
that referenced
this pull request
Feb 13, 2024
* Preview 2 filesystem: track open_mode instead of reporting the permissions (#7876) * fd_filestat_set test: assert that a file descriptor behaves the same... whether opened readonly or readwrite. This test now reproduces the behavior reported in #7829 Co-authored-by: Trevor Elliott <telliott@fastly.com> * preview1_path_link test: refactor; create and open file separately we create and open the file with separate descriptors because the creation descriptor will always imply writing, whereas the rest do not. there were a number of auxillary functions in here that were obscuring what was going on, and making errors harder to read, so i changed some assertions to use macro-rules so the panic message had the relevant line number, and i inlined the creation / opening functions. * preview2 filesystem: track open mode instead of reporting permissions it n DescriptorFlags FilePerms/DirPerms is a way of having wasmtime-wasi, instead of the operating system, mediate permissions on a preview2 Descriptor. This was conflated with the open mode, which should determine whether DescriptorFlags contains READ or WRITE or MUTATE_DIRECTORY. We no longer mask FilePerms with the DescriptorFlags (oflags) in open_at - instead, track what open mode is requested, and see if the file and directory permissions permit it. Additionally, File and Dir now track their open_mode (represented using OpenMode, which just like FilePerms is just a read and write bit), and report that in DescriptorFlags. Previously, we reported the FilePerms or DirPerms in the DescriptorFlags, which was totally wrong. Co-authored-by: Trevor Elliott <telliott@fastly.com> * different error on windows i guess? prtest:full * apparently set-times of read-only is rejected on windows. just skip testing that * path open read write: another alternate error code for windows * wasi-common actually gives a badf, so accept either * this case too --------- Co-authored-by: Trevor Elliott <telliott@fastly.com> * Return correct fs errors on the proxy adapter (#7926) This commit fixes an issue with the proxy adapter where if the proxy program attempted to look at prestat items, which wasi-libc always does on startup, it would invoke `fd_prestat_get` and receive `ERRNO_NOTSUP` in response (the standard response when using the `cfg_filesystem_available!` macro). This error code is unexpected by wasi-libc and causes wasi-libc to abort. The PR here is to instead return `ERRNO_BADF` which is indeed expected by wasi-libc and is recognized as meaning "that prestat isn't available". * Make wasi-common self-contained, deprecate exports from wasmtime-wasi (#7881) * WIP: try to make wasi-common self contained. * rebase: cargo.lock * remove all dependencies between wasi-common and wasmtime-wasi * use wasi-common directly throughout tests, benches, examples, cli run * wasi-threads: use wasi-common's maybe_exit_on_error in spawned thread not a very modular design, but at this point wasi-common and wasi-threads are forever wed * fix wasmtime's docs * re-introduce wasmtime-wasi's exports of wasi-common definitions behind deprecated * factor out determining i32 process exit code and remove libc dep because rustix provides the same constant * commands/run: inline the logic about aborting on trap since this is the sole place in the codebase its used * Add high-level summary to wasi-common's top-level doc comment. * c-api: fix use of wasi_cap_std_sync => wasi_common::sync, wasmtime_wasi => wasi_common * fix tokio example * think better of combining downcast and masking into one method * fix references to wasmtime_wasi in docs prtest:full * benches: use wasi-common * cfg-if around use of rustix::process because that doesnt exist on windows * wasi-common: include tests, caught by verify-publish * fix another bench * exit requires wasmtime dep. caught by verify-publish. --------- Co-authored-by: Trevor Elliott <telliott@fastly.com> Co-authored-by: Alex Crichton <alex@alexcrichton.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The goal of this PR is to set the stage for wasmtime-wasi::preview2 to be promoted to the root of the wasmtime-wasi crate. This PR is settling up on a bunch of tech debt, and is intended to be backported to the
release-18.0.0branch. Once landed and backported, a follow-up PR will finish this work to be released as part of the19.0release.Now that Preview 2 is out, and wasmtime-wasi::preview2::preview1 is providing wasmtime-cli's default implementation of Preview 1 for a few releases, wasi-common only needs to exist to keep the (zombie, given the proposal status) wasmtime-wasi-threads implementation working. Since most embedders don't care about that, we want to push wasi-common out of the wasmtime-wasi crate.
Historically,
wasi-commonwas "independent" ofwasmtimeandwasmtime-wasiprovided the integration. This hasn't actually been the case for a while, but nowwasi-commonuses a cargo featurewasmtimeto determine whether to take a dependency on wasmtime. In addition, another historical accident ofwasi-commonwas that it defined theWasiFile,WasiDir,WasiSchedand etc traits, but didn't actually provide impls of those traits - those were provided bywasi-cap-std-syncandwasi-tokio. (That architectural wart was mostly because I didn't understand cargo features and optional dependencies when I developed it.) To gloss over that whole mess,wasmtime-wasiprovided a conveniently integrated with wasmtime export ofwasi-commonhooked towasi-cap-std-sync's impls at its root (as well as underwasmtime_wasi::sync) and thewasi-commonhooked towasi-tokiounderwasmtime_wasi::tokio.This PR plows the whole legacy mess over into
wasi-common, and leaveswasmtime_wasi's exports of -common under#[deprecated(since = "18.0.0", note = "... permanent deletion in 19.0"]markers. These deprecations will be released for wasmtime 18 and then go away completely in wasmtime 19.The
wasi-cap-std-syncandwasi-tokiocrates have now been totally inlined atwasi_common::syncandwasi_common::tokio, behind features, as they always could have been. Wasmtime integration for those impls are defined in those mods as well, rather than inwasmtime-wasi.The
wasi-common::maybe_exit_on_errorfunc exists so that the cli and (recoils in horror) wasi-threads can terminate the entire process in the case of a trap, which is a pretty terrible idea in general. Besides providing the deprecated implementation, this PR moves the logic for thepreview2::I32Exithandling to an associated function that extracts an exit code in thewasmtime-wasi::preview2::errormodule, and all calls tostd::process::exitto thewasmtime-clicrate, since that is such an extraordinarily dangerous function. Additionally I switched to using a rustix export for the sigabort exit code, rather than libc, to eliminate an unnecessary dep.The follow-up PR will delete the deprecations (the entire contents of the root except for
pub mod preview2), move the entirewasmtime_wasi::preview2mod up to the root, and shift all of the tests and examples throughout this repository to use wasmtime_wasi instead of wasi_common.