fs: restore javascript realpath implementation #7899

bzoz · 2016-07-27T18:12:17Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

fs

Description of change

Libuv realpath implementation brought a lot of problems described here #7726. This restores old JS implementation of realpath and realpathSync, adding new API, with options in place of cache.

This requires #7846 to land, as it reverts changes to fs on which old JS implementation depended. This PR is about 3rd and 4th commit which bring back old realpath implementation with support for the new API.

First two commits are from #7846, reverting some changes to fs that landed after JS realpath implementation was dropped.

Third commit reverts b488b19 bringing back JS implementation of realpath and realpathSync.

Fourth commit updates old implementation. It removes the cache and implements new realpath API (support for encoding parameter).

This cannot land before #7846. In case #7846 does not make it, this can be easily updated.

addaleax · 2016-07-27T19:41:40Z

A couple of suggestions:

I don’t think the native binding to the libuv realpath needs to be removed. It can be, but it might be nice to retain it for comparing results with the JS implementation later.
You could add the benchmark/test commit from @trevnorris’ fs: fix long symlinks in realpath #7548 here.
I’ll try to benchmark it, but removing the cache altogether sounds like something that won’t work out. I realize that it conflicts with the encoding: option, but that seems to be easily worked around by never adding an encoding: key to the cache? It also won’t be a likely problem in practice, because only path.resolve()d keys are added to the cache. People in the CTC meeting made some good points and removing the cache sounds okay.
The encoding option should probably be used for all fs.* calls that are made from inside the realpath implementations?

orangemocha · 2016-07-27T20:36:35Z

CI run: https://ci.nodejs.org/job/node-test-pull-request/3441/

orangemocha · 2016-07-27T20:50:42Z

At the last meeting, the CTC has expressed consensus with the approach in this PR: reverting, leaving the cache out, but keeping the encoding option.

ChALkeR · 2016-07-27T20:53:20Z

To me it looks like this reverts too many tests. Could we keep the ones that still pass?

bzoz · 2016-07-28T11:06:47Z

@addaleax :

I think that since we are not using native realpath anymore there is no need to keep bindings around.
Adding benchmarks from fs: fix long symlinks in realpath #7548 is a good idea, I'll do that.
Inside we are using path.resolve, which expects utf-8 strings. So we stick to them when calling other fs functions, and only apply encoding option when returning result.

@ChALkeR: Those test are reverted by #7846 on which this PR depends.

addaleax · 2016-07-28T11:36:03Z

I think that since we are not using native realpath anymore there is no need to keep bindings around.

@bzoz Your call.There seems to be agreement that the long-term plan is to use a fixed libuv implementation, though, and I think keeping the binding around might be useful for e.g. comparative benchmarks.

we are using path.resolve, which expects utf-8 strings

path.resolve expects strings, which means that it it isn’t concerned with encodings at all. But you’re right, this is not as easy as I though it would be, and it’s probably best to just stick with UTF-8 for all intermediate results (and accepting that that means that symlinks encountered along the way must be valid UTF-8).

jasnell · 2016-07-28T16:58:49Z

In general this LGTM with a green CI. I definitely want to make sure this gets additional eyes on it to review, however. @nodejs/ctc

StefanScherer · 2016-07-28T20:37:32Z

@bzoz I've just compiled your PR on Windows and tried the node.exe inside a Windows Container. Installed Node.js 6.3.1 from MSI inside the container and just replaced node.exe.

Running npm init, npm install express ... works fine again. 🐳
So this PR would fix #7044

saghul · 2016-07-29T10:52:58Z

Why does this PR also revert "fs: validate args of truncate functions in js" and "fs: make callback mandatory to all async functions" ? How are those related to the realpath problems? It makes is a lot harder to review this PR.

saghul · 2016-07-29T10:55:19Z

Those seem to be handled by #7846, btw.

addaleax · 2016-07-29T11:16:41Z

@saghul This PR would otherwise create merge conflicts, that’s all; the intent here is to wait until #7846 is landed, if I understood @bzoz correctly.

addaleax · 2016-07-29T11:22:44Z

test/parallel/test-fs-realpath-on-substed-drive.js

+  assert(!err);
+  assert.equal(typeof result, 'string');
+  assert.equal(result, filename);
+});


You don’t need the asyncCompleted checks; wrapping the callback as common.mustCall(function(err, result) { … }) handles that automatically for you.

addaleax · 2016-07-29T11:53:44Z

@bzoz Many, many thanks for starting to add regression tests! Overall LGTM, too.

bzoz · 2016-07-29T13:18:15Z

CI run: https://ci.nodejs.org/job/node-test-pull-request/3453/

