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

Added support for recursively resolving symlinks #1566

Closed
wants to merge 9 commits into from
Closed

Added support for recursively resolving symlinks #1566

wants to merge 9 commits into from

Conversation

pvieito
Copy link
Contributor

@pvieito pvieito commented May 27, 2018

Used by URL.resolvingSymlinksInPath().

The current Swift Foundation implementation only tries to solve the first symlink while Darwin Foundation tries to solve recursively all the symlinks.

Used by URL.resolvingSymlinksInPath()
@pvieito pvieito changed the title Added support for recursively resolve symlinks Added support for recursively resolving symlinks May 27, 2018
@pushkarnk
Copy link
Collaborator

@swift-ci please test

@pvieito
Copy link
Contributor Author

pvieito commented May 28, 2018

I have set a recursion limit of 50, but I would like to know what Darwin Foundation does to maintain both implementations aligned.

@millenomi
Copy link
Contributor

This feels like it would need a test.

@pvieito
Copy link
Contributor Author

pvieito commented May 28, 2018

Added tests

@pvieito
Copy link
Contributor Author

pvieito commented May 28, 2018

@swift-ci please test

@spevans
Copy link
Collaborator

spevans commented May 28, 2018

@swift-ci please test

@pvieito
Copy link
Contributor Author

pvieito commented May 29, 2018

Please, can somebody initiate the @swift-ci tests?

@parkera
Copy link
Member

parkera commented May 29, 2018

@swift-ci please test

do {

// Initialization
var baseURL = FileManager.default.temporaryDirectory.appendingPathComponent("test_resolvingSymlinksInPath")
Copy link
Member

Choose a reason for hiding this comment

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

I think we have some of our other tests here stick a UUID in the path, so that if something gets stuck around from a previous run it doesn't hose the next test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good idea. Added!

@millenomi
Copy link
Contributor

@swift-ci please test

@@ -1033,16 +1033,39 @@ open class FileManager : NSObject {
NSUnimplemented()
}

internal func _tryToResolveTrailingSymlinkInPath(_ path: String) -> String? {
internal func _tryToResolveTrailingSymlinkInPath(_ path: String, recursionLevel: Int = 0, initialPath: String? = nil) -> String? {
guard _pathIsSymbolicLink(path) else {
return nil
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

The call to _pathIsSymbolicLink() can be removed as destinationOfSymbolicLink() should fail anyway if the path is not a symbolic link (according to readlink(2)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function _pathIsSymbolicLink() was already there so I decided not to touch it. It was introduced as is with the initial URL.resolvingSymlinksInPath() implementation.

@pvieito
Copy link
Contributor Author

pvieito commented May 30, 2018

@parkera / @millenomi Why NSURL.resolvingSymlinksInPath and NSString.resolvingSymlinksInPath have independent implementations?

@millenomi
Copy link
Contributor

That seems like an oversight; we should have only the one.

Generally, we want to keep these implementations closer to URL and related types.

@spevans
Copy link
Collaborator

spevans commented May 30, 2018

Is this functionality the same as provided by realpath(3)? If so, it might be simpler to just use this function.

@pvieito
Copy link
Contributor Author

pvieito commented Jun 3, 2018

Opened Swift Bug Report: SR-7857

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.

None yet

5 participants