Skip to content

Standardize file paths when attempting to find toolchains #2289

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Standardize file paths when attempting to find toolchains #2289

Show file tree
Hide file tree
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
2 changes: 2 additions & 0 deletions Sources/SwiftExtensions/URLExtensions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ extension URL {
}
}

/// Assuming this URL is a file URL, checks if it looks like a root path. This is a string check, ie. the return
/// value for a path of `"/foo/.."` would be `false`. An error will be thrown is this is a non-file URL.
package var isRoot: Bool {
get throws {
let checkPath = try filePath
Expand Down
14 changes: 12 additions & 2 deletions Sources/ToolchainRegistry/Toolchain.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,10 +366,20 @@ public final class Toolchain: Sendable {
}

/// Find a containing xctoolchain with plist, if available.
func containingXCToolchain(
private func containingXCToolchain(
_ path: URL
) -> (XCToolchainPlist, URL)? {
var path = path
// `deletingLastPathComponent` only makes sense on resolved paths (ie. those without symlinks or `..`). Any given
// toolchain path should have already been realpathed, but since this can turn into an infinite loop otherwise, it's
// better to be safe than sorry.
guard let resolvedPath = try? path.realpath else {
return nil
}
if path != resolvedPath {
logger.fault("\(path) was not realpathed")
}

var path = resolvedPath
while !((try? path.isRoot) ?? true) {
if path.pathExtension == "xctoolchain" {
if let infoPlist = orLog("Loading information from xctoolchain", { try XCToolchainPlist(fromDirectory: path) }) {
Expand Down
34 changes: 22 additions & 12 deletions Sources/ToolchainRegistry/ToolchainRegistry.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,13 @@ package final actor ToolchainRegistry {
var toolchainsByPath: [URL: Toolchain] = [:]
var toolchainsByCompiler: [URL: Toolchain] = [:]
for (toolchain, reason) in toolchainsAndReasonsParam {
// Toolchain should always be unique by path. It isn't particularly useful to log if we already have a toolchain
// though, as we could have just found toolchains through symlinks (this is actually quite normal - eg. OSS
// toolchains add a `swift-latest.xctoolchain` symlink on macOS).
if toolchainsByPath[toolchain.path] != nil {
continue
}

// Non-XcodeDefault toolchain: disallow all duplicates.
if toolchainsByIdentifier[toolchain.identifier] != nil,
toolchain.identifier != ToolchainRegistry.darwinDefaultToolchainIdentifier
Expand All @@ -104,12 +111,6 @@ package final actor ToolchainRegistry {
continue
}

// Toolchain should always be unique by path.
if toolchainsByPath[toolchain.path] != nil {
logger.fault("Found two toolchains with the same path: \(toolchain.path)")
continue
}

toolchainsByPath[toolchain.path] = toolchain
toolchainsByIdentifier[toolchain.identifier, default: []].append(toolchain)

Expand Down Expand Up @@ -219,7 +220,9 @@ package final actor ToolchainRegistry {
}

let toolchainsAndReasons = toolchainPaths.compactMap {
if let toolchain = Toolchain($0.path) {
if let resolvedPath = try? $0.path.realpath,
let toolchain = Toolchain(resolvedPath)
{
return (toolchain, $0.reason)
}
return nil
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

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.

return toolchainsByCompiler[resolvedPath]
Copy link
Member

Choose a reason for hiding this comment

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

We should run realpath when populating toolchainsByCompiler as well to cover symlinks within the toolchain, eg. swift and swiftc to swift-driver.

Copy link
Contributor Author

@bnbarham bnbarham Sep 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}
return nil
}

/// If we have a toolchain in the toolchain registry with the given URL, return it. Otherwise, return `nil`.
package func toolchain(withPath path: URL) -> Toolchain? {
if let resolvedPath = try? path.realpath {
return toolchainsByPath[resolvedPath]
}
return nil
Comment on lines +297 to +300
Copy link
Member

Choose a reason for hiding this comment

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

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

}
}

Expand All @@ -292,10 +306,6 @@ extension ToolchainRegistry {
package func toolchains(withIdentifier identifier: String) -> [Toolchain] {
return toolchainsByIdentifier[identifier] ?? []
}

package func toolchain(withPath path: URL) -> Toolchain? {
return toolchainsByPath[path]
}
}

extension ToolchainRegistry {
Expand Down
2 changes: 1 addition & 1 deletion Tests/ToolchainRegistryTests/ToolchainRegistryTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ final class ToolchainRegistryTests: XCTestCase {

try ProcessEnv.setVar(
"SOURCEKIT_PATH_FOR_TEST",
value: ["/bogus", binPath.filePath, "/bogus2"].joined(separator: separator)
value: ["/bogus/../parent", "/bogus", binPath.filePath, "/bogus2"].joined(separator: separator)
)
defer { try! ProcessEnv.setVar("SOURCEKIT_PATH_FOR_TEST", value: "") }

Expand Down