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

Windows file URL prepends "/" at beginning of path property, breaking compatibility #857

Closed
gregcotten opened this issue Aug 15, 2024 · 12 comments · Fixed by #964
Closed
Assignees

Comments

@gregcotten
Copy link

gregcotten commented Aug 15, 2024

For example, initialize a simple file URL and print its path:

let url = URL(fileURLWithPath: "C:/Test")
print(url.path)

In Swift 5.10.1, you get: C:/Test
In Swift 6.0 (August 7 snapshot) you get: /C:/Test

Maybe this is intended, but it definitely breaks my usage of passing file URL paths as arguments into a spawned Process for my build tools.

@gregcotten gregcotten changed the title Windows file URL prepends "/" at beginning of path property Windows file URL prepends "/" at beginning of path property, breaking compatibility Aug 15, 2024
@jmschonfeld
Copy link
Contributor

@jrflat I believe this is the new behavior of URL, so you might be able to look into this. @gregcotten I'd be interested hearing more about how this breaks your use of Process as I'd expect Process itself should still handle this (as URL.withUnsafeFileSystemRepresentation and FileManager.fileSystemRepresentation which are used to provide file system paths to C APIs should still handle this case and remove the leading slash) so at a minimum we should probably correct Process to account for this (I updated Process to correct the executable path with swiftlang/swift-corelibs-foundation#4999)

@gregcotten
Copy link
Author

Interesting! I guess my issue with path being RFC8089 is that it's a rather large change, especially for me :)

I often use someURL.path as a way of communicating a path to a non-Swift party, like calling out to an external process from Swift with a someURL.path as an argument, or in the case of our Electron app, communicating with their JS backend about presenting a save file dialog with a specified path, etc...

@jrflat
Copy link
Contributor

jrflat commented Aug 15, 2024

@jmschonfeld Yeah I think we were just discussing there could be merit in moving Windows path logic (replacing / with \, removing leading slash if there's a drive letter) to URL.fileSystemPath, which is vended to URL.path for compatibility. I can look into that.

@dschaefer2
Copy link
Member

Any update on this? @ahoppen mentioned it breaks sourcekit-lsp on Windows.

@jrflat
Copy link
Contributor

jrflat commented Oct 7, 2024

Yes, apologies for the delay! Opened a PR to resolve the leading slash issue.

I think in the future it could also be a good idea for URL.fileSystemPath to convert all slashes to backslashes on Windows. Although, since URL.fileSystemPath and URL.path are synonymous, and this might further break URL.path compatibility, we'll hold off on that change for now.

@grynspan
Copy link

grynspan commented Oct 16, 2024

@jrflat Can we get this cherry-picked to release/6.0 for the next dot release?
n/m just saw #983

@jrflat
Copy link
Contributor

jrflat commented Oct 16, 2024

Yes for reference this is cherry-picked to release/6.0 in #969 and release/6.0.2 in #983

@ahoppen ahoppen reopened this Oct 31, 2024
@ahoppen
Copy link
Member

ahoppen commented Oct 31, 2024

I’m still seeing this behavior in 6.0.2.

print(URL(string: "file:///C:/test.swift")!.path)

prints /C:/test.swift

Was this fix not cherry-picked to 6.0.2 after all

The issue does appear to be fixed on swift-DEVELOPMENT-SNAPSHOT-2024-10-25-a. There doesn’t seem to be any recent Swift 6.0 snapshots available for Windows, so I can’t test that. Just want to make sure that we aren’t unintentionally missing this from release/6.0.

@jrflat
Copy link
Contributor

jrflat commented Oct 31, 2024

This was cherry-picked to 6.0.2 in #983, I'll try to find a recent 6.0.2 Windows toolchain to confirm.

@jrflat
Copy link
Contributor

jrflat commented Oct 31, 2024

@gregcotten
Copy link
Author

Works for me on the Windows 6.0.2 x86_64 release toolchain available on https://swift.org/download

@ahoppen
Copy link
Member

ahoppen commented Oct 31, 2024

Ah, I think I forgot that there was still #977. Sorry for the confusion.

@ahoppen ahoppen closed this as completed Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants