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

path.posix.relative returns different results for *nix and Windows versions of node #13683

Closed
usergenic opened this issue Jun 14, 2017 · 18 comments · Fixed by #37747
Closed

path.posix.relative returns different results for *nix and Windows versions of node #13683

usergenic opened this issue Jun 14, 2017 · 18 comments · Fixed by #37747
Labels
confirmed-bug Issues with confirmed bugs. path Issues and PRs related to the path subsystem. windows Issues and PRs related to the Windows platform.

Comments

@usergenic
Copy link

  • Version: 8.1.1
  • Platform: Windows Server
  • Subsystem: path.js

This code returns different values depending on whether run on *nix or windows:

path.posix.relative('a/b/c', '../../x');
'../../../..../x' // on windows
'../../../../../x' // on *nix
@mscdex mscdex added path Issues and PRs related to the path subsystem. windows Issues and PRs related to the Windows platform. labels Jun 14, 2017
@vsemozhetbyt
Copy link
Contributor

On Windows, Node.js versions differ:

  // Node.js 4.8.3 (v8 4.5.103.47) x64

../../../.././x

  // Node.js 6.11.0 (v8 5.1.281.102) x64

../../../..../x

  // Node.js 8.1.1 (v8 5.8.283.41) x64

../../../..../x

@gibfahn gibfahn added the confirmed-bug Issues with confirmed bugs. label Jun 14, 2017
@DuanPengfei
Copy link
Contributor

I will try to fix it.

@DuanPengfei
Copy link
Contributor

DuanPengfei commented Jun 16, 2017

After reading the code found path.posix.relative call posix.resolve first convert path into an absolute path, but posix.resolve's all the parameters have not yet generated an absolute path then it will add the current path to generate the absolute path. Because of current path is platform-related, I think the direct use of path.posix.relative on Windows is not very suitable. And in the debugging of this problem I found the doc of path.relative is not strict, so I also have a PR doc: make path.relative stricter. So I have confusion about this issue, @gibfahn @XadillaX have some advices?

@refack
Copy link
Contributor

refack commented Jun 16, 2017

