Add Support for '\' in Windows Paths #2239

gmittert · 2019-05-09T01:51:14Z

While URLs get converted to forward slashes on Windows, strings are
handled as is and thus need to be able to handle \ separators. Since
many Windows apis also support /, we'll handle that as well, but default
to \ if required.

gmittert · 2019-05-09T01:51:43Z

@swift-ci please test

gmittert · 2019-05-09T17:32:19Z

@swift-ci please test linux platform

spevans · 2019-05-10T15:23:44Z

@gmittert This seems to allow paths containing both / and \ as seperators at the same time, eg \foo/bar/file.txt. Is that valid or should there be a check to ensure only one type is used throughout the path?

gmittert · 2019-05-10T18:33:16Z

@gmittert This seems to allow paths containing both / and \ as seperators at the same time, eg \foo/bar/file.txt. Is that valid or should there be a check to ensure only one type is used throughout the path?

That's intentional. Most Windows APIs are able to handle both / and \ but will canonicalize / to \. e.g. C:\Users/gmittertreiner\foo/bar is valid to pass to CreateFileW and will do the right thing. It only becomes an issue if you use the \\?\ paths, which will prevent the canonicalization. However, I think we only do that in URL which any thing passed to it should already get canonicalized to \ on Windows.

It potentially paints over areas that are doing things like concatting paths with forward slashes, but since Windows supports the path style, I think that's for the better.

gmittert · 2019-05-17T22:23:27Z

cc @millenomi

lxbndr · 2019-06-05T08:34:09Z

I am eagerly awaiting for this PR, looks very promising. Maybe I am missing something, but is it ok to allow Windows path separator in NSURL? Looks like NSURL should correctly handle initialization with filesystem path, but result URL string should contain separators defined in RFC. I guess path property of NSURL&URL also have to follow RFC rules, because it is not an issue for Windows file: URLs to have / separators.

gmittert · 2019-06-05T18:10:05Z

@lxbndr Based on how path is implemented it'll give you a file system path (e.g. C:\Users\gwenm\AppData\Local\Temp\ on Windows) as opposed to a file:\\. Then since NSURL uses its .path internally, it unfortunately does have to handle the \ separators.

gmittert · 2019-06-05T18:17:59Z

Rebased. @swift-ci please test

gmittert · 2019-06-05T20:52:26Z

@swift-ci please test macos platform

lxbndr · 2019-06-05T22:57:31Z

@gmittert Right, implementation of path is important for lastPathComponent. Actually I am here because I was looking for pending solution of broken lastPathComponent. What looks wrong to me is that URL API threats URL path as filesystem path. I see two ways of fixing that.

Force NSURL.path always use POSIX path separators. This will make lastPathComponent work. And follows RFC.
Make path components related code work with both slashes.

I did rough implementation of first solution, and it kinda works. Does it make sense? Still not ready for PR yet, but if it sounds good, then guess I able to finish it - look for side effects, check tests, etc.

gmittert · 2019-06-05T23:48:20Z

@lxbndr Oh I see, (URL/NSURL).path should return a RFC 1808 style url and it doesn't right now. That does seem very wrong.

The PathUtilities changes need to go in anyways, but I could drop the NSURL changes I have one and then you can PR your patch.

Alternatively, I can fix the .path style in this patch and make sure it fixes the same FileManager tests I was aiming to fix.

Does either work better for you?

lxbndr · 2019-06-06T05:50:55Z

@gmittert I believe it would be faster to implement decent fix with your experience. If your current schedule allows, can you please take a time for this? If not, I always glad to contribute, first approach PR could be ready next days after this #2239 merged.

gmittert · 2019-06-19T23:46:14Z

Alright, removed most of the NSURL changes except for one which has handling input paths.

I've then made URL.path return an RFC1808 path in #2366 and #2367

gmittert · 2019-06-20T01:30:46Z

@swift-ci please test linux

millenomi · 2019-07-23T22:43:42Z

Foundation/NSURL.swift

