Skip to content

Strip \\?\ prefix from path generated by resolveSymlink #485

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 11, 2024

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Oct 10, 2024

This aligns the implementation of resolveSymlinks with that of Foundation.String._resolvingSymlinksInPath, which received this canonicalization fix in swiftlang/swift-foundation#639.

@ahoppen
Copy link
Member Author

ahoppen commented Oct 10, 2024

@swift-ci Please test

@MaxDesiatov MaxDesiatov requested a review from compnerd October 10, 2024 16:35
throw FileSystemError(.unknownOSError, path)
}
let path = String(decodingCString: $0.baseAddress!, as: UTF16.self)
// When using `VOLUME_NAME_DOS`, the returned path uses `\\?\`.
let path = String(decodingCString: $0.baseAddress!.advanced(by: 4), as: UTF16.self)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want an assertion that the prefix is present?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was following what you did in swiftlang/swift-foundation#639. I guess we should also add the check there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Making the corresponding change to swift-foundation here: swiftlang/swift-foundation#974

This aligns the implementation of resolveSymlinks with that of Foundation.String._resolvingSymlinksInPath, which received this canonicalization fix in swiftlang/swift-foundation#639.
@ahoppen
Copy link
Member Author

ahoppen commented Oct 10, 2024

@swift-ci Please test

ahoppen added a commit to ahoppen/swift-foundation that referenced this pull request Oct 10, 2024
As far as I know `\\?\` is always returned by `GetFinalPathNameByHandleW` but it’s better to be safe and actually check instead of unconditionally trimming the first 4 characters from the returned path.

See swiftlang/swift-tools-support-core#485 (comment)
@ahoppen
Copy link
Member Author

ahoppen commented Oct 10, 2024

@swift-ci Please test Windows

ahoppen added a commit to ahoppen/swift-foundation that referenced this pull request Oct 10, 2024
As far as I know `\\?\` is always returned by `GetFinalPathNameByHandleW` but it’s better to be safe and actually check instead of unconditionally trimming the first 4 characters from the returned path.

See swiftlang/swift-tools-support-core#485 (comment)
ahoppen added a commit to ahoppen/swift-foundation that referenced this pull request Oct 10, 2024
As far as I know `\\?\` is always returned by `GetFinalPathNameByHandleW` but it’s better to be safe and actually check instead of unconditionally trimming the first 4 characters from the returned path.

See swiftlang/swift-tools-support-core#485 (comment)
@ahoppen
Copy link
Member Author

ahoppen commented Oct 10, 2024

@swift-ci Please test Windows

jrflat pushed a commit to swiftlang/swift-foundation that referenced this pull request Oct 11, 2024
As far as I know `\\?\` is always returned by `GetFinalPathNameByHandleW` but it’s better to be safe and actually check instead of unconditionally trimming the first 4 characters from the returned path.

See swiftlang/swift-tools-support-core#485 (comment)
@ahoppen
Copy link
Member Author

ahoppen commented Oct 11, 2024

macOS CI failure is unrelated. This change only affects Windows. macOS CI is failing because this is the only repo that build SourceKit-LSP using a 5.9 host compiler, but SourceKit-LSP requires 5.10 to build successfully. We didn’t hit this before because this repo hasn’t seen any changes in the last 2 months, which is when SourceKit-LSP started requiring Swift 5.10.

@shahmishal shahmishal enabled auto-merge (squash) October 11, 2024 22:09
@shahmishal shahmishal disabled auto-merge October 11, 2024 22:09
@shahmishal shahmishal merged commit 27042e9 into swiftlang:main Oct 11, 2024
2 of 3 checks passed
@ahoppen ahoppen deleted the resolvesymlink branch October 16, 2024 18:14
cthielen pushed a commit to cthielen/swift-foundation that referenced this pull request Nov 8, 2024
…tlang#974)

As far as I know `\\?\` is always returned by `GetFinalPathNameByHandleW` but it’s better to be safe and actually check instead of unconditionally trimming the first 4 characters from the returned path.

See swiftlang/swift-tools-support-core#485 (comment)
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.

3 participants