FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: expose realpath(3) bindings by bnoordhuis · Pull Request #15776 · nodejs/node · GitHub

/ node Public

fs: expose realpath(3) bindings - #15776

Merged
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:fix8715
Nov 9, 2017
ConversationCommits (1)ChecksFiles changed
Merged

fs: expose realpath(3) bindings#15776
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:fix8715

Conversation

bnoordhuis commented Oct 5, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Make the uv_fs_realpath() binding (which calls the libc realpath()
on UNIX and GetFinalPathNameByHandle() on Windows) available as the
fs.realpath.native() and fs.realpathSync.native() functions.

The binding was already available as process.binding('fs').realpath
but was not exposed or tested - and partly broken as a result.

Fixes: #8715
Refs: #7899
CI: https://ci.nodejs.org/job/node-test-pull-request/10411/

If we don't go down this, ah, path, we should just remove the method from src/node_file.cc.

edit: forgot to mention: no documentation just yet because that would be wasted effort if everyone hates this.

bnoordhuis added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 5, 2017
nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Oct 5, 2017
bnoordhuis commented Oct 5, 2017
View reviewed changes
Comment thread test/parallel/test-fs-realpath.js Outdated

bnoordhuis Oct 5, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It gets somewhat lost in the noise but this is a bug fix: the fs.rmdirSync() logic didn't work because a/ contains more than just a/b. It didn't result in actual failures because the test runner cleans it up afterwards.

bnoordhuis mentioned this pull request Oct 5, 2017
Closed
bricss mentioned this pull request Oct 5, 2017
Merged
trevnorris approved these changes Oct 12, 2017
View reviewed changes

trevnorris left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Think this is worth documenting. With this addition I imagine that if there are other native fs functions people want they'll request something similar (though I'm not sure what those would be).

jasnell commented Oct 12, 2017

Copy link
Copy Markdown
Member

I'm happy with this approach.

jasnell approved these changes Oct 12, 2017
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Lgtm but needs docs

peterdanis mentioned this pull request Oct 20, 2017
Merged
SimenB mentioned this pull request Oct 22, 2017
Closed

bnoordhuis commented Nov 7, 2017

Copy link
Copy Markdown
Member Author

I wrought documentation, PTAL.

Rebase + new CI: https://ci.nodejs.org/job/node-test-pull-request/11282/

jasnell approved these changes Nov 8, 2017
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with docs. Thanks @bnoordhuis

fs: expose realpath(3) bindings
74023c0 
Make the `uv_fs_realpath()` binding (which calls the libc `realpath()`
on UNIX and `GetFinalPathNameByHandle()` on Windows) available as the
`fs.realpath.native()` and `fs.realpathSync.native()` functions.

The binding was already available as `process.binding('fs').realpath`
but was not exposed or tested - and partly broken as a result.

Fixes: nodejs#8715
PR-URL: nodejs#15776
Refs: nodejs#7899
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
bnoordhuis closed this Nov 9, 2017
bnoordhuis deleted the fix8715 branch November 9, 2017 10:45
bnoordhuis merged commit 74023c0 into nodejs:master Nov 9, 2017

bnoordhuis commented Nov 9, 2017

Copy link
Copy Markdown
Member Author

Landed in 74023c0. The one CI failure was a known problematic test on AIX.

evanlucas pushed a commit that referenced this pull request Nov 13, 2017
fs: expose realpath(3) bindings
8611e3b 
Make the `uv_fs_realpath()` binding (which calls the libc `realpath()`
on UNIX and `GetFinalPathNameByHandle()` on Windows) available as the
`fs.realpath.native()` and `fs.realpathSync.native()` functions.

The binding was already available as `process.binding('fs').realpath`
but was not exposed or tested - and partly broken as a result.

Fixes: #8715
PR-URL: #15776
Refs: #7899
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
evanlucas mentioned this pull request Nov 13, 2017
Merged
evanlucas added a commit that referenced this pull request Nov 13, 2017
2017-11-14, Version 9.2.0 (Current)
2f07e1c 
Notable changes:

* **crypto**:
  - Support building with both 1.1.0 and 1.0.2 (David Benjamin) #16130
* **fs**:
  - fs.realpathSync.native and fs.realpath.native are now exposed (Ben Noordhuis) #15776
* **process**:
  - expose process.ppid (cjihrig) #16839

