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

wasi-filesystem: delete access modes #7510

Merged
merged 3 commits into from
Nov 9, 2023
Merged

Conversation

pchickey
Copy link
Contributor

@pchickey pchickey commented Nov 8, 2023

Implements the change for WebAssembly/wasi-filesystem#139: Remove the definition of access modes from the wasi-filesystem interface, the three functions for reading and modifying those modes, and the modes argument from open_at.

We never made time to implement these in Wasmtime, so the implementations deleted are all just todo!()s. We will add these functions back to wasi-filesystem in a future spec revision and accompany them with an implementation.

@pchickey pchickey force-pushed the pch/delete_filesystem_modes branch from 83b7be0 to 77cbdbf Compare November 8, 2023 23:33
@pchickey pchickey marked this pull request as ready for review November 8, 2023 23:38
@pchickey pchickey requested a review from a team as a code owner November 8, 2023 23:38
@pchickey pchickey requested review from alexcrichton and removed request for a team November 8, 2023 23:38
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Nov 9, 2023
Comment on lines -541 to -542
/// Permissions to use when creating a new file.
modes: modes
Copy link
Member

Choose a reason for hiding this comment

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

Question on this, if we know we want to add this in the future should we be trying to future-proof this function? For example we could add an options: option<open-options> with a currently empty resource open-options or something like that.

Alternatively we could add a feature to the component model and WIT for something like keyword arguments with defaults. That's a much longer- term vision though.

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 believe that we can eliminate the modes parameter here and let open-at continue to do the reasonable default behavior, and then when users of the re-introduced modes stuff want to opt into a non-default behavior for modes, we can expose a new open-at-modes function that has the additional argument. Then when it comes time to make a breaking change in preview 3 we can consolidate down to just one open-at with mode arguments.

We could get fancier than that, but I expect the above will work fine without introducing much complexity.

@alexcrichton alexcrichton added this pull request to the merge queue Nov 9, 2023
Merged via the queue into main with commit aebf163 Nov 9, 2023
20 checks passed
@alexcrichton alexcrichton deleted the pch/delete_filesystem_modes branch November 9, 2023 19:10
alexcrichton pushed a commit to alexcrichton/wasmtime that referenced this pull request Nov 9, 2023
* import wit changes from wasi-filesystem#139

* remove modes from wasi-filesystem implementations

* component adapter: remove modes from call to open_at
alexcrichton added a commit that referenced this pull request Nov 9, 2023
* import wit changes from wasi-filesystem#139

* remove modes from wasi-filesystem implementations

* component adapter: remove modes from call to open_at

Co-authored-by: Pat Hickey <phickey@fastly.com>
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