addaleax · 2016-08-02T13:38:21Z

LGTM, thanks for doing the work behind this PR!

CI: https://ci.nodejs.org/job/node-test-commit/4374/

addaleax · 2016-08-02T14:52:31Z

One Windows build bot failure, running CI again to be safe: https://ci.nodejs.org/job/node-test-commit/4376/

bzoz · 2016-08-02T14:56:47Z

I had one running here https://ci.nodejs.org/job/node-test-commit/4373/, it is green.

addaleax · 2016-08-02T15:02:02Z

Oh, sorry (but good to hear)! I’d say this is good to go once #7846 is resolved.

misterdjules · 2016-08-02T18:28:41Z

@bzoz What do you think of squashing all commits but the ones from #7846 to make reviewing changes easier? Currently one has to go through all these commits separately in order to focus on reviewing the changes that are actually related to this PR, which is a bit tedious.

bzoz · 2016-08-03T09:10:54Z

@misterdjules OK, squashed

Commits:

Restore old js realpath implementation
Implement new API (i.e. options.encoding) and add a test
Add tests and benchmarks from fs: fix long symlinks in realpath #7548
Remove one test from fs: fix long symlinks in realpath #7548 which was failing

saghul · 2016-08-03T11:24:05Z

What was the result of the discussion about the substed realpath behavior thing? This PR adds a test thus making certain behavior "correct" which might not necessarily be the case. It depends on how substed drives are interpreted, IMHO.

On the maze of issues and notes I recall reading that some people where using substed drives to get shorter paths. Was that the case? What breaks if we would give them the long path?

bzoz · 2016-08-10T13:51:08Z

I've added a note to the fs documentation.

jasnell · 2016-08-10T14:17:45Z

LGTM, thank you.

jasnell · 2016-08-10T14:19:30Z

CI is green. Let's give it another day before landing. @nodejs/ctc @nodejs/collaborators ... please take one final look.

saghul · 2016-08-10T15:18:40Z

lib/fs.js

@@ -1563,38 +1563,239 @@ fs.unwatchFile = function(filename, listener) {
 };