PR-URL: #16992
evanlucas added a commit that referenced this pull request Nov 13, 2017
2017-11-14, Version 9.2.0 (Current)
e3090b8 
Notable changes:

* **crypto**:
  - Support building with both 1.1.0 and 1.0.2 (David Benjamin) #16130
* **fs**:
  - fs.realpathSync.native and fs.realpath.native are now exposed (Ben Noordhuis) #15776
* **process**:
  - expose process.ppid (cjihrig) #16839

PR-URL: #16992
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Nov 16, 2017
doc: document fs.realpath.native()
cb137eb 
Mea culpa, somehow I managed to drop the documentation commit while
merging the pull request.  This should have been included in commit
74023c0 ("fs: expose realpath(3) bindings") from this month.

PR-URL: nodejs#17059
Refs: nodejs#15776
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

bnoordhuis commented Nov 16, 2017

Copy link
Copy Markdown
Member Author

Back-porters, this should land together with the documentation from #17059.

SimenB commented Dec 6, 2017

Copy link
Copy Markdown
Member

Hiya, this completely breaks Jest with the following trace:

../src/node_file.cc:835:void node::RealPath(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `(args.Length()) >= (2)' failed.
 1: node::Abort() [node]
 2: node::Assert(char const* const (*) [4]) [node]
 3: 0x5595756fed3d [node]
 4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [node]
 5: 0x5595750292df [node]
 6: 0x55957502982f [node]
 7: 0x2e88c60842fd

Should I create a new issue?

Simple repro can be seen in issue jestjs/jest#5030, but also on jest's own code base.

SimenB mentioned this pull request Dec 6, 2017
Closed

SimenB commented Dec 6, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Completely minimal reproduction is: node -p "process.binding('fs').realpath(process.cwd())". Works on 9.1, aborts on 9.2. Adding 'utf8' as second parameter makes it work

cjihrig commented Dec 6, 2017

Copy link
Copy Markdown
Contributor

Is Jest using process.binding() directly? If so, it really shouldn't be.

SimenB commented Dec 6, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

It is indeed. PR on the way.

EDIT: jestjs/jest#5031

SimenB mentioned this pull request Dec 6, 2017
Merged

jasnell commented Dec 6, 2017

Copy link
Copy Markdown
Member

Looks like jest also has graceful-fs in it's dependency tree.

graceful-fs is known to be problematic due to it's insistence on using process.binding()

SimenB commented Dec 6, 2017

Copy link
Copy Markdown
Member

It only happens on the beta releases of jest, not on the version marked latest. Its usage was added recently to fix case sensitivity on Windows (jestjs/jest#4730). PR with "fix": jestjs/jest#5031.

Would be ideal to use the function added in this PR, but we still have to support older versions of node.

MylesBorins pushed a commit that referenced this pull request Dec 11, 2017
doc: document fs.realpath.native()
7329537 
Mea culpa, somehow I managed to drop the documentation commit while
merging the pull request.  This should have been included in commit
74023c0 ("fs: expose realpath(3) bindings") from this month.

PR-URL: #17059
Refs: #15776
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

gibfahn commented Jan 15, 2018

Copy link
Copy Markdown
Member

Release team were -1 on landing on 6.x, but +1 on reconsidering for the next 8.x minor.

gibfahn added the baking-for-lts PRs that need to wait before landing in a LTS release. label Jan 15, 2018

MylesBorins commented Aug 2, 2018

Copy link
Copy Markdown
Contributor

Choosing not to land this on 8.x right now as it had earlier broken jest and could cause similar unexpected breakages. Someone should feel open to change labels / open a PR if they feel otherwise

MylesBorins removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v8.x labels Aug 17, 2018

MylesBorins commented Aug 17, 2018

Copy link
Copy Markdown
Contributor

quick ping to ensure no one wants to see this backported

coderabbitai Bot mentioned this pull request Jun 23, 2026
Merged
coderabbitai Bot mentioned this pull request Jul 20, 2026
Merged
coderabbitai Bot mentioned this pull request Jul 28, 2026
Merged
This was referenced Aug 14, 2026
Merged
Closed
Merged
coderabbitai Bot mentioned this pull request Aug 25, 2026
Open
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
JungMinu JungMinu approved these changes
+2 more reviewers
trevnorris trevnorris approved these changes
Fishrock123 Fishrock123 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL