Make WASMFS FS.createPath match the FS version behavior on EEXIST #23603
Conversation
If the file packager tries to add content to *an existing path* in the filesystem (e.g., if the filesystem already has some directories, OPFS content, loading multiple package, etc), the different behavior of createPath causes an exception on WASMFS and no exception on JS FS.
JoeOsborn
force-pushed
the
fix-wasmfs-createpath
branch
from
83a2bc9 to
af57bd6
Compare
|
I've added a test. The trigger I noticed is when you load multiple packages which overlap at all in their target paths, or when you load a package into a filesystem that already has content (even if it doesn't overlap with the actual package contents). createPath was also just less pleasant to use in WASMFS compared to JS FS due to this difference. |
JoeOsborn
force-pushed
the
fix-wasmfs-createpath
branch
from
bc18c55 to
3e2760e
Compare
|
So I didn't add a test specifically for createPath, because I do like that the current test exercises the separate file packager and createPath isn't documented to succeed or fail with respect to whether the paths exist already. I can add a separate JS test if you think it's important. |
|
The fs_js_api tests seem a bit broken. They don't work in strict mode (and changing the octal literals to 0o777 causes breakage in other conditions) or WASM64, and there's a memory leak in wasmfs found during read. Do you still want me to add a WASMFS condition to test_fs_js_api? |
My understanding is the test_fs_js_api was added precisely so that we could test the wasmfs versions of the APIs, so I think we should fix them if they are broken, yes. I'm a little confused as to why its not marked as |
test/third_party/googletest
Outdated
There was a problem hiding this comment.
Sorry, I didn't mean to include this.
JoeOsborn
force-pushed
the
fix-wasmfs-createpath
branch
from
f079bba to
f07e4ac
Compare
|
So I tried to move my packager test into test_other, but it actually only breaks in browser (node has its own filesystem stuff used by the file_packager scripts). Is there a good way to write a test_other test that still runs in a browser environment? Currently my packager test is still in test_core |
No, the point of test_other is that doesn't run in the browser. |
| @@ -111,7 +111,11 @@ FS.init(); | |||
| if (!wasmFSPreloadingFlushed) { | |||
| wasmFSPreloadedDirs.push({parentPath: parent, childName: part}); | |||
| } else { | |||
| FS.mkdir(current); | |||
| try { | |||
| FS.mkdir(current); | |||
There was a problem hiding this comment.
I'm actually little confused by this fix. I tried running wasmfs.test_fs_js_api and it passed both with or without this change.
Looking into it it looks the repeated calls to FS.mkdir here always seem to succeed. I modified the code here to add some extra debugging and I see:
mkdir /home/nested1
0
mkdir /home/nested2
0
I'm a little confused how you were hitting an error here? I must be missing something.
There was a problem hiding this comment.
Hmm even stranger, browser.test_emscripten_overlapped_package seem to pass both before and after this test.
There was a problem hiding this comment.
Ah I wasn't running browser.test_emscripten_overlapped_package_wasmfs.. that one does fail.
There was a problem hiding this comment.
I found the issue with test_fs_js_api: #23645
This added test coverage for emscripten-core#23603.
This added test coverage for #23603.
Fixes #23602.