Skip to content

Add createFile to JS API for wasmfs (#23667) #23668

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add createFile to JS API for wasmfs (#23667) #23668

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
82c5e54
Add createFile to JS API for wasmfs (#23667)
JoeOsborn Feb 13, 2025
587c135
Add test for createfile
JoeOsborn Feb 13, 2025
24edb2f
Revert year change
JoeOsborn Feb 13, 2025
41ce74a
Change WASMFS createFile to match JS API arg order
JoeOsborn Feb 14, 2025
5edbd0b
Fix improper use of assert in JS tests
JoeOsborn Feb 14, 2025
893d559
Missing comma
JoeOsborn Feb 14, 2025
cc7f2af
fix createFile in case of non-string parent
JoeOsborn Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/lib/libwasmfs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ FS.init();
FS.ErrnoError.prototype = new Error();
FS.ErrnoError.prototype.constructor = FS.ErrnoError;
},
createFile: (path, mode, backend) => FS.handleError(withStackSave(() => _wasmfs_create_file(stringToUTF8OnStack(path), mode, backend))),
createDataFile(parent, name, fileData, canRead, canWrite, canOwn) {
FS_createDataFile(parent, name, fileData, canRead, canWrite, canOwn);
},
Expand Down
14 changes: 13 additions & 1 deletion test/fs/test_fs_js_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2023 The Emscripten Authors. All rights reserved.
* Copyright 2025 The Emscripten Authors. All rights reserved.
Copy link
Collaborator

@sbc100 sbc100 Feb 13, 2025

Choose a reason for hiding this comment

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

We don't update out copyright dates when we modify files

* Emscripten is available under two separate licenses, the MIT license and the
* University of Illinois/NCSA Open Source License. Both these licenses can be
* found in the LICENSE file.
Expand Down Expand Up @@ -469,6 +469,17 @@ void cleanup() {
remove("closetestfile");
}

#if WASMFS
Copy link
Collaborator

@sbc100 sbc100 Feb 13, 2025

Choose a reason for hiding this comment

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

This API is not specific to WASMFS is it? I see it exists in libfs.js too.

Copy link
Contributor Author

@JoeOsborn JoeOsborn Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh indeed it does, I had no idea. But this version of the test is wasm-specific since it uses a backend. It also looks like the libfs version ignores the "properties" field and has a different interface.

Should a different name than createFile be picked for the wasmfs version? createFileInBackend?

Copy link
Collaborator

@sbc100 sbc100 Feb 13, 2025

Choose a reason for hiding this comment

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

I think the goal with the JS API is mostly feature parity with the existing/old FS. If possible I thin we should avoid extending the JS API.

But yes, if we decide we really do need a new API we should give it a new name.

Would you make making a version of createFile that is compatible with the old FS and testing that as part of this PR? Or as a separate PR?

Copy link
Contributor Author

@JoeOsborn JoeOsborn Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I've made WASMFS createFile compatible with the JS version. The JS version never used the third parameter, so I'm using it for the backend here. I also made it so the test (though a bit different) runs on both filesystems.

void test_fs_createfile_js() {
EM_ASM(
var backend = MEMFS.createBackend({});
var file = FS.createFile("/test.txt", 0o777, backend);
assert(file > 0);
FS.close(file);
);
}
#endif

int main() {
test_fs_open();
test_fs_createPath();
Expand All @@ -483,6 +494,7 @@ int main() {
#if WASMFS
// TODO: Fix legacy API FS.mmap bug involving emscripten_builtin_memalign
test_fs_mmap();
test_fs_createfile_js();
#endif
test_fs_mkdirTree();
test_fs_utime();
Expand Down