@@ -957,7 +957,7 @@ extension NSURL {

    open func appendingPathComponent(_ pathComponent: String) -> URL? {
        var result : URL? = appendingPathComponent(pathComponent, isDirectory: false)
-        if !pathComponent.hasSuffix("/") && isFileURL {
+        if !validPathSeps.contains(where: { pathComponent.hasSuffix(String($0)) }) && isFileURL {


So;

Foundation distinguishes between paths (in NSStrings) and URLs. The latter have harmonized behavior between all platforms, whereas the former can contain a platform path and can thus accommodate specialized platform behaviors.

Trying to append a \ to an URL is a programmer error regardless of platform. URL paths are specified to use / as separators. Thus: I would back off this line and I'm okay taking the rest.

The only bit that puts a wrench into this particular getup is that the result of URL's .path must be useable as a platform path, which can get complicated.

The only bit that puts a wrench into this particular getup is that the result of URL's .path must be useable as a platform path, which can get complicated.

Looks like / path separator will meet this requirement for all platforms, as Windows is (more or less) loyal to both variants. But can you please add more on this "must" part? I always thought that URL/CFURL api clearly distinguish between file system paths and URL paths. Now I feel like I need to reevaluate this for myself.

The only bit that puts a wrench into this particular getup is that the result of URL's .path must be useable as a platform path, which can get complicated.

I see, that does complicate things :(. What are your thoughts on handling absolute URL paths then? On Windows, paths which begin with / or \ are drive relative. That is, if my cwd is C:\ then /Users/gwenm resolves to C:\Users\gwenm.

Unfortunately, this makes /C:/Users/gwenm not a valid windows path, so I think there really only two reasonable things to return from URL(fileURLWithPath: "C:\\Users\\gwenm").path on Windows:

/C:/Users/gwenm which, while not a valid Windows path, we would at least be able to add support for throughout Foundation so that e.g. "foo".write(to: myURL.path, ...) would work.

C:/Users/gwenm which isn't a proper RFC 1808 path as specified to be returned by Foundation, as an absolute path must start with /, but would work directly with Windows apis (though would not be able to be copied and pasted into cmd.exe and directly used without first being quoted).

millenomi

Per above.

millenomi · 2019-07-23T22:44:13Z

The only exceptions I make to this contract are those already documented in CFURL.

While URLs get converted to forward slashes on Windows, strings are handled as is and thus need to be able to handle \ separators. Since many Windows apis also support /, we'll handle that as well, and default to a posix style / if required.

gmittert · 2019-07-26T18:13:22Z

Removed the check for backslashes in NSURL and left a comment on why it's not needed instead.

gmittert · 2019-07-26T18:13:29Z

@swift-ci please test linux

gmittert · 2019-07-31T00:01:27Z

@swift-ci please test linux

gmittert · 2019-08-06T16:47:35Z

@millenomi One week courtesy ping

gmittert · 2019-09-05T22:43:22Z

@millenomi Courtesy ping :)

compnerd · 2019-12-03T00:31:32Z

I believe that Lily's comments have been addressed. I am going to merge this to get s-p-m on Windows working.

gmittert force-pushed the SlashingForwardsAndBack branch from 86cc5aa to e2220be Compare June 5, 2019 18:10

gmittert force-pushed the SlashingForwardsAndBack branch from e2220be to 4f37b6c Compare June 19, 2019 23:21

millenomi reviewed Jul 23, 2019

View reviewed changes

millenomi suggested changes Jul 23, 2019

View reviewed changes

gmittert force-pushed the SlashingForwardsAndBack branch from 4f37b6c to 64d5f3e Compare July 26, 2019 18:12

gmittert requested a review from millenomi September 10, 2019 18:42

compnerd merged commit 3b47193 into swiftlang:master Dec 3, 2019

gmittert deleted the SlashingForwardsAndBack branch December 3, 2019 00:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for '\' in Windows Paths #2239

Add Support for '\' in Windows Paths #2239

gmittert commented May 9, 2019

gmittert commented May 9, 2019

gmittert commented May 9, 2019

spevans commented May 10, 2019

gmittert commented May 10, 2019 •

edited

Loading

gmittert commented May 17, 2019

lxbndr commented Jun 5, 2019

gmittert commented Jun 5, 2019 •

edited

Loading

gmittert commented Jun 5, 2019 •

edited

Loading

gmittert commented Jun 5, 2019

lxbndr commented Jun 5, 2019

gmittert commented Jun 5, 2019 •

edited

Loading

lxbndr commented Jun 6, 2019

gmittert commented Jun 19, 2019 •

edited

Loading

gmittert commented Jun 20, 2019

millenomi Jul 23, 2019

millenomi Jul 23, 2019

lxbndr Jul 24, 2019

gmittert Jul 24, 2019 •

edited

Loading

millenomi left a comment

millenomi commented Jul 23, 2019

gmittert commented Jul 26, 2019

gmittert commented Jul 26, 2019

gmittert commented Jul 31, 2019

gmittert commented Aug 6, 2019

gmittert commented Sep 5, 2019

compnerd commented Dec 3, 2019

Add Support for '\' in Windows Paths #2239

Add Support for '\' in Windows Paths #2239

Conversation

gmittert commented May 9, 2019

gmittert commented May 9, 2019

gmittert commented May 9, 2019

spevans commented May 10, 2019

gmittert commented May 10, 2019 • edited Loading

gmittert commented May 17, 2019

lxbndr commented Jun 5, 2019

gmittert commented Jun 5, 2019 • edited Loading

gmittert commented Jun 5, 2019 • edited Loading

gmittert commented Jun 5, 2019

lxbndr commented Jun 5, 2019

gmittert commented Jun 5, 2019 • edited Loading

lxbndr commented Jun 6, 2019

gmittert commented Jun 19, 2019 • edited Loading

gmittert commented Jun 20, 2019

millenomi Jul 23, 2019

Choose a reason for hiding this comment

millenomi Jul 23, 2019

Choose a reason for hiding this comment

lxbndr Jul 24, 2019

Choose a reason for hiding this comment

gmittert Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

millenomi left a comment

Choose a reason for hiding this comment

millenomi commented Jul 23, 2019

gmittert commented Jul 26, 2019

gmittert commented Jul 26, 2019

gmittert commented Jul 31, 2019

gmittert commented Aug 6, 2019

gmittert commented Sep 5, 2019

compnerd commented Dec 3, 2019

gmittert commented May 10, 2019 •

edited

Loading

gmittert commented Jun 5, 2019 •

edited

Loading

gmittert commented Jun 5, 2019 •

edited

Loading

gmittert commented Jun 5, 2019 •

edited

Loading

gmittert commented Jun 19, 2019 •

edited

Loading

gmittert Jul 24, 2019 •

edited

Loading