Skip to content

Basic: use .standardizingPath instead of realpath #2288

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Basic: use .standardizingPath instead of realpath #2288

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions Sources/Basic/PathShims.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,11 @@
while making it fairly easy to find those calls later.
*/

import SPMLibc
import Foundation

/// Returns the "real path" corresponding to `path` by resolving any symbolic links.
public func resolveSymlinks(_ path: AbsolutePath) -> AbsolutePath {
let pathStr = path.pathString

// FIXME: We can't use FileManager's destinationOfSymbolicLink because
// that implements readlink and not realpath.
if let resultPtr = SPMLibc.realpath(pathStr, nil) {
let result = String(cString: resultPtr)
// FIXME: We should measure if it's really more efficient to compare the strings first.
return result == pathStr ? path : AbsolutePath(result)
}

return path
return AbsolutePath(path.pathString.resolvingSymlinksInPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand and support the motivation here, but I have some concerns about performance. For example, I think that the swift-corelibs-foundation implementation implements this by creating multiple URLs and breaking it into components. Would it be possible to conditionally keep the optimized implementation and use the more generic one only for platforms that don't support realpath()?

We should of course measure to see whether there's really a difference, but shouldn't make this change unless we know it doesn't slow down the current platforms.

Copy link
Contributor

@karwa karwa Sep 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Just a curious onlooker here, but the Foundation API does exactly what this one is supposed to do, and if there are more optimal ways to get the result, that should be a Foundation-level optimisation which would benefit everybody, right?

}

/// Creates a new, empty directory at `path`. If needed, any non-existent ancestor paths are also created. If there is
Expand Down