Standardize file paths when attempting to find toolchains #2289
Conversation
bnbarham
requested review from
ahoppen,
hamishknight and
rintaro
as code owners
|
@swift-ci please test |
There was a problem hiding this comment.
I think this will cause issues if you have a toolchain with a symlink during the discovery phase and the BSP server returns a path that contains the symlink. You’ll also need to use the same path normalization in ToolchainRegistry.toolchain or wherever we get the path that is passed to it from.
Also, we should check how this behaves if we discover two toolchains that have different paths (due to symlinks) but point to the same realpath. This is usually the case on macOS when you have open source toolchains installed and swift-latest.xctoolchain is a symlink to the latest installed toolchain. Ideally, we would only keep one copy of these toolchains in ToolchainRegistry now because they are identical.
Finally, if we realpath the path of xctoolchains, we should also standardize the path of other toolchains of consistent matching behavior between the toolchain returned by BSP and the ones discovered by ToolchainRegistry. For example, if I have a PATH entry /symlink/to/usr/bin pointing to /usr/bin (which contains toolchains) and a BSP server returns /usr/bin/ as the toolchain of a target, we should be able to match those as well – at the moment we explicitly don’t do any path canonicalization for toolchain paths but if we introduce that now, it should cover all toolchain paths.
| @@ -369,7 +369,7 @@ public final class Toolchain: Sendable { | |||
| func containingXCToolchain( | |||
| _ path: URL | |||
| ) -> (XCToolchainPlist, URL)? { | |||
| var path = path | |||
| var path = path.standardizedFileURL | |||
There was a problem hiding this comment.
I would suggest using .realpath instead of standardizedFileURL to avoid the /tmp vs /private/tmp issue on macOS.
FWIW this already happens today - they both have the same identifier, so we only use one and the other is skipped (with an error logged).
Good point, I'll fix up the lookups.
Yeah, this is the cause of the test failures at the moment (well, sort of, in that the test doesn't do a standardize and thus we get different /tmp vs /private/tmp). |
bnbarham
force-pushed
the
infinite-root-2
branch
from
a082d41 to
0462b9c
Compare
|
@swift-ci please test |
`containingXCToolchain` loops infinitely when given eg. `C:\foo\..\bar`. The underlying cause is that `deletingLastPathComponent` does not remove the `..` on Windows. On macOS it's potentially worse - it *adds* `..`. We don't see the infinite loop on macOS/Linux though because `AbsolutePath` already removes them (which is not the case on Windows). Resolves swiftlang#2174.
bnbarham
force-pushed
the
infinite-root-2
branch
from
0462b9c to
1325e32
Compare
| @@ -283,7 +286,18 @@ package final actor ToolchainRegistry { | |||
| /// If we have a toolchain in the toolchain registry that contains the compiler with the given URL, return it. | |||
| /// Otherwise, return `nil`. | |||
| package func toolchain(withCompiler compiler: URL) -> Toolchain? { | |||
| return toolchainsByCompiler[compiler] | |||
| if let resolvedPath = try? compiler.realpath { | |||
| return toolchainsByCompiler[resolvedPath] | |||
There was a problem hiding this comment.
We should run realpath when populating toolchainsByCompiler as well to cover symlinks within the toolchain, eg. swift and swiftc to swift-driver.
There was a problem hiding this comment.
Ah, good point. I didn't do this since we already realpathed the toolchain path coming into ToolchainRegistry, but forgot about the case the binaries themselves could be. We could just realpath the parent folder here?
EDIT: Hmmm, but we don't know the "toolchain" path in order to do that. I don't love the idea of realpathing to swift-driver though.
| @@ -283,7 +286,18 @@ package final actor ToolchainRegistry { | |||
| /// If we have a toolchain in the toolchain registry that contains the compiler with the given URL, return it. | |||
| /// Otherwise, return `nil`. | |||
| package func toolchain(withCompiler compiler: URL) -> Toolchain? { | |||
| return toolchainsByCompiler[compiler] | |||
| if let resolvedPath = try? compiler.realpath { | |||
There was a problem hiding this comment.
This means that we need to run realpath and thus hit the filesystem for every entry in a JSON compilation database. Should we try and look up the non-realpath version first? And if we have a hit using the realpath, we can add a mapping for the original path to the toolchain as well as a cache. That way we only need to realpath every given path in a JSON compilation database once.
| if let resolvedPath = try? path.realpath { | ||
| return toolchainsByPath[resolvedPath] | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Similar to my comment on toolchain(withCompiler:), should we try a non-realpath lookup first and then cache if necessary to avoid hitting the file system unnecessarily? E.g. something like
if let toolchain = toolchainsByPath[path] {
return toolchain
}
guard let realpath = orLog("Realpath toolchain", { try path.realpath }) else {
return nil
}
guard let toolchain = lchainsByPath[realpath] else {
return nil
}
// Cache mapping of non-realpath to the realpath toolchain for faster subsequent lookups
toolchainsByPath[path] = toolchain
return toolchain
containingXCToolchainloops infinitely when given eg.C:\foo\..\bar. The underlying cause is thatdeletingLastPathComponentdoes not remove the..on Windows. On macOS it's potentially worse - it adds... We don't see the infinite loop on macOS/Linux though becauseAbsolutePathalready removes them (which is not the case on Windows).Resolves #2174.