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: replaces existing filesystem apis with fs.FS #394

Merged
merged 5 commits into from
Mar 24, 2022

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Mar 18, 2022

This changes SysConfig.WithFS and SysConfig.WithWorkDirFS to accept an fs.FS for the root ("/") and working directory (".") file systems. This allows end users to leverage standard libraries that adhere to this, including OS, HTTP and embedded filesystems.

For example, here's how you can allow WebAssembly modules to read
"/work/home/a.txt" as "/a.txt" or "./a.txt":

wasi, err := r.InstantiateModule(wazero.WASISnapshotPreview1())
defer wasi.Close()

sysConfig := wazero.NewSysConfig().WithFS(os.DirFS("/work/home"))
module, err := wazero.StartWASICommandWithConfig(r, compiled, sysConfig)
defer module.Close()
...

Note: Further development will happen after this, for example to allow creation of new directories, though priority of that is demand-based. If you need further FS features, you should ask on #390 with your use-case!

@codefromthecrypt codefromthecrypt marked this pull request as draft March 18, 2022 22:44
codefromthecrypt pushed a commit that referenced this pull request Mar 21, 2022
The prior design had a problem, where multiple imports of WASI would end
up having different file descriptors for the same file. Moreover, there
was no means to close any of these when a module importing WASI was
closed.

This moves all existing functionality to a new type SystemContext, which
is owned by the importing module, similar to how it owns its memory.
While this PR doesn't fix some problems such as unclosed files, the code
is now organized in a way it can be, and these issues will be resolved
by #394.

In order to fix scope, `WASISnapshotPreview1WithConfig` had to be
removed.

Signed-off-by: Adrian Cole <[email protected]>
codefromthecrypt added a commit that referenced this pull request Mar 21, 2022
The prior design had a problem, where multiple imports of WASI would end
up having different file descriptors for the same file. Moreover, there
was no means to close any of these when a module importing WASI was
closed.

This moves all existing functionality to a new type SystemContext, which
is owned by the importing module, similar to how it owns its memory.
While this PR doesn't fix some problems such as unclosed files, the code
is now organized in a way it can be, and these issues will be resolved
by #394.

In order to fix scope, `WASISnapshotPreview1WithConfig` had to be
removed.

Signed-off-by: Adrian Cole <[email protected]>
@codefromthecrypt codefromthecrypt force-pushed the wasi-WithFS branch 2 times, most recently from 44fba44 to 84a369e Compare March 23, 2022 07:21
@tetratelabs tetratelabs deleted a comment from nullpo-head Mar 23, 2022
@codefromthecrypt
Copy link
Contributor Author

ok this is rebased. next steps before ready for review:

  1. fix whatever's going on in the write tests in windows
  2. Test_Cat is the only thing actually using this. Add tests to ensure working directory and root directory paths work
  3. scrub for TODOs and fix here, leave them or open a new issue.

Adrian Cole added 3 commits March 24, 2022 14:44
Signed-off-by: Adrian Cole <[email protected]>
Signed-off-by: Adrian Cole <[email protected]>
Signed-off-by: Adrian Cole <[email protected]>
@codefromthecrypt codefromthecrypt marked this pull request as ready for review March 24, 2022 07:26
@codefromthecrypt
Copy link
Contributor Author

opened #408 to follow-up at some point and add another kind of integration test, now that it is easy to compare against an fs.FS

// // Files relative to this source under appA is available under "/" and files relative to "/work/appA" under ".".
// sysConfig := wazero.NewSysConfig().WithFS(rootFS).WithWorkDirFS(os.DirFS("/work/appA"))
//
// Note: os.DirFS documentation includes important notes about isolation, which also applies to fs.Sub. As of Go 1.18,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

took some reading to discover there are no plans for an actual chrooted FS. Until then, folks with high isolation needs will have to write their own fs.FS impl (or find some library that isn't abandoned that tries to do this!)

Signed-off-by: Adrian Cole <[email protected]>
@codefromthecrypt
Copy link
Contributor Author

cc @pims this is an API break, but a good one as we've gotten rid of an incompatible FS and formalized the API. Hope this doesn't drift you too much!

@codefromthecrypt codefromthecrypt merged commit 000cbde into main Mar 24, 2022
@codefromthecrypt codefromthecrypt deleted the wasi-WithFS branch March 24, 2022 07:35
@pims
Copy link
Contributor

pims commented Mar 24, 2022

Thanks for the heads-up @codefromthecrypt

This looks like a very sensible change and until 1.0 is released, no stress about API breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants