Ensure compatibility with older versions of Node.js #21863
Conversation
The previous `fs.writeSync` API was not available in Node.js 17.x, see: https://nodejs.org/api/fs.html#fswritesyncfd-buffer-options Also, do the same for `fs.readSync` to ensure compat with Node.js < v13.13.0. Resolves emscripten-core#21118.
| @@ -276,14 +276,22 @@ addToLibrary({ | |||
| // Node.js < 6 compatibility: node errors on 0 length reads | |||
| if (length === 0) return 0; | |||
| try { | |||
| return fs.readSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), { position: position }); | |||
| #if MIN_NODE_VERSION < 131300 | |||
| return fs.readSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), 0, length, position); | |||
There was a problem hiding this comment.
Do newer versions of node also support this old pattern? If so, can we not just use this older pattern everwhere? Is there any downside? It looks like it creates less gabage since you don't need to create a new option object each call.
If newer versions don't support this pattern then I suppose we would want a runtime check rather than a compile time one.
There was a problem hiding this comment.
Newer versions of Node.js ought to support this too. I guarded this for code size reasons, but perhaps that is negligible.
There was a problem hiding this comment.
Commit 65a6ff3 prefers the old API instead.
kleisauke
force-pushed
the
old-nodejs-compat
branch
from
3663d34 to
65a6ff3
Compare
|
Can we add some test to verify that fixes the issue. (See |
I tried to verify this locally with Node.js v10.19.0 but it failed on the optional chaining ( $ ./test/runner core0.test_fs_js_api core0.test_fs_write
Test suites:
['test_core']
Running test_core: (2 tests)
Using 2 parallel test processes
-- begin program output --
/tmp/emtest_qrar9kcz/emscripten_test_core0_26cak6rw/test_fs_js_api.js:649
Module['monitorRunDependencies']?.(runDependencies);
^
SyntaxError: Unexpected token .
at Module._compile (internal/modules/cjs/loader.js:723:23)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
at Module.load (internal/modules/cjs/loader.js:653:32)
at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
at Function.Module._load (internal/modules/cjs/loader.js:585:3)
at Function.Module.runMain (internal/modules/cjs/loader.js:831:12)
at startup (internal/bootstrap/node.js:283:19)
at bootstrapNodeJSCore (internal/bootstrap/node.js:623:3)
-- end program output --
test_fs_js_api (test_core.core0.test_fs_js_api) ... FAIL
-- begin program output --
/tmp/emtest_mmgpbjvl/emscripten_test_core0_8rucs0x_/test_write.js:575
Module['monitorRunDependencies']?.(runDependencies);
^
SyntaxError: Unexpected token .
at Module._compile (internal/modules/cjs/loader.js:723:23)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
at Module.load (internal/modules/cjs/loader.js:653:32)
at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
at Function.Module._load (internal/modules/cjs/loader.js:585:3)
at Function.Module.runMain (internal/modules/cjs/loader.js:831:12)
at startup (internal/bootstrap/node.js:283:19)
at bootstrapNodeJSCore (internal/bootstrap/node.js:623:3)
-- end program output --
test_fs_write (test_core.core0.test_fs_write) ... FAIL
...FWIW, I could also reproduce this with the emscripten/.circleci/config.yml Line 764 in 63c648f |
The test framework should automatically inject |
|
Commit 99b0518 adds a test case for this. I had to modify the unit test to prevent an Details$ grep NODE_JS_TEST .emscripten
NODE_JS_TEST = [os.path.expanduser('~/Downloads/node-v10.19.0-linux-x64/bin/node')]
JS_ENGINES = [NODE_JS_TEST, V8_ENGINE]
$ ./test/runner core0.test_fs_write_rawfs
Test suites:
['test_core']
Running test_core: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
test_fs_write_rawfs (test_core.core0.test_fs_write_rawfs) ... (test did not pass in JS engine: ['/home/kleisauke/Downloads/node-v10.19.0-linux-x64/bin/node'])
FAIL
======================================================================
FAIL: test_fs_write_rawfs (test_core.core0.test_fs_write_rawfs)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/kleisauke/emscripten/test/common.py", line 710, in resulting_test
return func(self, *args)
^^^^^^^^^^^^^^^^^
File "/home/kleisauke/emscripten/test/test_core.py", line 160, in metafunc
func(self)
File "/home/kleisauke/emscripten/test/test_core.py", line 5829, in test_fs_write
self.do_run_in_out_file_test('fs/test_write.cpp')
File "/home/kleisauke/emscripten/test/common.py", line 1692, in do_run_in_out_file_test
output = self._build_and_run(srcfile, expected, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/kleisauke/emscripten/test/common.py", line 1743, in _build_and_run
self.assertContained(o, js_output, regex=regex)
File "/home/kleisauke/emscripten/test/common.py", line 1416, in assertContained
self.fail("Expected to find '%s' in '%s', diff:\n\n%s\n%s" % (
AssertionError: Expected to find 'read Hello! 1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
' in 'read
', diff:
--- expected
+++ actual
@@ -1 +1,2 @@
-read Hello! 1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
+read
+
$ git checkout old-nodejs-compat
M .circleci/config.yml
M test/fs/test_write.cpp
M test/test_core.py
Switched to branch 'old-nodejs-compat'
$ ./test/runner core0.test_fs_write_rawfs
Test suites:
['test_core']
Running test_core: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
test_fs_write_rawfs (test_core.core0.test_fs_write_rawfs) ... ok
----------------------------------------------------------------------
Ran 1 test in 1.125s
OK
$ git restore test/fs/test_write.cpp
$ ./test/runner core0.test_fs_write_rawfs
Test suites:
['test_core']
Running test_core: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
test_fs_write_rawfs (test_core.core0.test_fs_write_rawfs) ... -- begin program output --
/home/kleisauke/emscripten/out/test/test_write.js:113
throw ex;
^
RangeError: Invalid typed array length: 101
at new Int8Array (<anonymous>)
at write (/home/kleisauke/emscripten/out/test/test_write.js:2405:49)
at Object.write (/home/kleisauke/emscripten/out/test/test_write.js:4985:16)
at Object.79964 (/home/kleisauke/emscripten/out/test/test_write.js:840:8)
at runEmAsmFunction (/home/kleisauke/emscripten/out/test/test_write.js:4465:26)
at _emscripten_asm_const_int (/home/kleisauke/emscripten/out/test/test_write.js:4468:10)
at wasm-function[16]:73
at wasm-function[100]:3
at /home/kleisauke/emscripten/out/test/test_write.js:578:12
at callMain (/home/kleisauke/emscripten/out/test/test_write.js:5075:15)
at doRun (/home/kleisauke/emscripten/out/test/test_write.js:5113:23)
at run (/home/kleisauke/emscripten/out/test/test_write.js:5125:5)
at runCaller (/home/kleisauke/emscripten/out/test/test_write.js:5065:19)
at removeRunDependency (/home/kleisauke/emscripten/out/test/test_write.js:515:7)
at receiveInstance (/home/kleisauke/emscripten/out/test/test_write.js:694:5)
at receiveInstantiationResult (/home/kleisauke/emscripten/out/test/test_write.js:712:5)
-- end program output --
FAIL
... |
test/fs/test_write.cpp
Outdated
| @@ -24,7 +24,7 @@ int main() | |||
| var pos = lengthBytesUTF8(str); | |||
|
|
|||
| str = '1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890'; | |||
| data = new Uint8Array(100); | |||
| data = new Uint8Array(101); | |||
There was a problem hiding this comment.
Can you do str.length there instead maybe?
|
Node 17 is EOL (and has been for almost 2 years). We shouldn't go out of our way to support it. But, these changes are very simple, so I guess why not. |
We support versions of node much older than 17. Our support goes as far back as far as 10.19.0! |
The previous
fs.writeSyncAPI was not available in Node.js 17.x, see:https://nodejs.org/api/fs.html#fswritesyncfd-buffer-options
Also, do the same for
fs.readSyncto ensure compat with Node.js < v13.13.0.Resolves #21118.
Noticed this while trying to update wasm-vips minimum required Node.js version to 17.2.0 (commit kleisauke/wasm-vips@9fc6338 - in preparation for llvm/llvm-project#80923).