Skip to content

Conversation

@hoodmane
Copy link
Collaborator

@hoodmane hoodmane commented Jan 6, 2025

No description provided.

var stat;
NODEFS.tryFSOperation(() => stat = fs.lstatSync(path));
if (NODEFS.isWindows) {
// node.js v0.10.20 doesn't report blksize and blocks on Windows. Fake
Copy link
Collaborator

Choose a reason for hiding this comment

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

We do still support MIN_NODE_VERSION=101900 (10.19.0) so if we want to drop this I guess we would need to bump that minimum too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh wait, thats is 0.10.20!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add some stat tests to test-node-compat in .circleci/config.yml?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How's 6f92152 look?

@sbc100
Copy link
Collaborator

sbc100 commented Jan 6, 2025

Can you mention nodefs in the title somehow? Maybe prefix with [nodefs]?

@sbc100 sbc100 closed this Jan 6, 2025
@sbc100 sbc100 reopened this Jan 6, 2025
if (flags["fs"]) {
flags = flags["fs"];
}
var flags = process.binding("constants")["fs"];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was going to suggest single quotes here but it looks like this file users double-quote a lot, so maybe we leave as is.

core2.test_hello_world
core2.test_fs_write_rawfs"
core2.*nodefs*
core2.*rawfs*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thats quite a few tests but maybe thats fine. Can you remove core2.test_fs_write_rawfs above?

And you need to move the closing quote the last line.

@sbc100
Copy link
Collaborator

sbc100 commented Jan 9, 2025

It looks like not all of those tests are actually runnable on the oldest version of node that we support (v10.19.0). Can you perhaps revert the .circleci change and just at one extra test there rather than adding all of them?

We can work on adding all of them in followup PRs perhaps?

@sbc100
Copy link
Collaborator

sbc100 commented Jan 9, 2025

For the closure compiler errors I think you can maybe add blksize to src/closure-externs/node-externs.js?

@hoodmane
Copy link
Collaborator Author

hoodmane commented Jan 9, 2025

I also wonder how long we want to keep supporting EOL node versions. Node v10 has been EOL for about 4 years. But we'd probably need to bump to version 16 to get a big help. Node 14 has only been EOL since April 2023 which might be a bit recent to drop.

@hoodmane hoodmane changed the title Remove compatibility code for ancient node versions [nodefs] Remove compatibility code for ancient node versions Jan 9, 2025
@sbc100
Copy link
Collaborator

sbc100 commented Jan 9, 2025

I also wonder how long we want to keep supporting EOL node versions. Node v10 has been EOL for about 4 years. But we'd probably need to bump to version 16 to get a big help. Node 14 has only been EOL since April 2023 which might be a bit recent to drop.

That is certainly valid discussion and we should consider bumping the min version. Perhaps you could open an issue regarding that?

This is an automatic change generated by tools/maint/rebaseline_tests.py.

The following (13) test expectation files were updated by
running the tests with `--rebaseline`:

```
other/codesize/test_codesize_cxx_ctors1.jssize: 20271 => 20282 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_ctors2.gzsize: 8325 => 8326 [+1 bytes / +0.01%]
other/codesize/test_codesize_cxx_ctors2.jssize: 20239 => 20250 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_except.jssize: 24039 => 24050 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_except_wasm.gzsize: 8288 => 8291 [+3 bytes / +0.04%]
other/codesize/test_codesize_cxx_except_wasm.jssize: 20164 => 20175 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_except_wasm_exnref.gzsize: 8288 => 8291 [+3 bytes / +0.04%]
other/codesize/test_codesize_cxx_except_wasm_exnref.jssize: 20164 => 20175 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_lto.gzsize: 8357 => 8356 [-1 bytes / -0.01%]
other/codesize/test_codesize_cxx_lto.jssize: 20347 => 20357 [+10 bytes / +0.05%]
other/codesize/test_codesize_cxx_mangle.jssize: 24039 => 24050 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_noexcept.jssize: 20271 => 20282 [+11 bytes / +0.05%]
other/codesize/test_codesize_files_js_fs.jssize: 18817 => 18828 [+11 bytes / +0.06%]

Average change: +0.04% (-0.01% - +0.06%)
```
@hoodmane hoodmane force-pushed the windows-nodefs-cleanup branch from b234dee to e93545c Compare January 10, 2025 09:52
This is an automatic change generated by tools/maint/rebaseline_tests.py.

The following (14) test expectation files were updated by
running the tests with `--rebaseline`:

```
other/codesize/test_codesize_cxx_ctors1.jssize: 20272 => 20283 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_ctors2.gzsize: 8334 => 8332 [-2 bytes / -0.02%]
other/codesize/test_codesize_cxx_ctors2.jssize: 20240 => 20251 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_except.jssize: 24040 => 24051 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_except_wasm.gzsize: 8296 => 8298 [+2 bytes / +0.02%]
other/codesize/test_codesize_cxx_except_wasm.jssize: 20165 => 20176 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_except_wasm_exnref.gzsize: 8296 => 8298 [+2 bytes / +0.02%]
other/codesize/test_codesize_cxx_except_wasm_exnref.jssize: 20165 => 20176 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_lto.gzsize: 8364 => 8362 [-2 bytes / -0.02%]
other/codesize/test_codesize_cxx_lto.jssize: 20348 => 20358 [+10 bytes / +0.05%]
other/codesize/test_codesize_cxx_mangle.jssize: 24040 => 24051 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_noexcept.jssize: 20272 => 20283 [+11 bytes / +0.05%]
other/codesize/test_codesize_files_js_fs.gzsize: 7652 => 7653 [+1 bytes / +0.01%]
other/codesize/test_codesize_files_js_fs.jssize: 18819 => 18830 [+11 bytes / +0.06%]

Average change: +0.03% (-0.02% - +0.06%)
```
@hoodmane hoodmane merged commit 5ac384b into emscripten-core:main Jan 12, 2025
29 checks passed
@hoodmane hoodmane deleted the windows-nodefs-cleanup branch January 12, 2025 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants