WasmFS: Open and close files in the JS API _wasmfs_write_file - #19397
Merged
Conversation
tlively
approved these changes
May 19, 2023
tlively
left a comment
tlively
left a comment
Member
There was a problem hiding this comment.
Seems reasonable. There are definitely situations where an open might fail but a write would not, for example for files that have been unlinked already (for backends that support that) or perhaps for special files like pipes that don't expect to be opened at all, but in those cases I would expect users to use lower level APIs rather than this all-in-one high level API.
kripken
added a commit
that referenced
this pull request
May 22, 2023
This PR makes the NODERAWFS flag make the root directory be created using the Node backend. That may not be 100% of what we need for NODERAWFS but it gets a significant amount of tests passing. To do that, add a hook that backends can use to override creation of the root directory. Then NODERAWFS support basically is just to link in a tiny library that overrides that hook. This adds a new test, test_noderawfs_wasmfs which verifies we write files out to the normal filesystem. Also, enable test_freetype, which now passes; previously it failed during a configure step (specifically, configure tried to write the size of an integer to a file... before this PR, the file was empty and it defined integer size as the empty string). So test_freetype both verifies NODERAWFS writes, and also it uses files internally (to read font files etc.) which seems useful to enable. Also, this is tested in test_fs_writeFile_rawfs. That test does FS.writeFile with NODERAWFS enabled. It passed before, because enabling NODERAWFS did nothing... for WasmFS the test really just worked on the MemoryFile backend, since it doesn't create a Node backend. With this PR, the test properly uses Node files, and passes (thanks to #19397 and #19396). Note: other.test_unistd_fstatfs_wasmfs was removed from CI because it was unnecessary - all other tests run anyhow. We only need to add select wasmfs tests because that mode does not run in full already.
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
2 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 memory backend appears to allow
lockedFile.writewithout.openfirst,but other backends like Node do not. In general, it seems correct to open
the file before writing and close it after, and the read JS API call does so, so this
fixes the write one.
This is necessary for NODERAWFS support and will be tested in that PR.