Clean up symlink following and recursively follow symlink chains. #947
Conversation
allevato
requested review from
ahoppen,
bnbarham and
shawnhyam
as code owners
| while followSymlinks && fileType == .typeSymbolicLink, | ||
| let destination = try? FileManager.default.destinationOfSymbolicLink(atPath: url.path) |
There was a problem hiding this comment.
Should we detect circles here if you have a symlink that points to itself?
There was a problem hiding this comment.
Good catch; fixed. Symlink cycles (either a link pointing to itself, or a multi-link cycle) are silently ignored now. We could warn on this, but we'd have to plumb DiagnosticsEngine down into FileIterator, which I didn't really want to do here if we can avoid it.
There was a problem hiding this comment.
As long as we don’t infinite loop, I think the behavior is OK.
Also, it only just occurs to me, why can’t we use resolvingSymlinksInPath/realpath here if we follow all symlinks?
In SourceKit-LSP, we have the following code to avoid the /tmp vs /private/tmp issue: https://github.com/swiftlang/sourcekit-lsp/blob/3e7f350246dc86072a9dfc88f25782dc86dd1c9a/Sources/SwiftExtensions/URLExtensions.swift#L40-L57
There was a problem hiding this comment.
Hmm, good question. That realpath code seems to work on macOS from local testing, so let me see if it works on Linux/Windows. Assuming they both consistently resolve all symlinks deeply or stop when a cycle is reached on all platforms, then that would remove the need for our own loop.
There was a problem hiding this comment.
Sigh, no good. Based on CI results and local testing, it looks like resolvingSymlinksInPath on Linux pre-5.x Foundation doesn't correctly resolve through multiple symlinks. So we'd need a special case to handle those, which would essentially be the code I had earlier anyway.
Maybe one day when we drop 5.x support, we can clean this all up.
allevato
force-pushed
the
symlink-tweaks
branch
2 times, most recently
from
3c83909 to
8d5c78f
Compare
allevato
force-pushed
the
symlink-tweaks
branch
2 times, most recently
from
d3c336c to
01094d1
Compare
This started out as a weird debugging journey. We had a team running
swift-formatas part of a Bazel action to format generated Swift code. The action basically just ranswift-format $input > $output. This action started failing (generating no output) in this environment because the Bazel action workspace was set up such that$inputwas a symlink to the actual file. Even if I updated the action to pass--follow-symlinks, the oldFileIteratorcode had afallthroughthat didn't correctly handle the case where a path passed directly on the command line was a symlink to a file instead of a directory.The reason that this actually started failing was because we finally updated our internal toolchain to use the new swift-foundation instead of the legacy swift-corelibs-foundation (before it was layered on top of swift-foundation). Something in the implementation of
FileManagerchanged subtly and started giving usURLs back that pointed to symlinks when previously we were getting them pre-resolved, which revealed the above issue.While I was in here, I decided to add support for following symlinks to other symlinks, just to make sure that didn't become an issue in the future.