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: expose realpath(3) bindings #15776

Merged
merged 1 commit into from Nov 9, 2017

Conversation

@bnoordhuis
Member

bnoordhuis commented Oct 5, 2017

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.

cleanup();
}
setup();
common.refreshTmpDir();

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 5, 2017

Member

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

bnoordhuis Oct 5, 2017

Member

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.

@cjihrig

cjihrig approved these changes Oct 5, 2017

@bricss bricss referenced this pull request Oct 5, 2017

Merged

v8.7.0 proposal #15762

@trevnorris

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Oct 12, 2017

Member

I'm happy with this approach.

Member

jasnell commented Oct 12, 2017

I'm happy with this approach.

@jasnell

Lgtm but needs docs

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Nov 7, 2017

Member

I wrought documentation, PTAL.

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

Member

bnoordhuis commented Nov 7, 2017

I wrought documentation, PTAL.

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

@jasnell

jasnell approved these changes Nov 8, 2017

LGTM with docs. Thanks @bnoordhuis

fs: expose realpath(3) bindings
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>

@bnoordhuis bnoordhuis closed this Nov 9, 2017

@bnoordhuis bnoordhuis deleted the bnoordhuis:fix8715 branch Nov 9, 2017

@bnoordhuis bnoordhuis merged commit 74023c0 into nodejs:master Nov 9, 2017

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Nov 9, 2017

Member

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

Member

bnoordhuis commented Nov 9, 2017

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

evanlucas added a commit that referenced this pull request Nov 13, 2017

fs: expose realpath(3) bindings
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 added a commit that referenced this pull request Nov 13, 2017

2017-11-14, Version 9.2.0 (Current)
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

@evanlucas evanlucas referenced this pull request Nov 13, 2017

Merged

v9.2.0 proposal #16992

evanlucas added a commit that referenced this pull request Nov 13, 2017

2017-11-14, Version 9.2.0 (Current)
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

PR-URL: #16992

evanlucas added a commit that referenced this pull request Nov 13, 2017

2017-11-14, Version 9.2.0 (Current)
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)
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 14, 2017

2017-11-14, Version 9.2.0 (Current)
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 14, 2017

2017-11-14, Version 9.2.0 (Current)
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
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Nov 15, 2017

Member

Where are the docs? Ctrl+Fing https://nodejs.org/api/fs.html for "native" turns up nothing.

Member

domenic commented Nov 15, 2017

Where are the docs? Ctrl+Fing https://nodejs.org/api/fs.html for "native" turns up nothing.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax
Member

addaleax commented Nov 15, 2017

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Nov 15, 2017

doc: document fs.realpath.native()
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.

Refs: nodejs#15776

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Nov 16, 2017

doc: document fs.realpath.native()
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

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Nov 16, 2017

Member

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

Member

bnoordhuis commented Nov 16, 2017

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

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Dec 6, 2017

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 facebook/jest#5030, but also on jest's own code base.

Member

SimenB commented Dec 6, 2017

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 facebook/jest#5030, but also on jest's own code base.

@SimenB SimenB referenced this pull request Dec 6, 2017

Closed

nodejs 9.2 failure #5030

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Dec 6, 2017

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

Member

SimenB commented Dec 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Dec 6, 2017

Contributor

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

Contributor

cjihrig commented Dec 6, 2017

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

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Dec 6, 2017

Member

It is indeed. PR on the way.

EDIT: facebook/jest#5031

Member

SimenB commented Dec 6, 2017

It is indeed. PR on the way.

EDIT: facebook/jest#5031

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Dec 6, 2017

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()

Member

jasnell commented Dec 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Dec 6, 2017

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 (facebook/jest#4730). PR with "fix": facebook/jest#5031.

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

Member

SimenB commented Dec 6, 2017

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 (facebook/jest#4730). PR with "fix": facebook/jest#5031.

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

MylesBorins added a commit that referenced this pull request Dec 11, 2017

doc: document fs.realpath.native()
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

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Jan 15, 2018

Member

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

Member

gibfahn commented Jan 15, 2018

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

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

fs: expose realpath(3) bindings
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>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

2017-11-14, Version 9.2.0 (Current)
Notable changes:

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

PR-URL: nodejs#16992

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

doc: document fs.realpath.native()
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>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Aug 2, 2018

Member

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

Member

MylesBorins commented Aug 2, 2018

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

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Aug 17, 2018

Member

quick ping to ensure no one wants to see this backported

Member

MylesBorins commented Aug 17, 2018

quick ping to ensure no one wants to see this backported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment