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

Sync wasi-filesystem with wit definitions in standards repo #6791

Merged
merged 24 commits into from
Aug 8, 2023

Conversation

pchickey
Copy link
Contributor

@pchickey pchickey commented Aug 1, 2023

This PR pulls in the latest wit files from the wasi-filesystem repo, with the addition of just one pending upstream change to remove the nonblocking descriptor-flag.

The changes look like:

  • the wasi:filesystem/filesystem interface has been renamed wasi:filesystem/types. This resulted in lots of renaming across the repo. For the main filesystem impl I use the new types:: module prefix but in most other places I followed use ...::filesystem::types as filesystem; to make it clearer to read.
  • the interface removes device and inode from the descriptor-stat struct, and provides new metadata-hash family functions to replace this concept.
    • these new methods are implemented in preview2 host, using std::hash::Hasher to hash the device and inode of the file.
    • the preview 1 adapters still need to fill in a device and inode in some places (fd_filestat_get and fd_readdir). I elected to fill in 1 for the device (because the stdio stream "files" use 0) and use the lower 64 bits of the metadata hash to fill in the inode.
    • the preview 1 host adapter needed to use an async method to calculate each inode in fd_readdir, so I had to move some calculation out of the iterator map and into a for loop, done eagerly for each entry in the directory. Ideally this would be lazy for only entries in the directory that are being serialized as part of the adapter, or cache it between entries, but I didn't consider this worthwhile to invest in right now.
  • the new wasi:filesystem/preopens interface replaces the local wasi:cli-base/preopens. (There is no such thing as an upstream cli-base package, but morally it is in wasi-cli)

See also:
WebAssembly/wasi-filesystem#125: remove nonblocking descriptor flag. still open.
WebAssembly/wasi-filesystem#126: fix parameters to metadata functions. merged.

Pat Hickey added 4 commits July 31, 2023 16:57
…h/metadata_hash_fixes

upstream main changes the interface to be named `types`, and has
changes to descriptor-stat, directory-entry, adds the metadata hash
methods.

pch/blocking_is_a_stream_concern upstreams the deletion of non-blocking
from descriptor flags.

pch/metadata_hash_fixes adds missing this: descriptor arguments to
the metadata-hash and metadata-hash-at methods.
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Aug 1, 2023
@pchickey pchickey marked this pull request as ready for review August 2, 2023 02:16
@pchickey pchickey requested a review from a team as a code owner August 2, 2023 02:16
@pchickey pchickey requested review from itsrainy, elliottt and sunfishcode and removed request for a team and itsrainy August 2, 2023 02:16
Copy link
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

I think this looks great! Just had a question about the hasher choice before merging.

Comment on lines 56 to 57
"streams"::"stream-error": Error,
"filesystem"::"error-code": Error,
"types"::"error-code": Error,
Copy link
Member

Choose a reason for hiding this comment

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

These will need to be updated after #6795 merges.

crates/wasi/src/preview2/preview2/filesystem.rs Outdated Show resolved Hide resolved
Copy link
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

🚀

@pchickey pchickey enabled auto-merge August 3, 2023 21:21
@pchickey pchickey added this pull request to the merge queue Aug 4, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 4, 2023
wiggle takes care of endianness translation. The to_le in fd_readdir
entries is because that implementation is writing structs directly to
memory (very cursed)

s390x CI caught this bug
@pchickey pchickey force-pushed the pch/sync_wasi_filesystem_wit branch from 154b82a to 9ae2212 Compare August 4, 2023 23:43
@pchickey pchickey added this pull request to the merge queue Aug 7, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 7, 2023
Pat Hickey and others added 2 commits August 7, 2023 15:13
…emory

fd_readdir will write its results to a buffer, and needs to be careful
with endianness as a result. However, fd_filestat_get returns the value
directly, and as such does not need to call `to_le`.
@elliottt elliottt enabled auto-merge August 8, 2023 22:06
@elliottt elliottt disabled auto-merge August 8, 2023 22:26
@elliottt elliottt enabled auto-merge August 8, 2023 22:26
@elliottt elliottt added this pull request to the merge queue Aug 8, 2023
Merged via the queue into main with commit dd87d29 Aug 8, 2023
@elliottt elliottt deleted the pch/sync_wasi_filesystem_wit branch August 8, 2023 23:36
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Aug 18, 2023
…alliance#6791)

* filesystem.wit: upstream main + pch/blocking_is_a_stream_concern + pch/metadata_hash_fixes

upstream main changes the interface to be named `types`, and has
changes to descriptor-stat, directory-entry, adds the metadata hash
methods.

pch/blocking_is_a_stream_concern upstreams the deletion of non-blocking
from descriptor flags.

pch/metadata_hash_fixes adds missing this: descriptor arguments to
the metadata-hash and metadata-hash-at methods.

* copy in preopens.wit and world.wit from wasi-filesystem repo, and delete wasi-cli-base/preopens.wit

* other wits: fix names of filesystem/{types, preopens}

* fix bindgen invocations and import paths for filesystem changes

* component adapter: fix bindings paths for preopens and filesystem

* wip

* wippp

* fill in a reasonable-ish metadata hash impl

* preview 1 adapter: we need async in the dir entries, so collect the whole thing to a vec

this isnt great but i dont have the time or energy to do this better
right now

* component adapter: use metadata hash to fake ino

* wasi-tests: fix warnings

* reactor-tests: fix filesystem paths

* only create a BuildHasher impl once

* consistiency

* component adapter: use descriptor to call filesystem funcs, not the fd

* reactor test: fix filesystem types paths

* metadata hash: use DefaultHasher

* fix missed trappable-error-type merge conflicts

* preview1-in-preview2: dont use to_le in Filestat assignment of ino

wiggle takes care of endianness translation. The to_le in fd_readdir
entries is because that implementation is writing structs directly to
memory (very cursed)

s390x CI caught this bug

* debugging: forward stdio for fd_readdir test so we can figure out CI failure on s390x

prtest:full

* Don't convert filestat ino to little-endian, as it's not written to memory

fd_readdir will write its results to a buffer, and needs to be careful
with endianness as a result. However, fd_filestat_get returns the value
directly, and as such does not need to call `to_le`.

---------

Co-authored-by: Trevor Elliott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants