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

Allow passing fs.FS when calling functions #571

Merged
merged 2 commits into from
May 20, 2022

Conversation

knqyf263
Copy link
Contributor

@knqyf263 knqyf263 commented May 17, 2022

Issue

Close #563

ToDo

  • FSContext
  • FSConfig
  • FSKey
  • Override fs.FS in WASI
  • Add an example
  • Unit test
  • Example test

internal/wasm/sys.go Outdated Show resolved Hide resolved
experimental/fs/fs.go Outdated Show resolved Hide resolved
experimental/fs/fs.go Outdated Show resolved Hide resolved
@codefromthecrypt
Copy link
Contributor

taking a look now, thanks!

Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started, as it helps me know where you are going. I nudged towards a specific direction, so that we can avoid exporting more api-visible symbols and get the same result.

experimental/fs/fs.go Outdated Show resolved Hide resolved
experimental/fs/fs.go Outdated Show resolved Hide resolved
experimental/fs/fs.go Outdated Show resolved Hide resolved
internal/wasm/fs.go Outdated Show resolved Hide resolved
wasi/wasi.go Outdated Show resolved Hide resolved
@codefromthecrypt
Copy link
Contributor

ping me when you are ready for the next round. I'll keep a look out

@knqyf263
Copy link
Contributor Author

knqyf263 commented May 17, 2022

@codefromthecrypt I think I finished reflecting your comments. There are a few concerns though.

  1. I've added a test for WithFS to PathOpen only.
    • I'm not sure if it is enough or you may prefer another approach
  2. FSConfig is still embedded into ModuleConfig
    • Do you think it should be extracted?

Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

I wanted to reply quickly to you about some confusion I caused. I apologize for wasting some of your time. In general this looks good.

The main test needed would be to ensure that the context FS overrides vs adds to the one you expect. So one neat way to prove that is to have a test with the base FS having two files and the overlay having only one with a different content.

Meanwhile once done experimenting, maybe roll back the changes to the examples/wasi as we don't put experiemental things in that dir.

Finally, thanks again, I think after the adjustments here, this is getting to polish stage!

wasi/wasi.go Outdated Show resolved Hide resolved
internal/wasm/sys.go Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
@codefromthecrypt
Copy link
Contributor

@mathetake I think this is on the right track. I'm going to be mostly away this evening until tomorrow. Can you make sure this gets past the finish line. Cheers!

@mathetake
Copy link
Member

I think this is on the right track :D

@mathetake mathetake marked this pull request as ready for review May 18, 2022 00:10
Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

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

I'm good when you two feel good to go!

@knqyf263 knqyf263 force-pushed the fs_context branch 3 times, most recently from 8310282 to 56714b8 Compare May 18, 2022 06:48
@knqyf263
Copy link
Contributor Author

Meanwhile once done experimenting, maybe roll back the changes to the examples/wasi as we don't put experiemental things in that dir.

Reverted. 56714b8

@knqyf263
Copy link
Contributor Author

The main test needed would be to ensure that the context FS overrides vs adds to the one you expect. So one neat way to prove that is to have a test with the base FS having two files and the overlay having only one with a different content.

I wanted to do that, but I didn't find a good place to add the test. Also, it appeared to require somewhat larger changes. I probably missed something.

BTW, please feel free to add any commits. Sometimes it's faster to write code than to explain it in writing.

@codefromthecrypt
Copy link
Contributor

@knqyf263 thanks for all the work here. I'll be at the conference all day, but I can review and add commits to address any things, possibly today but more likely by tomorrow. In any case thanks for putting this together as you made it possible to land during a busy time 👍

@codefromthecrypt
Copy link
Contributor

ps I started fiddling with this, but might not push commits until tomorrow. Main thing I noticed was tension in the packages and made internal/fs instead. There are some tests in sys_test.go that should move into there also. I'll do it!

@codefromthecrypt
Copy link
Contributor

I landed the other PRs that would drift this, and ran out of time to finish this up (still at conference and should meet people :) ). I'll finish this soon

@codefromthecrypt
Copy link
Contributor

I force pushed a squash of the preceding work because the merges were getting hairy. I'll add a commit later with the changes I mentioned above

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

@mathetake I finished re-org, can you take a look and add a commit as needed? otherwise merge it.

@mathetake mathetake merged commit 7794530 into tetratelabs:main May 20, 2022
@mathetake
Copy link
Member

mathetake commented May 20, 2022

@knqyf263 thank you so much for working on this! 🏅 👍

@knqyf263
Copy link
Contributor Author

@codefromthecrypt Thanks for your help!
I saw you added internal/fs package. It looks good!

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.

Assign the file system when calling function
3 participants