Skip to content

Fix path handling for plugins on Windows #8007

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

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

jakepetroules
Copy link
Contributor

Due to RFC8089 compliance changes for Foundation.URL in Swift 6, URL.path does NOT behave as one might expect, producing a path with a leading slash which will be interpreted by Windows as relative.

Closes #6851

@jakepetroules
Copy link
Contributor Author

@swift-ci please test

@MaxDesiatov MaxDesiatov added the needs tests This change needs test coverage label Sep 29, 2024
@jakepetroules jakepetroules force-pushed the fix-plugins-url-representation branch from 74c84ed to 2cfa4a0 Compare September 29, 2024 09:50
@jakepetroules
Copy link
Contributor Author

@swift-ci please test

@dschaefer2
Copy link
Member

I'd much rather see us adopt FilePath everywhere and get away from URLs.

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Sep 30, 2024

Unfortunately, https://github.com/apple/swift-system doesn't have Windows CI and FilePath support on Windows is known to have had regressions in the past. To make sure it doesn't unexpectedly regress in the future, I'd consider Windows CI on that repo to be a prerequisite.

@jakepetroules
Copy link
Contributor Author

I'd much rather see us adopt FilePath everywhere and get away from URLs.

That would be nice, but it's not clear to me that we'd be able to make FilePath part of the PackageDescription API, since that would introduce a new dependency on SwiftSystem (and that would require an evolution proposal and deprecation period).

@jakepetroules jakepetroules force-pushed the fix-plugins-url-representation branch from 2cfa4a0 to e31e7db Compare October 1, 2024 01:18
@jakepetroules
Copy link
Contributor Author

@SwiftCI please test

@DougGregor
Copy link
Member

@swift-ci please test

@DougGregor
Copy link
Member

@swift-ci please test Windows

Due to RFC8089 compliance changes for Foundation.URL in Swift 6, URL.path does _NOT_ behave as one might expect, producing a path with a leading slash which will be interpreted by Windows as relative.

Closes #6851
@jakepetroules jakepetroules force-pushed the fix-plugins-url-representation branch from e31e7db to c21f771 Compare October 1, 2024 06:40
@jakepetroules
Copy link
Contributor Author

@swift-ci please test

/// This should always be used whenever the file path equivalent of a URL is needed. DO NOT use ``path`` or ``path(percentEncoded:)``, as these deal in terms of the path portion of the URL representation per RFC8089, which on Windows would include a leading slash.
///
/// - throws: ``FileURLError`` if the URL does not represent a file or its path is otherwise not representable.
public var filePath: AbsolutePath {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Since there's some specific requirements here for this computed property, should there be some tests to cover them?

@dschaefer2
Copy link
Member

dschaefer2 commented Oct 1, 2024

I'd much rather see us adopt FilePath everywhere and get away from URLs.

That would be nice, but it's not clear to me that we'd be able to make FilePath part of the PackageDescription API, since that would introduce a new dependency on SwiftSystem (and that would require an evolution proposal and deprecation period).

Agreed. I wouldn't want FilePath in the API, the ergonomics are still a bit suspect. It's just the one that handles Windows paths the best (according to @al45tair if I remember correctly). And FWIW, PackageDescription doesn't have URL in its API. It's just Strings which isn very type safe either.

At any rate, we have plans to look deeper in the Paths issue. I'm happy with this for the near term. Or maybe it's just the better answer anyway :).

@dschaefer2
Copy link
Member

@swift-ci please test windows

@al45tair
Copy link
Contributor

al45tair commented Oct 1, 2024

It's just the one that handles Windows paths the best (according to @al45tair if I remember correctly).

Yeah. FilePath is certainly not perfect. In its current form, the Codable conformance is just dangerous. It also can't handle POSIX paths on Windows or Windows paths on POSIX systems (and this is "by design" since swift-system is officially not for portability purposes). It's also not supported by swift-argument-parser. It's a bit of a frustration really.

@rauhul
Copy link
Member

rauhul commented Oct 1, 2024

It's just the one that handles Windows paths the best (according to @al45tair if I remember correctly).

Yeah. FilePath is certainly not perfect. In its current form, the Codable conformance is just dangerous. It also can't handle POSIX paths on Windows or Windows paths on POSIX systems (and this is "by design" since swift-system is officially not for portability purposes). It's also not supported by swift-argument-parser. It's a bit of a frustration really.

What argparser support are you thinking about?

@dschaefer2
Copy link
Member

You know? I just yearn for Python's pathlib API but in Swift. It was really sweet to work with.

@al45tair
Copy link
Contributor

al45tair commented Oct 1, 2024

What argparser support are you thinking about?

You can't have an argument of type FilePath (also, String is manifestly the wrong type for arguments, which does not help with any of this — there's no reason any given argument should be valid UTF-8).

You know? I just yearn for Python's pathlib API but in Swift. It was really sweet to work with.

FilePath is annoyingly close, but not quite there.

@cmcgee1024
Copy link
Member

You know? I just yearn for Python's pathlib API but in Swift. It was really sweet to work with.

It seems that most languages have a decent universal file path support. Go has this one in its stdlib based on regular strings. https://pkg.go.dev/path/filepath

@jakepetroules jakepetroules merged commit efe0580 into main Oct 1, 2024
5 checks passed
@jakepetroules jakepetroules deleted the fix-plugins-url-representation branch October 1, 2024 22:21
@al45tair
Copy link
Contributor

al45tair commented Oct 2, 2024

It seems that most languages have a decent universal file path support. Go has this one in its stdlib based on regular strings. https://pkg.go.dev/path/filepath

A lot of them get it wrong in various subtle ways, often related to encodings or unusual edge cases (Windows is especially troublesome because there are multiple ways to express the exact same path even before you consider symbolic links or reparse points, plus there is the notion of a per-drive current directory, though the system APIs only expose a single current directory directly and you get to look in the environment for the rest, so you can write things like drive-relative paths.)

@cmcgee1024
Copy link
Member

A lot of them get it wrong in various subtle ways, often related to encodings or unusual edge cases (Windows is especially troublesome because there are multiple ways to express the exact same path even before you consider symbolic links or reparse points, plus there is the notion of a per-drive current directory, though the system APIs only expose a single current directory directly and you get to look in the environment for the rest, so you can write things like drive-relative paths.)

Windows definitely has its own peculiarities, such as reserved filenames like aux, com1, and lpt1. At least the differences with unix filesystem semantics isn't as wildly different as something like the z/OS filesystem or the i5/OS fs, both of which have special vendor-specific Java libraries outside of the standard nio package for working with them. Still, I think that other languages have file path libs that are able to handle the majority of cases at the intersection between unix and windows that most cross-platform packages need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests This change needs test coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PackagePlugin is entirely broken
8 participants