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

fs: add statfs to fs #31351

Closed
wants to merge 11 commits into from
Closed

fs: add statfs to fs #31351

wants to merge 11 commits into from

Conversation

SheikhSajid
Copy link
Contributor

Add support for statfs system call to the fs module using
uv_fs_statfs() function from libuv.

Refs: #10745

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 14, 2020
doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
lib/fs.js Outdated Show resolved Hide resolved
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thanks, nice work!

This looks good with the spare field(s) removed.

doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Show resolved Hide resolved
doc/api/fs.md Show resolved Hide resolved
doc/api/fs.md Show resolved Hide resolved
src/node_file.cc Outdated Show resolved Hide resolved
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM but as mentioned, all references to the spare fields can be dropped.

src/node_file-inl.h Outdated Show resolved Hide resolved
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with all of the spares removed.

test/parallel/test-fs-promises.js Show resolved Hide resolved
test/parallel/test-fs-statfs.js Outdated Show resolved Hide resolved
lib/fs.js Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 15, 2020
@Trott
Copy link
Member

Trott commented Jan 16, 2020

@SheikhSajid I only meant to alphabetize the documentation headers/entries, not the order of properties in the object output in the console. Sorry that I wasn't clear about that!

In other words:

### foo

should appear after:

### bar

but this is OK, and should be retained if it's what the order of the actual output will be:

```console
{
  foo: 'first',
  bar: 'second'
}
```

@SheikhSajid
Copy link
Contributor Author

@SheikhSajid I only meant to alphabetize the documentation headers/entries, not the order of properties in the object output in the console. Sorry that I wasn't clear about that!

In other words:

### foo

should appear after:

### bar

but this is OK, and should be retained if it's what the order of the actual output will be:

```console
{
  foo: 'first',
  bar: 'second'
}

Fixed

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 17, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

I'm sorry to report that the test for this new feature fails on Windows in our CI:

13:24:34 not ok 190 parallel/test-fs-statfs
13:24:34   ---
13:24:34   duration_ms: 0.198
13:24:34   severity: fail
13:24:34   exitcode: 1
13:24:34   stack: |-
13:24:34     internal/fs/utils.js:259
13:24:34         throw err;
13:24:34         ^
13:24:34     
13:24:34     Error: ENOENT: no such file or directory, statfs 'C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-fs-statfs.js'
13:24:34         at Object.statfsSync (fs.js:976:3)
13:24:34         at Object.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-fs-statfs.js:39:24)
13:24:34         at Module._compile (internal/modules/cjs/loader.js:1208:30)
13:24:34         at Object.Module._extensions..js (internal/modules/cjs/loader.js:1228:10)
13:24:34         at Module.load (internal/modules/cjs/loader.js:1057:32)
13:24:34         at Function.Module._load (internal/modules/cjs/loader.js:952:14)
13:24:34         at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
13:24:34         at internal/main/run_main_module.js:17:47 {
13:24:34       errno: -4058,
13:24:34       syscall: 'statfs',
13:24:34       code: 'ENOENT',
13:24:34       path: 'C:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\parallel\\test-fs-statfs.js'
13:24:34     }
13:24:34   ...

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2020
@BridgeAR
Copy link
Member

Ping @SheikhSajid

@SheikhSajid
Copy link
Contributor Author

SheikhSajid commented Jan 22, 2020

Seems like a libuv issue on Windows. I compiled libuv on Windows and uv_fs_statfs is returning errors whenever a path to a file is passed to it. Folders work fine.

@SheikhSajid
Copy link
Contributor Author

I will try to look into why this is happening.

@nodejs nodejs locked as spam and limited conversation to collaborators Feb 19, 2020
@nodejs nodejs unlocked this conversation Feb 19, 2020
@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jun 25, 2020
@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

@SheikhSajid ... looks like this has stalled out and needs a rebase. Do you intend on moving this forward?

@aduh95 aduh95 added stalled Issues and PRs that are stalled. and removed stalled Issues and PRs that are stalled. labels Oct 19, 2020
@github-actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@github-actions
Copy link
Contributor

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

@github-actions github-actions bot closed this Nov 19, 2020
@tiejunhu
Copy link

it looks like everything is ready, should anyone make this happen will be super.

cjihrig added a commit to cjihrig/node that referenced this pull request Jan 26, 2023
This commit adds statfs() and statfsSync() to the fs module, and
statfs() to the fsPromises module.

Co-authored-by: cjihrig <cjihrig@gmail.com>
Fixes: nodejs#10745
Refs: nodejs#31351
cjihrig added a commit to cjihrig/node that referenced this pull request Jan 26, 2023
This commit adds statfs() and statfsSync() to the fs module, and
statfs() to the fsPromises module.

Co-authored-by: cjihrig <cjihrig@gmail.com>
Fixes: nodejs#10745
Refs: nodejs#31351
nodejs-github-bot pushed a commit that referenced this pull request Jan 29, 2023
This commit adds statfs() and statfsSync() to the fs module, and
statfs() to the fsPromises module.

Co-authored-by: cjihrig <cjihrig@gmail.com>
Fixes: #10745
Refs: #31351
PR-URL: #46358
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
This commit adds statfs() and statfsSync() to the fs module, and
statfs() to the fsPromises module.

Co-authored-by: cjihrig <cjihrig@gmail.com>
Fixes: #10745
Refs: #31351
PR-URL: #46358
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
This commit adds statfs() and statfsSync() to the fs module, and
statfs() to the fsPromises module.

Co-authored-by: cjihrig <cjihrig@gmail.com>
Fixes: #10745
Refs: #31351
PR-URL: #46358
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
This commit adds statfs() and statfsSync() to the fs module, and
statfs() to the fsPromises module.

Co-authored-by: cjihrig <cjihrig@gmail.com>
Fixes: #10745
Refs: #31351
PR-URL: #46358
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
This commit adds statfs() and statfsSync() to the fs module, and
statfs() to the fsPromises module.

Co-authored-by: cjihrig <cjihrig@gmail.com>
Fixes: #10745
Refs: #31351
PR-URL: #46358
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.