Skip to content
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.

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

[nodefs] Remove compatibility code for ancient node versions #23316

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
54 changes: 53 additions & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -766,14 +766,66 @@ jobs:
- run-tests:
title: "node (oldest / 10.19.0)"
extra-cflags: "-sMIN_NODE_VERSION=101900"
# We include most but not all of the nodefs and node rawfs tests here.
# test_fs_nodefs_rw, test_fs_nodefs_statvfs, and test_unistd_io_nodefs_bigint fail.
test_targets: "
other.test_gen_struct_info
other.test_native_call_before_init
other.test_js_optimizer_verbose
other.test_node_unhandled_rejection
other.test_full_js_library*
core2.test_hello_world
core2.test_fs_write_rawfs"
core2.test_fcntl_open_nodefs
core2.test_fcntl_open_rawfs
core2.test_fgetc_ungetc_nodefs
core2.test_fgetc_ungetc_rawfs
core2.test_fs_append_rawfs
core2.test_fs_emptyPath_rawfs
core2.test_fs_enotdir_nodefs
core2.test_fs_enotdir_rawfs
core2.test_fs_errorstack_rawfs
core2.test_fs_llseek_rawfs
core2.test_fs_mkdir_dotdot_nodefs
core2.test_fs_mkdir_dotdot_rawfs
core2.test_fs_mmap_nodefs
core2.test_fs_mmap_rawfs
core2.test_fs_nodefs_cloexec
core2.test_fs_nodefs_cloexec_rawfs
core2.test_fs_nodefs_dup
core2.test_fs_nodefs_dup_rawfs
core2.test_fs_nodefs_home
core2.test_fs_nodefs_nofollow
core2.test_fs_nodefs_readdir
core2.test_fs_noderawfs_nofollow
core2.test_fs_readdir_ino_matches_stat_ino_nodefs
core2.test_fs_readdir_ino_matches_stat_ino_rawfs
core2.test_fs_readv_rawfs
core2.test_fs_rename_on_existing_nodefs
core2.test_fs_rename_on_existing_rawfs
core2.test_fs_symlink_resolution_nodefs
core2.test_fs_symlink_resolution_rawfs
core2.test_fs_writeFile_rawfs
core2.test_fs_writeFile_wasmfs_rawfs
core2.test_fs_write_rawfs
core2.test_fs_writev_rawfs
core2.test_futimens_rawfs
core2.test_readdir_rawfs
core2.test_stat_chmod_rawfs
core2.test_unistd_access_nodefs
core2.test_unistd_access_rawfs
core2.test_unistd_close_rawfs
core2.test_unistd_dup_rawfs
core2.test_unistd_io_nodefs
core2.test_unistd_links_nodefs
core2.test_unistd_misc_nodefs
core2.test_unistd_pipe_rawfs
core2.test_unistd_symlink_on_nodefs
core2.test_unistd_truncate_nodefs
core2.test_unistd_truncate_rawfs
core2.test_unistd_unlink_nodefs
core2.test_unistd_unlink_rawfs
core2.test_unistd_write_broken_link_rawfs
"
# Run a few test with the most recent version of node
# In particular we have some tests that require node flags on older
# versions of node but not with the most recent version.
Expand Down
5 changes: 5 additions & 0 deletions src/closure-externs/node-externs.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,11 @@ fs.Stats.prototype.mtimeMs;
*/
fs.Stats.prototype.ctimeMs;

/**
* @type {number}
*/
fs.Stats.prototype.blksize;

/**
* @param {string} p
* @return {boolean}
Expand Down
17 changes: 1 addition & 16 deletions src/library_nodefs.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,7 @@ addToLibrary({
isWindows: false,
staticInit() {
NODEFS.isWindows = !!process.platform.match(/^win/);
var flags = process.binding("constants");
// Node.js 4 compatibility: it has no namespaces for constants
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.

NODEFS.flagsForNodeMap = {
"{{{ cDefs.O_APPEND }}}": flags["O_APPEND"],
"{{{ cDefs.O_CREAT }}}": flags["O_CREAT"],
Expand Down Expand Up @@ -123,15 +119,6 @@ addToLibrary({
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?

// them with default blksize of 4096.
// See http://support.microsoft.com/kb/140365
if (!stat.blksize) {
stat.blksize = 4096;
}
if (!stat.blocks) {
stat.blocks = (stat.size+stat.blksize-1)/stat.blksize|0;
}
// Windows does not report the 'x' permission bit, so propagate read
// bits to execute bits.
stat.mode |= (stat.mode & {{{ cDefs.S_IRUGO }}}) >> 2;
Expand Down Expand Up @@ -261,8 +248,6 @@ addToLibrary({
stream.shared.refcount++;
},
read(stream, buffer, offset, length, position) {
// Node.js < 6 compatibility: node errors on 0 length reads
if (length === 0) return 0;
return NODEFS.tryFSOperation(() =>
fs.readSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), 0, length, position)
);
Expand Down
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_ctors1.jssize
Original file line number Diff line number Diff line change
@@ -1 +1 @@
20271
20282
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_ctors2.gzsize
Original file line number Diff line number Diff line change
@@ -1 +1 @@
8325
8326
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_ctors2.jssize
Original file line number Diff line number Diff line change
@@ -1 +1 @@
20239
20250
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_except.jssize
Original file line number Diff line number Diff line change
@@ -1 +1 @@
24039
24050
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_except_wasm.gzsize
Original file line number Diff line number Diff line change
@@ -1 +1 @@
8288
8291
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_except_wasm.jssize
Original file line number Diff line number Diff line change
@@ -1 +1 @@
20164
20175
Original file line number Diff line number Diff line change
@@ -1 +1 @@
8288
8291
Original file line number Diff line number Diff line change
@@ -1 +1 @@
20164
20175
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_lto.gzsize
Original file line number Diff line number Diff line change
@@ -1 +1 @@
8357
8356
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_lto.jssize
Original file line number Diff line number Diff line change
@@ -1 +1 @@
20347
20357
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_mangle.jssize
Original file line number Diff line number Diff line change
@@ -1 +1 @@
24039
24050
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_noexcept.jssize
Original file line number Diff line number Diff line change
@@ -1 +1 @@
20271
20282
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_files_js_fs.jssize
Original file line number Diff line number Diff line change
@@ -1 +1 @@
18817
18828
Loading