[WasmFS] Make the Node backend optional for web builds#26608
[WasmFS] Make the Node backend optional for web builds#26608kleisauke merged 8 commits intoemscripten-core:mainfrom
Conversation
Previously, the WasmFS Node backend assumed Node.js was always available. This change allows building binaries that include Node backend stubs, so they can run in web environments.
| _wasmfs_node_read: (fd, buf_p, len, pos, nread_p) => {}, | ||
| _wasmfs_node_write: (fd, buf_p, len, pos, nwritten_p) => {}, | ||
| #else | ||
| $wasmfsNodeIsWindows: "!!process.platform.match(/^win/)", |
There was a problem hiding this comment.
This is a small drive-by fix. Previously, this would emit (when building on Linux):
var wasmfsNodeIsWindows = false;in the JS output, which breaks when deploying these binaries on Windows.
As possible follow-up, we could also do:
| $wasmfsNodeIsWindows: "!!process.platform.match(/^win/)", | |
| $wasmfsNodeIsWindows: "!!globalThis.process?.platform.match(/^win/)", |
This would allow a -sENVIRONMENT=node,web build to work on both Node.js and the web, using the same pattern as in other.test_wasmfs_nodefs_stubs, i.e.:
#include <emscripten/emscripten.h>
#include <emscripten/wasmfs.h>
EM_JS(bool, is_node, (), { return ENVIRONMENT_IS_NODE; });
backend_t wasmfs_create_root_dir() {
return is_node() ? wasmfs_create_node_backend("")
: wasmfs_create_memory_backend();
}
src/lib/libwasmfs_node.js
Outdated
| _wasmfs_node_readlink: (path_p, target_p, bufsize) => {}, | ||
| _wasmfs_node_close: (fd) => {}, | ||
| _wasmfs_node_read: (fd, buf_p, len, pos, nread_p) => {}, | ||
| _wasmfs_node_write: (fd, buf_p, len, pos, nwritten_p) => {}, |
There was a problem hiding this comment.
Should these assert/abort or return non-zero?
Returning nothing/undefined here means they implicitly return zero to wasm
There was a problem hiding this comment.
Good idea! Done (and simplified) with commit d30f7de.
| @@ -5,7 +5,26 @@ | |||
| */ | |||
There was a problem hiding this comment.
What about simply not allowing this file to be included at all it !ENVIRONMENT_MAY_BE_NODE?
There was a problem hiding this comment.
That would lead to linker failures. For example, if I remove all these stubs and run ./test/runner other.test_wasmfs_nodefs_stubs, I see:
Details
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_fstat_size
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_stat_size
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_open
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_open
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_close
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_close
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_read
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_write
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_ftruncate
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_truncate
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_get_mode
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_insert_file
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_insert_directory
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_symlink
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_rename
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_unlink
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_rmdir
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_readdir
wasm-ld: error: /home/kleisauke/emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs-debug.a(node_backend.o): undefined symbol: _wasmfs_node_readlink
...I also thought about moving these stubs to native code and using weak linkage (like how wasmfs_create_root_dir() is handled), but this seemed easier.
There was a problem hiding this comment.
Fair enough. I guess we should abort/assert in debug builds there though.
src/lib/libwasmfs_node.js
Outdated
| '}'; | ||
| }); | ||
|
|
||
| library[x] = eval(`(${t})`); |
There was a problem hiding this comment.
Why is eval needed here? I don't think we normally use eval in combination with modifyJSFunction
There was a problem hiding this comment.
I copied-pasted this from wrapSyscallFunction():
Line 2596 in 34f6bcf
There was a problem hiding this comment.
... but you're right, it seems unnecessary. Fixed with commit 4143136.
src/lib/libwasmfs_node.js
Outdated
| delete library[x + '__deps']; | ||
| t = modifyJSFunction(t, (args, body) => { | ||
| return `(${args}) => {\n` + | ||
| (ASSERTIONS ? "abort('wasmfs::NodeBackend is no-op when !ENVIRONMENT_MAY_BE_NODE');\n" : '') + |
There was a problem hiding this comment.
How about just abort('attempt to call nodejs backend function without ENVIRONMENT_MAY_BE_NODE)`.
I think the abort should exist even in release builds (you can make the error message optional if you like) since we never actually want a no-op behaviour here do we?
There was a problem hiding this comment.
Commit eea0053 adjusts the error message.
I tried enabling abort() in release builds, but the wasmImports for these functions are somehow being DCE'd when building other.test_wasmfs_nodefs_stubs with -sASSERTIONS=0 or -O3, which is unexpected. I'll investigate.
There was a problem hiding this comment.
... it was able to DCE away these functions as main() was empty, commit 4b86c1c fixes this.
There was a problem hiding this comment.
Regarding abort() in release builds, perhaps that should be done as follow-up? I also noticed this in wrapSyscallFunction():
Lines 2514 to 2524 in 34f6bcf
src/lib/libwasmfs_node.js
Outdated
| '}'; | ||
| }); | ||
|
|
||
| library[x] = t; |
There was a problem hiding this comment.
Just combine this with the line above?
src/lib/libwasmfs_node.js
Outdated
|
|
||
| addToLibrary({ | ||
| $wasmfsNodeIsWindows: !!process.platform.match(/^win/), | ||
| var WasmFSNodeLibrary = { |
There was a problem hiding this comment.
JS names start with lowercase.
This avoids filesystem related functions from being DCE'd when building with `-sASSERTIONS=0` or `-O3`.
test/test_other.py
Outdated
| return 0; | ||
| } | ||
| ''') | ||
| self.run_process([EMCC, 'main.c', '-sWASMFS', '-sENVIRONMENT=web']) |
There was a problem hiding this comment.
I'm not sure I understand what this test is doing exactly. Its not actually running the result so what is it supposed to show?
Also, when -sENVIRONMENT=web I think ENVIRONMENT_IS_NODE not even be declared/defined so this program would just crash with undefined reference I think?
There was a problem hiding this comment.
This was a test for the linker issue mentioned in comment #26608 (comment), but since commit d30f7de it's probably no longer relevant, as we can now be sure that these stubs are always in sync.
Commit d0c6c63 removes this test.
Also, when
-sENVIRONMENT=webI thinkENVIRONMENT_IS_NODEnot even be declared/defined so this program would just crash with undefined reference I think?
I just checked this, but ENVIRONMENT_IS_NODE is also defined in -sENVIRONMENT=web builds.
There was a problem hiding this comment.
... PR #26624 reinstates this test in test_browser.py.
test/test_other.py
Outdated
| self.do_runf('access.c', cflags=['-sNODERAWFS'], args=[os.path.abspath('foo')]) | ||
|
|
||
| def test_wasmfs_nodefs_stubs(self): | ||
| # This is equivalent to building with `-sWASMFS -sNODERAWFS`, except that the Wasm |
There was a problem hiding this comment.
I'm not sure I under stand that the This is here?
There was a problem hiding this comment.
This comment should probably be part of the wasmfs_create_root_dir() hook.
It became superfluous after commit d30f7de.
Previously, the WasmFS Node backend assumed Node.js was always available. This change allows building binaries that include Node backend stubs, so they can run in web environments.
Split out from #24733.