-fs.realpathSync = function realpathSync(path, options) {
+// Regexp that finds the next partion of a (partial) path


typo: partion

This reverts parts of b488b19 restoring javascript implementation of realpath and realpathSync. Fixes: #7175 Fixes: #6861 Fixes: #7294 Fixes: #7192 Fixes: #7044 Fixes: #6624 Fixes: #6978 PR-URL: #7899 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

The benchmarks included also work for the previous JS implementation of fs.realpath(). In case the new implementation of realpath() needs to be reverted, we want these changes to stick around. PR-URL: #7899 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

bzoz · 2016-08-12T11:16:01Z

Landed in 08996fd and b9832eb

This reverts parts of b488b19 restoring javascript implementation of realpath and realpathSync. Fixes: #7175 Fixes: #6861 Fixes: #7294 Fixes: #7192 Fixes: #7044 Fixes: #6624 Fixes: #6978 PR-URL: #7899 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

The benchmarks included also work for the previous JS implementation of fs.realpath(). In case the new implementation of realpath() needs to be reverted, we want these changes to stick around. PR-URL: #7899 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

bpasero · 2016-09-22T05:53:24Z

@bzoz @addaleax one thing I really liked from the behaviour of native fs.realpath is that it would return the actual casing of a path as it exists on disk. E.g. if there is an actual path /foo/bar/some.txt, running fs.realpath("/foo/BAR/some.txt") would return /foo/bar/some.txt. Are there any plans to support this behaviour after going back to the old JS implementation? Or any plans to provide this functionality in a separate method?

addaleax · 2016-09-22T12:16:14Z

@bpasero I guess you can open a separate issue for this? It sounds like something that would be okay to have.

bpasero · 2016-09-22T12:50:05Z

@addaleax makes sense, opened #8715

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>

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>

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 Jul 27, 2016

orangemocha mentioned this pull request Jul 27, 2016

[meta] realpath issues in v6 #7726

Closed

7 tasks

bzoz force-pushed the bartek-restore-js-realpath branch from bddaab2 to 0329b6a Compare July 28, 2016 16:27

addaleax reviewed Jul 29, 2016
View reviewed changes

addaleax mentioned this pull request Aug 2, 2016

Revert fs changes #7846

Closed

2 tasks

bzoz force-pushed the bartek-restore-js-realpath branch from 28efdae to 0c0a6b4 Compare August 3, 2016 09:08

bzoz mentioned this pull request Aug 10, 2016

win, fs: fix realpath behavior on substed drives #7559

Closed

3 tasks

Add note about buffer parameters to documentation

94b4214

saghul reviewed Aug 10, 2016
View reviewed changes

fixup: spelling

1ed6028

bzoz closed this Aug 12, 2016

cjihrig mentioned this pull request Aug 15, 2016

v6.4.0 Proposal #8070

Merged

vadimi mentioned this pull request Aug 16, 2016

Case sensitive comparison issue on Windows, node 6.3 WearyMonkey/ngtemplate-loader#49

Closed

vsemozhetbyt mentioned this pull request Aug 20, 2016

(reading files issue) ENOENT: no such file or dicrectory - fs:js.640 electron/electron#6911

Closed

MylesBorins added the dont-land-on-v4.x label Sep 9, 2016

bpasero mentioned this pull request Sep 22, 2016

Provide a way to call the native realpath function #8715

Closed

damieng mentioned this pull request Nov 15, 2016

Network mapped paths resolve differently in Atom 1.12 atom/atom#13104

Closed

6 tasks

bnoordhuis mentioned this pull request Oct 5, 2017

fs: expose realpath(3) bindings #15776

Merged

bjcairns mentioned this pull request Aug 5, 2019

realize_ does not work if the path points toward a network drive r-lib/fs#147

Open

bluwy mentioned this pull request Nov 16, 2022

fix: reduce number of fs.realpath calls vitejs/vite#6412

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: restore javascript realpath implementation #7899

fs: restore javascript realpath implementation #7899

bzoz commented Jul 27, 2016

addaleax commented Jul 27, 2016 •

edited

Loading

orangemocha commented Jul 27, 2016

orangemocha commented Jul 27, 2016

ChALkeR commented Jul 27, 2016 •

edited

Loading

bzoz commented Jul 28, 2016

addaleax commented Jul 28, 2016

jasnell commented Jul 28, 2016

StefanScherer commented Jul 28, 2016 •

edited

Loading

saghul commented Jul 29, 2016

saghul commented Jul 29, 2016

addaleax commented Jul 29, 2016

addaleax Jul 29, 2016

addaleax commented Jul 29, 2016

bzoz commented Jul 29, 2016

addaleax commented Aug 2, 2016

addaleax commented Aug 2, 2016

bzoz commented Aug 2, 2016

addaleax commented Aug 2, 2016

misterdjules commented Aug 2, 2016

bzoz commented Aug 3, 2016

saghul commented Aug 3, 2016

bzoz commented Aug 10, 2016

jasnell commented Aug 10, 2016

jasnell commented Aug 10, 2016

saghul Aug 10, 2016

bzoz Aug 11, 2016

bzoz commented Aug 12, 2016

bpasero commented Sep 22, 2016 •

edited

Loading

addaleax commented Sep 22, 2016

bpasero commented Sep 22, 2016

fs: restore javascript realpath implementation #7899

fs: restore javascript realpath implementation #7899

Conversation

bzoz commented Jul 27, 2016

Checklist

Affected core subsystem(s)

Description of change

addaleax commented Jul 27, 2016 • edited Loading

orangemocha commented Jul 27, 2016

orangemocha commented Jul 27, 2016

ChALkeR commented Jul 27, 2016 • edited Loading

bzoz commented Jul 28, 2016

addaleax commented Jul 28, 2016

jasnell commented Jul 28, 2016

StefanScherer commented Jul 28, 2016 • edited Loading

saghul commented Jul 29, 2016

saghul commented Jul 29, 2016

addaleax commented Jul 29, 2016

addaleax Jul 29, 2016

Choose a reason for hiding this comment

addaleax commented Jul 29, 2016

bzoz commented Jul 29, 2016

addaleax commented Aug 2, 2016

addaleax commented Aug 2, 2016

bzoz commented Aug 2, 2016

addaleax commented Aug 2, 2016

misterdjules commented Aug 2, 2016

bzoz commented Aug 3, 2016

saghul commented Aug 3, 2016

bzoz commented Aug 10, 2016

jasnell commented Aug 10, 2016

jasnell commented Aug 10, 2016

saghul Aug 10, 2016

Choose a reason for hiding this comment

bzoz Aug 11, 2016

Choose a reason for hiding this comment

bzoz commented Aug 12, 2016

bpasero commented Sep 22, 2016 • edited Loading

addaleax commented Sep 22, 2016

bpasero commented Sep 22, 2016

addaleax commented Jul 27, 2016 •

edited

Loading

ChALkeR commented Jul 27, 2016 •

edited

Loading

StefanScherer commented Jul 28, 2016 •

edited

Loading

bpasero commented Sep 22, 2016 •

edited

Loading