The first solution that comes to mind is adding a check (re #13714): if explicitly running the posix variant of path.posix.relative on a Windows platform, throw a TypeError.
More correct (check resolve): if explicitly running the posix variant of path.posix.resolve on a Windows platform, throw a TypeError

@XadillaX
Copy link
Contributor

@DuanPengfei you may create a new PR to check and throw error in posix.resolve.

@DuanPengfei
Copy link
Contributor

👌🏻I will create a new PR.

@refack refack added the wip Issues and PRs that are still a work in progress. label Jun 17, 2017
@addaleax addaleax removed the wip Issues and PRs that are still a work in progress. label Jun 17, 2017
@addaleax
Copy link
Member

@refack Throwing an error instead of fixing the bug seems like a really bad idea; I think there is no reason inherent to the API why path.posix.relative('a/b/c', '../../x') wouldn’t be able to return a correct result on Windows.

@tniessen
Copy link
Member

I am a little surprised as well, I didn't follow the conversation, but I wonder why we do not even consider fixing the bug instead of just saying "you cannot use this function on Windows". Did I miss something? Is this really too difficult to implement? How is posix.resolve platform-dependent except for process.cwd()?

@XadillaX
Copy link
Contributor

@addaleax @tniessen relative uses cwd to build the missing part, eg.

path.relative("../../../../x", "y")

@addaleax
Copy link
Member

@XadillaX I understand that; but that should work just fine on Windows, no? Also, there’s not always a missing part, for example if you reverse the parameters in your code snippet.

@XadillaX
Copy link
Contributor

if the correct result may be implemented fine, I think to implement is better too.

@DuanPengfei
Copy link
Contributor

If use different systems pwd can produce correct result, of course, that is better, I will try it again later. Create PR because I think use C:\Users\someone as *nix root directory is not reasonable.

@tniessen
Copy link
Member

because I think use C:\Users\someone as *nix root directory is not reasonable

Even if it is not, that is not a reason to prevent Windows applications from dealing with posix paths and vice versa. For example, you might want to talk to a posix FTP server from a Windows machine (in which case paths shouldn't be resolved against cwd anyways), so you would need the posix path API.

@DuanPengfei
Copy link
Contributor

So is that means path.posix.resolve can work well in some cases. When it rely on cwd will generate incorrect path or throw error. This is also an acceptable way of compromise I think.

@refack
Copy link
Contributor

refack commented Jun 17, 2017

I'm not sure which thread is better to comment in, this or #13738 🤔
I suggested there to throw only if resolving requires using process.cwd() since that is a bug.
A solution for this issue could be implemented in parallel.

P.S. The in progress label is used to indicate to other contributors that someone is working on this issue, RE: #13565

@refack refack added the wip Issues and PRs that are still a work in progress. label Jun 17, 2017
@refack
Copy link
Contributor

refack commented Jun 17, 2017

a posix FTP server from a Windows machine

In that case I would recommend using URL. But there are obviously cases where it's useful to do cross-platform path manipulation, but IMHO we should limit the nonsense outputs, like when process.pwd() is necessary.
(reposting):

> path.posix.resolve('./gaga/')
'C:\\bin\\dev\\node/gaga'  // bug
> path.posix.resolve('/gaga/')
'/gaga'   // Ok

DuanPengfei added a commit to souche-koumakan/node that referenced this issue Jun 22, 2017
Throw an error `ERR_UNSUPPORTED_PLATFOMR` when direct use
`path.posix.resolve` on Windows or direct use `path.win32.resolve`
on *nix.

Update docs, list win32 functions do not support direct use on
*nix and posix functions do not support direct use on Windows.

Update tests, only run current platform type test.

Fixes: nodejs#13683
Refs: nodejs#13714
Trott added a commit to Trott/io.js that referenced this issue Mar 13, 2021
Add a known_issues test for a known Windows issue.

Refs: nodejs#13683
Trott added a commit to Trott/io.js that referenced this issue Mar 13, 2021
Trott added a commit to Trott/io.js that referenced this issue Mar 13, 2021
Trott added a commit to Trott/io.js that referenced this issue Mar 14, 2021
Add a known_issues test for a known Windows issue.

Refs: nodejs#13683
Trott added a commit to Trott/io.js that referenced this issue Mar 14, 2021
Trott added a commit to Trott/io.js that referenced this issue Mar 14, 2021
Trott added a commit to Trott/io.js that referenced this issue Mar 14, 2021
Trott added a commit to Trott/io.js that referenced this issue Mar 14, 2021
Trott added a commit to Trott/io.js that referenced this issue Mar 14, 2021
Trott added a commit to Trott/io.js that referenced this issue Mar 14, 2021
Trott added a commit to Trott/io.js that referenced this issue Mar 14, 2021
Add a known_issues test for a known Windows issue.

Refs: nodejs#13683
Trott added a commit to Trott/io.js that referenced this issue Mar 14, 2021
@Trott
Copy link
Member

Trott commented Mar 15, 2021

I know this issue is quite old, but if anyone wants to test out #37747 to see if it fixes the issue for them, that would be great.

Trott added a commit to Trott/io.js that referenced this issue Mar 18, 2021
Add a known_issues test for a known Windows issue.

Refs: nodejs#13683

PR-URL: nodejs#37744
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Trott added a commit to Trott/io.js that referenced this issue Mar 18, 2021
Trott added a commit to Trott/io.js that referenced this issue Mar 19, 2021
Trott added a commit to Trott/io.js that referenced this issue Mar 19, 2021
ruyadorno pushed a commit that referenced this issue Mar 20, 2021
Add a known_issues test for a known Windows issue.

Refs: #13683

PR-URL: #37744
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@Trott Trott closed this as completed in b0d5e03 Apr 3, 2021
MylesBorins pushed a commit that referenced this issue Apr 4, 2021
Fixes: #13683

PR-URL: #37747
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this issue May 1, 2021
Add a known_issues test for a known Windows issue.

Refs: #13683

PR-URL: #37744
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. path Issues and PRs related to the path subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
10 participants