Closed
Labels
pathIssues and PRs related to the path subsystem.Issues and PRs related to the path subsystem.windowsIssues and PRs related to the Windows platform.Issues and PRs related to the Windows platform.
Description
- 6.11.1:
- Window 10 x64:
In windows, path.join('C:') return 'C:.' not 'C:'
In other system, such as linux,macos always return 'C:'
In windows, 'path.join('C:\')' is corrrect, return 'C:\'
But you give path.join more than one args, it's correct.
such as path.join('C:', 'a') == 'C:\a'
here is a interesting thing, if you run path.format(path.parse(path.join('C:'))) in windows,
returns 'C:C:.'
I don't know why...
Metadata
Metadata
Assignees
Labels
pathIssues and PRs related to the path subsystem.Issues and PRs related to the path subsystem.windowsIssues and PRs related to the Windows platform.Issues and PRs related to the Windows platform.
Activity
TimothyGu commentedon Jul 21, 2017
In general, the path normalization behavior is platform-dependent. You might find
path.win32.*andpath.posix.*to be helpful for testing on different platforms.Now, the issue itself is actually two issues as far as I can tell.
The
path.win32.join('C:') === 'C:.'issue is actually coming frompath.win32.normalize('C:') === 'C:.'. It should normalize to'C:\\'IMO.The join->parse->format issue is more complex:
I would expect
'C:.'to parse toSimilarly:
seems to be faulty as well.
TimothyGu commentedon Jul 21, 2017
These issues are also reproducible on master.
tniessen commentedon Jul 22, 2017
Well, what can I say... Paths on Windows are a mess, and node really isn't dealing with that mess too well yet.
Okay, the colon has three separate meanings under Windows, and it is not trivial to dinstinguish them:
This document describes some of these aspects. Consider this paragraph:
Note that
C:is not an absolute path according to Microsoft. It is quite the opposite:We always tried to keep the
pathmodule as platform-independent as possible. POSIX paths are easy to work with, they follow simple rules, but Windows paths do not.Note that the path API has some other platform-specific problems, mostly related to
path.xxx.resolve, see #1999, #13738 and #14107. I was hoping to have some time to fix some of the problems soon.I will try to explain why some of your examples @XGHeaven and @TimothyGu are not as unambiguous as you might think:
should either throw, return
C:or the current working directory on driveC:.should return
C:\.I am not sure whether
C:\ais actually correct. Windows will returnC:ain this case, even though most users would probably expectC:\a, which Windows will return if you pass inC:\anda.Again, this is not an absolute path:
I am not sure how we would represent this.
tl;dr:
It will be impossible to solve these problems without a very tight connection to the host platform, therefore making it impossible to use the same API on other platforms. Windows paths follow highly complex patterns and cannot be parsed as easily as POSIX paths.
cc @refack
XGHeaven commentedon Jul 22, 2017
Whatever it return, I think
C:.is absolute wrong. fix this first.I prefer to return
C:a. But if you want to getC:\a, best to useAnd colon usage case 3 in windows, it never used in development or cmd for myself.
So I think case 3 has a low priority.
refack commentedon Jul 23, 2017
Sometimes I wish GitHub would add 😭 as a reaction.
refack commentedon Jul 23, 2017
Actually, beside being ugly
C:.is correct (as inc:is for the drive,.is for current directory)But I agree that
path.format(path.parse(path.join('C:'))) === 'C:C:.'is a bug.refack commentedon Jul 23, 2017
pathshould not only be platform independent, it should also be stateless, performing pure string manipulation with no system calls. So we have two issues here resolvingC:is state dependant, and resolving.is state dependant... 🤷♂️TimothyGu commentedon Jul 23, 2017
@tniessen It might be helpful to look at what other platforms are doing. Python 3.5.3 does:
From what I'm reading in this thread, the
C:.->C:normalization should be expected and correct, and it is IMO what we should do as well. On the other hand, Python'sntpath.joindoesn't normalize the path after returning, so joining'C:'and'.'returns'C:.'.About
parse, I guess now I would still expectC:.to parse toThe
rootisC:alright, while the actual path isdir + path.sep + nameso..By the same logic
C:abcshould beTimothyGu commentedon Jul 23, 2017
Here's the Python equivalent for
parse:PureWindowsPathnormalizes the path (as seen in the last pair of lines), but otherwise I think it's pretty consistent with what I proposed above. Ourrootis basically Python'sanchorBTW.tniessen commentedon Jul 23, 2017
Yes, it is correct, but I am not entirely sure we should prefer that over
C:without the dot (see python ↑). Apparently,C:.comes as a surprise to many people. The main question is whetherC:behaves the same asC:.when given as input to other API functions.@TimothyGu Sounds good. Your interpretation of
parseseems to be reasonable. We are still stuck with the problem of implementingresolveto deal with these paths...Currently, the only platform-dependent functions are the
resolveimplementations, and we won't be able to change that. In retrospect,resolvemight have been better suited forosto distinguish it from its platform-independent siblings, but we would still need to use it withinpath.relativeetc., or change the semantics of those functions. My best approach to solving some platform-dependent problems was this variant ofresolve(I was about to implement it for #13738, but did not have time yet):cwdis not required to compute the result, use the old implementation.path.relative), letcwdbe.and return a relative path.In my opinion, this is our best shot at a compromise between correctness and platform-independentness. This behavior will need to be documented precisely. For the vast majority of users, this won't change anything. I hope to get some time within the next days to propose a solution to some of these problems.
refack commentedon Jul 23, 2017
There's also my suggested #13887 (comment) opt-in way, either:
or
TimothyGu commentedon Jul 23, 2017
@tniessen This issue is about path normalization (which doesn't depend on the local file system), not resolution.
tniessen commentedon Jul 23, 2017
@TimothyGu I know, I just thought the other problems were related enough to bring them up here. Sure, this issue can be solved independently.
2 remaining items
TimothyGu commentedon Jul 24, 2017
I just opened #14440 after auditing all
pathfunctions, which fixesparse()and adds tests to make sure all of the pure functions work with volume-relative paths. I intentionally did not changepath.normalize('C:') === 'C:.'however, sincepath.normalize('') === '.'under the current Node.js implementation and the current behavior just seems to be consistent with it.tniessen commentedon Jul 30, 2017
@XGHeaven Thank you for your report, @TimothyGu fixed the behavior of the
pathAPI to correctly work with volume-relative paths.XGHeaven commentedon Jul 31, 2017
@TimothyGu Thanks. I have a question about if this bug fixed in LTS? I remember this bug just fixed in master.
tniessen commentedon Jul 31, 2017
Backporting to release lines can take some time. cc @nodejs/lts
path: fix win32 volume-relative paths
gibfahn commentedon Aug 4, 2017
@XGHeaven This will be considered in the next v6.x release. If 2791761 cherry-picks cleanly to v6.x, and the risk of the change is deemed to be low, it should go into the next release. If it doesn't cherry-pick cleanly it'll have to be backported (process here).
v4.x is in maintenance, so most fixes aren't backported.
I suggest you subscribe to #14440, any discussion about backporting to v6.x will happen on there.
path: fix win32 volume-relative paths
path: fix win32 volume-relative paths
path: fix win32 volume-relative paths
path: fix win32 volume-relative paths