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

PathCchIsRoot Updated isRoot extension. #892

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

macshome
Copy link
Contributor

@macshome macshome commented Dec 9, 2024

Now that swiftlang/swift-foundation#976 and swiftlang/swift-foundation#980 are fixed we can clean this code up a bit by removing the empty path check on Linux and by using the native PathCchIsRoot on Windows.

The isRoot extension now uses native Windows and macOS API and has the Linux tests cleaned up.

  • macOS root is determined by comparing the path to NSOpenStepRootDirectory()
  • Windows root is determined by a call to PathCchIsRoot
  • Linux root is determined by a comparison to "/"
  • Non-filesystem type URLs always return false as they can't be a filesystem root.

Fixes: #844

All tests pass on macOS and Windows 11.

@ahoppen
Copy link
Member

ahoppen commented Dec 9, 2024

We can’t switch this unconditionally yet because those fixes aren’t in Swift 6.0 toolchains. I think we need to guard this by #if compiler(>=6.1) and fall back to the old behavior if not.

@macshome
Copy link
Contributor Author

macshome commented Dec 9, 2024

OK, I gated the new behavior behind a check for >=6.1. Below that the old code is still in place. The Mac behavior isn't gated as it most certainly isn't new. 😄

#if os(Windows)
// FIXME: We should call into Windows' native check to check if this path is a root once https://github.com/swiftlang/swift-foundation/issues/976 is fixed.
return self.path.withCString(encodedAs: UTF16.self, PathCchIsRoot)
Copy link
Member

Choose a reason for hiding this comment

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

path will be the URL path components. We should use filePath or withUnsafeFileSystemRepresentation instead.

#if os(Windows)
// FIXME: We should call into Windows' native check to check if this path is a root once https://github.com/swiftlang/swift-foundation/issues/976 is fixed.
return self.path.withCString(encodedAs: UTF16.self, PathCchIsRoot)
#elseif os(Linux)
Copy link
Member

Choose a reason for hiding this comment

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

This should be #else. Otherwise this function doesn’t have a return value eg. on platforms that are neither macOS, Linux, nor Windows, like Android.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had that thought and will fix it.

Comment on lines 27 to 25
#endif

#if compiler(>=6.1)
Copy link
Member

Choose a reason for hiding this comment

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

I would change this to #elseif compiler(>=6.1) to ensure we don’t get any of the returns below for macOS

@macshome
Copy link
Contributor Author

OK. I updated with the suggested changes, but note that I don't have my Windows setup with me right now. If someone can run the tests on Windows it would be great.

Now that swiftlang/swift-foundation#976 and swiftlang/swift-foundation#980 are fixed we can clean this code up a bit by removing the empty path check on Linux and by using the native `PathCchIsRoot` on Windows.

Existing code is retained for toolchains <6.1
@ahoppen
Copy link
Member

ahoppen commented Dec 10, 2024

I updated your branch make it build on Windows. I also removed the macOS-specific call to NSOpenStepRootDirectory. It’s effectively / and I would prefer to not have the code between macOS and Linux unless necessary.

@ahoppen ahoppen enabled auto-merge December 10, 2024 21:23
@ahoppen ahoppen merged commit ed01028 into swiftlang:main Dec 10, 2024
19 checks passed
@macshome macshome deleted the PathCchIsRoot branch December 13, 2024 18:44
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 this pull request may close these issues.

Call into PathCchIsRoot to check if a URL points to a root directory
2 participants