From d8161b371f3d045faa1cf784504612aeb82abc89 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Fri, 13 Sep 2024 09:19:23 -0700 Subject: [PATCH 1/4] Cache build targets by ID and associated depths We often need targets by ID, so cache it at that level and also simplify the cache of the target depths by computing it when getting build targets. --- .../BuildSystemManager.swift | 80 +++++++++++-------- 1 file changed, 47 insertions(+), 33 deletions(-) diff --git a/Sources/BuildSystemIntegration/BuildSystemManager.swift b/Sources/BuildSystemIntegration/BuildSystemManager.swift index e9aa11ea..5a15fe57 100644 --- a/Sources/BuildSystemIntegration/BuildSystemManager.swift +++ b/Sources/BuildSystemIntegration/BuildSystemManager.swift @@ -25,15 +25,15 @@ import struct TSCBasic.AbsolutePath import os #endif -fileprivate class RequestCache { - private var storage: [Request: Task] = [:] +fileprivate class RequestCache { + private var storage: [Request: Task] = [:] func get( _ key: Request, isolation: isolated any Actor = #isolation, - compute: @Sendable @escaping (Request) async throws(Error) -> Request.Response - ) async throws(Error) -> Request.Response { - let task: Task + compute: @Sendable @escaping (Request) async throws(Error) -> Result + ) async throws(Error) -> Result { + let task: Task if let cached = storage[key] { task = cached } else { @@ -110,15 +110,15 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate { /// Force-unwrapped optional because initializing it requires access to `self`. private var filesDependenciesUpdatedDebouncer: Debouncer>! = nil - private var cachedTargetsForDocument = RequestCache() + private var cachedTargetsForDocument = RequestCache() - private var cachedSourceKitOptions = RequestCache() + private var cachedSourceKitOptions = RequestCache() - private var cachedBuildTargets = RequestCache() + private var cachedBuildTargets = RequestCache< + BuildTargetsRequest, [BuildTargetIdentifier: (target: BuildTarget, depth: Int)] + >() - private var cachedTargetSources = RequestCache() - - private var cachedTargetDepths: (buildTargets: [BuildTarget], depths: [BuildTargetIdentifier: Int])? = nil + private var cachedTargetSources = RequestCache() /// The root of the project that this build system manages. For example, for SwiftPM packages, this is the folder /// containing Package.swift. For compilation databases it is the root folder based on which the compilation database @@ -222,10 +222,9 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate { if !options.backgroundIndexingOrDefault, events.contains(where: { $0.uri.fileURL?.pathExtension == "swiftmodule" }) { - let targets = await orLog("Getting build targets") { - try await self.buildTargets() + await orLog("Getting build targets") { + targetsWithUpdatedDependencies.formUnion(try await self.buildTargets().keys) } - targetsWithUpdatedDependencies.formUnion(targets?.map(\.id) ?? []) } var filesWithUpdatedDependencies: Set = [] @@ -490,9 +489,6 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate { /// The root targets of the project have depth of 0 and all target dependencies have a greater depth than the target // itself. private func targetDepths(for buildTargets: [BuildTarget]) -> [BuildTargetIdentifier: Int] { - if let cachedTargetDepths, cachedTargetDepths.buildTargets == buildTargets { - return cachedTargetDepths.depths - } var nonRoots: Set = [] for buildTarget in buildTargets { nonRoots.formUnion(buildTarget.dependencies) @@ -511,7 +507,6 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate { } } } - cachedTargetDepths = (buildTargets, depths) return depths } @@ -522,25 +517,25 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate { /// /// `nil` if the build system doesn't support topological sorting of targets. package func topologicalSort(of targets: [BuildTargetIdentifier]) async throws -> [BuildTargetIdentifier]? { - guard let workspaceTargets = await orLog("Getting build targets for topological sort", { try await buildTargets() }) + guard let buildTargets = await orLog("Getting build targets for topological sort", { try await buildTargets() }) else { return nil } - let depths = targetDepths(for: workspaceTargets) return targets.sorted { (lhs: BuildTargetIdentifier, rhs: BuildTargetIdentifier) -> Bool in - return depths[lhs, default: 0] > depths[rhs, default: 0] + return (buildTargets[lhs]?.depth ?? 0) > (buildTargets[rhs]?.depth ?? 0) } } /// Returns the list of targets that might depend on the given target and that need to be re-prepared when a file in /// `target` is modified. package func targets(dependingOn targetIds: Set) async -> [BuildTargetIdentifier] { - guard let buildTargets = await orLog("Getting build targets for dependencies", { try await self.buildTargets() }) + guard + let buildTargets = await orLog("Getting build targets for dependencies", { try await self.buildTargets().values }) else { return [] } - return buildTargets.filter { $0.dependencies.contains(anyIn: targetIds) }.map { $0.id } + return buildTargets.filter { $0.target.dependencies.contains(anyIn: targetIds) }.map { $0.target.id } } package func prepare( @@ -564,26 +559,46 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate { self.watchedFiles[uri] = nil } - package func buildTargets() async throws -> [BuildTarget] { + package func buildTargets() async throws -> [BuildTargetIdentifier: (target: BuildTarget, depth: Int)] { guard let buildSystem else { - return [] + return [:] } let request = BuildTargetsRequest() - let response = try await cachedBuildTargets.get(request) { request in - try await buildSystem.send(request) + let result = try await cachedBuildTargets.get(request) { request in + let buildTargets = try await buildSystem.send(request).targets + let depths = await self.targetDepths(for: buildTargets) + var result: [BuildTargetIdentifier: (target: BuildTarget, depth: Int)] = [:] + result.reserveCapacity(buildTargets.count) + for buildTarget in buildTargets { + guard result[buildTarget.id] == nil else { + logger.error("Found two targets with the same ID \(buildTarget.id)") + continue + } + let depth: Int + if let d = depths[buildTarget.id] { + depth = d + } else { + logger.fault("Did not compute depth for target \(buildTarget.id)") + depth = 0 + } + result[buildTarget.id] = (buildTarget, depth) + } + return result } - return response.targets + return result } - package func sourceFiles(in targets: [BuildTargetIdentifier]) async throws -> [SourcesItem] { + package func sourceFiles(in targets: some Sequence) async throws -> [SourcesItem] { guard let buildSystem else { return [] } // FIXME: (BSP migration) If we have a cached request for a superset of the targets, serve the result from that // cache entry. - let request = BuildTargetSourcesRequest.init(targets: targets) + // Sort targets to help cache hits if we have two calls to `sourceFiles` with targets in different orders. + let sortedTargets = targets.sorted { $0.uri.stringValue < $1.uri.stringValue } + let request = BuildTargetSourcesRequest(targets: sortedTargets) let response = try await cachedTargetSources.get(request) { request in try await buildSystem.send(request) } @@ -595,9 +610,8 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate { // retrieving the source files for those targets. // FIXME: (BSP Migration) Handle source files that are in multiple targets let targets = try await self.buildTargets() - let targetsById = Dictionary(elements: targets, keyedBy: \.id) - let sourceFiles = try await self.sourceFiles(in: targets.map(\.id)).flatMap { sourcesItem in - let target = targetsById[sourcesItem.target] + let sourceFiles = try await self.sourceFiles(in: targets.keys).flatMap { sourcesItem in + let target = targets[sourcesItem.target]?.target return sourcesItem.sources.map { sourceItem in SourceFileInfo( uri: sourceItem.uri, From 12923b6d73c316c689a35c7ad1ec023de95b04b6 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Sat, 24 Aug 2024 20:13:43 -0700 Subject: [PATCH 2/4] Implement `toolchain(for:)` using BSP --- .../BuildTargetsRequest.swift | 48 ++++++++++++++++--- .../BuildServerProtocol/InitializeBuild.swift | 2 +- .../BuildServerBuildSystem.swift | 4 -- .../BuildSystemManager.swift | 34 +++++++++++-- .../BuiltInBuildSystem.swift | 5 -- .../CompilationDatabaseBuildSystem.swift | 4 -- .../SwiftPMBuildSystem.swift | 13 +++-- .../TestBuildSystem.swift | 4 -- .../UpdateIndexStoreTaskDescription.swift | 2 +- Sources/SourceKitLSP/SourceKitLSPServer.swift | 13 +++-- 10 files changed, 92 insertions(+), 37 deletions(-) diff --git a/Sources/BuildServerProtocol/BuildTargetsRequest.swift b/Sources/BuildServerProtocol/BuildTargetsRequest.swift index 827c542d..d6a033e3 100644 --- a/Sources/BuildServerProtocol/BuildTargetsRequest.swift +++ b/Sources/BuildServerProtocol/BuildTargetsRequest.swift @@ -81,7 +81,9 @@ public struct BuildTarget: Codable, Hashable, Sendable { tags: [BuildTargetTag], capabilities: BuildTargetCapabilities, languageIds: [Language], - dependencies: [BuildTargetIdentifier] + dependencies: [BuildTargetIdentifier], + dataKind: BuildTargetDataKind? = nil, + data: LSPAny? = nil ) { self.id = id self.displayName = displayName @@ -90,6 +92,8 @@ public struct BuildTarget: Codable, Hashable, Sendable { self.capabilities = capabilities self.languageIds = languageIds self.dependencies = dependencies + self.dataKind = dataKind + self.data = data } } @@ -177,22 +181,52 @@ public struct BuildTargetDataKind: RawRepresentable, Codable, Hashable, Sendable } /// `data` field must contain a CargoBuildTarget object. - public static let cargo = "cargo" + public static let cargo = BuildTargetDataKind(rawValue: "cargo") /// `data` field must contain a CppBuildTarget object. - public static let cpp = "cpp" + public static let cpp = BuildTargetDataKind(rawValue: "cpp") /// `data` field must contain a JvmBuildTarget object. - public static let jvm = "jvm" + public static let jvm = BuildTargetDataKind(rawValue: "jvm") /// `data` field must contain a PythonBuildTarget object. - public static let python = "python" + public static let python = BuildTargetDataKind(rawValue: "python") /// `data` field must contain a SbtBuildTarget object. - public static let sbt = "sbt" + public static let sbt = BuildTargetDataKind(rawValue: "sbt") /// `data` field must contain a ScalaBuildTarget object. - public static let scala = "scala" + public static let scala = BuildTargetDataKind(rawValue: "scala") + + /// `data` field must contain a SourceKitBuildTarget object. + public static let sourceKit = BuildTargetDataKind(rawValue: "sourceKit") +} + +public struct SourceKitBuildTarget: LSPAnyCodable, Codable { + /// The toolchain that should be used to build this target. The URI should point to the directory that contains the + /// `usr` directory. On macOS, this is typically a bundle ending in `.xctoolchain`. If the toolchain is installed to + /// `/` on Linux, the toolchain URI would point to `/`. + /// + /// If no toolchain is given, SourceKit-LSP will pick a toolchain to use for this target. + public var toolchain: URI? + + public init(toolchain: URI? = nil) { + self.toolchain = toolchain + } + + public init(fromLSPDictionary dictionary: [String: LanguageServerProtocol.LSPAny]) { + if case .string(let toolchain) = dictionary[CodingKeys.toolchain.stringValue] { + self.toolchain = try? URI(string: toolchain) + } + } + + public func encodeToLSPAny() -> LanguageServerProtocol.LSPAny { + var result: [String: LSPAny] = [:] + if let toolchain { + result[CodingKeys.toolchain.stringValue] = .string(toolchain.stringValue) + } + return .dictionary(result) + } } /// The build target output paths request is sent from the client to the server diff --git a/Sources/BuildServerProtocol/InitializeBuild.swift b/Sources/BuildServerProtocol/InitializeBuild.swift index 5305b6a5..475a428b 100644 --- a/Sources/BuildServerProtocol/InitializeBuild.swift +++ b/Sources/BuildServerProtocol/InitializeBuild.swift @@ -104,7 +104,7 @@ public struct InitializeBuildResponseDataKind: RawRepresentable, Hashable, Codab } /// `data` field must contain a `SourceKitInitializeBuildResponseData` object. - public static let sourceKit = InitializeBuildResponseDataKind(rawValue: "sourcekit") + public static let sourceKit = InitializeBuildResponseDataKind(rawValue: "sourceKit") } public struct SourceKitInitializeBuildResponseData: LSPAnyCodable, Codable, Sendable { diff --git a/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift b/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift index 14d61141..9425e33c 100644 --- a/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift +++ b/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift @@ -330,10 +330,6 @@ extension BuildServerBuildSystem: BuiltInBuildSystem { ) } - package func toolchain(for uri: DocumentURI, _ language: Language) async -> Toolchain? { - return nil - } - package func waitForUpToDateBuildGraph() async {} package func addSourceFilesDidChangeCallback(_ callback: @escaping () async -> Void) { diff --git a/Sources/BuildSystemIntegration/BuildSystemManager.swift b/Sources/BuildSystemIntegration/BuildSystemManager.swift index 5a15fe57..60ba94a2 100644 --- a/Sources/BuildSystemIntegration/BuildSystemManager.swift +++ b/Sources/BuildSystemIntegration/BuildSystemManager.swift @@ -270,9 +270,37 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate { } /// Returns the toolchain that should be used to process the given document. - package func toolchain(for uri: DocumentURI, _ language: Language) async -> Toolchain? { - if let toolchain = await buildSystem?.underlyingBuildSystem.toolchain(for: uri, language) { - return toolchain + package func toolchain( + for uri: DocumentURI, + in target: BuildTargetIdentifier?, + language: Language + ) async -> Toolchain? { + let toolchainPath = await orLog("Getting toolchain from build targets") { () -> AbsolutePath? in + guard let target else { + return nil + } + let targets = try await self.buildTargets() + guard let target = targets[target]?.target else { + logger.error("Failed to find target \(target.forLogging) to determine toolchain") + return nil + } + guard target.dataKind == .sourceKit, case .dictionary(let data) = target.data else { + return nil + } + guard let toolchain = SourceKitBuildTarget(fromLSPDictionary: data).toolchain else { + return nil + } + guard let toolchainUrl = toolchain.fileURL else { + logger.error("Toolchain is not a file URL") + return nil + } + return try AbsolutePath(validating: toolchainUrl.path) + } + if let toolchainPath { + if let toolchain = await self.toolchainRegistry.toolchain(withPath: toolchainPath) { + return toolchain + } + logger.error("Toolchain at \(toolchainPath) not registered in toolchain registry.") } switch language { diff --git a/Sources/BuildSystemIntegration/BuiltInBuildSystem.swift b/Sources/BuildSystemIntegration/BuiltInBuildSystem.swift index e60e146f..59bcdf45 100644 --- a/Sources/BuildSystemIntegration/BuiltInBuildSystem.swift +++ b/Sources/BuildSystemIntegration/BuiltInBuildSystem.swift @@ -105,11 +105,6 @@ package protocol BuiltInBuildSystem: AnyObject, Sendable { /// Wait until the build graph has been loaded. func waitForUpToDateBuildGraph() async - /// The toolchain that should be used to open the given document. - /// - /// If `nil` is returned, then the default toolchain for the given language is used. - func toolchain(for uri: DocumentURI, _ language: Language) async -> Toolchain? - /// Adds a callback that should be called when the value returned by `sourceFiles()` changes. /// /// The callback might also be called without an actual change to `sourceFiles`. diff --git a/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift b/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift index d7b2af30..4ca7e36b 100644 --- a/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift +++ b/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift @@ -153,10 +153,6 @@ extension CompilationDatabaseBuildSystem: BuiltInBuildSystem { ) } - package func toolchain(for uri: DocumentURI, _ language: Language) async -> Toolchain? { - return nil - } - package func waitForUpToDateBuildGraph() async {} private func database(for uri: DocumentURI) -> CompilationDatabase? { diff --git a/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift b/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift index 0f61b80c..227ec258 100644 --- a/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift +++ b/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift @@ -130,7 +130,12 @@ extension BuildTargetIdentifier { return (target, runDestination) } } +} +fileprivate extension TSCBasic.AbsolutePath { + var asURI: DocumentURI? { + DocumentURI(self.asURL) + } } fileprivate let preparationTaskID: AtomicUInt32 = AtomicUInt32(initialValue: 0) @@ -531,7 +536,9 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem { capabilities: BuildTargetCapabilities(), // Be conservative with the languages that might be used in the target. SourceKit-LSP doesn't use this property. languageIds: [.c, .cpp, .objective_c, .objective_cpp, .swift], - dependencies: self.targetDependencies[targetId, default: []].sorted { $0.uri.stringValue < $1.uri.stringValue } + dependencies: self.targetDependencies[targetId, default: []].sorted { $0.uri.stringValue < $1.uri.stringValue }, + dataKind: .sourceKit, + data: SourceKitBuildTarget(toolchain: toolchain.path?.asURI).encodeToLSPAny() ) } return BuildTargetsResponse(targets: targets) @@ -595,10 +602,6 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem { ) } - package func toolchain(for uri: DocumentURI, _ language: Language) async -> Toolchain? { - return toolchain - } - package func targets(for uri: DocumentURI) -> [BuildTargetIdentifier] { guard let url = uri.fileURL, let path = try? AbsolutePath(validating: url.path) else { // We can't determine targets for non-file URIs. diff --git a/Sources/BuildSystemIntegration/TestBuildSystem.swift b/Sources/BuildSystemIntegration/TestBuildSystem.swift index b5c9b6d8..2de58ee1 100644 --- a/Sources/BuildSystemIntegration/TestBuildSystem.swift +++ b/Sources/BuildSystemIntegration/TestBuildSystem.swift @@ -70,10 +70,6 @@ package actor TestBuildSystem: BuiltInBuildSystem { return buildSettingsByFile[request.textDocument.uri] } - package func toolchain(for uri: DocumentURI, _ language: Language) async -> Toolchain? { - return nil - } - package func waitForUpToDateBuildGraph() async {} package func topologicalSort(of targets: [BuildTargetIdentifier]) -> [BuildTargetIdentifier]? { diff --git a/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift b/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift index d4bb8943..ce974ea7 100644 --- a/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift +++ b/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift @@ -255,7 +255,7 @@ package struct UpdateIndexStoreTaskDescription: IndexTaskDescription { logger.error("Not indexing \(file.forLogging) because it has fallback compiler arguments") return } - guard let toolchain = await buildSystemManager.toolchain(for: file.mainFile, language) else { + guard let toolchain = await buildSystemManager.toolchain(for: file.mainFile, in: target, language: language) else { logger.error( "Not updating index store for \(file.forLogging) because no toolchain could be determined for the document" ) diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift index 93d2d9bf..ff49499c 100644 --- a/Sources/SourceKitLSP/SourceKitLSPServer.swift +++ b/Sources/SourceKitLSP/SourceKitLSPServer.swift @@ -573,9 +573,16 @@ package actor SourceKitLSPServer { return service } - guard let toolchain = await workspace.buildSystemManager.toolchain(for: uri, language), - let service = await languageService(for: toolchain, language, in: workspace) - else { + let toolchain = await workspace.buildSystemManager.toolchain( + for: uri, + in: workspace.buildSystemManager.canonicalTarget(for: uri), + language: language + ) + guard let toolchain else { + logger.error("Failed to determine toolchain for \(uri)") + return nil + } + guard let service = await languageService(for: toolchain, language, in: workspace) else { logger.error("Failed to create language service for \(uri)") return nil } From fac21f5976d82047998d1b7ba97d149e1a1c2bd7 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Sat, 24 Aug 2024 20:40:46 -0700 Subject: [PATCH 3/4] Implement `waitForUpToDateBuildGraph` using BSP --- Sources/BuildServerProtocol/CMakeLists.txt | 3 ++- .../WaitForBuildSystemUpdates.swift | 25 +++++++++++++++++++ .../BuildServerBuildSystem.swift | 4 ++- .../BuildSystemManager.swift | 6 ++++- .../BuiltInBuildSystem.swift | 2 +- .../BuiltInBuildSystemAdapter.swift | 2 ++ .../CompilationDatabaseBuildSystem.swift | 4 ++- .../SwiftPMBuildSystem.swift | 3 ++- .../TestBuildSystem.swift | 4 ++- .../SwiftPMBuildSystemTests.swift | 4 +++ 10 files changed, 50 insertions(+), 7 deletions(-) create mode 100644 Sources/BuildServerProtocol/WaitForBuildSystemUpdates.swift diff --git a/Sources/BuildServerProtocol/CMakeLists.txt b/Sources/BuildServerProtocol/CMakeLists.txt index 8a6d9cc7..d02d8b87 100644 --- a/Sources/BuildServerProtocol/CMakeLists.txt +++ b/Sources/BuildServerProtocol/CMakeLists.txt @@ -10,7 +10,8 @@ add_library(BuildServerProtocol STATIC PrepareTargetsRequest.swift RegisterForChangeNotifications.swift ShutdownBuild.swift - SourceKitOptionsRequest.swift) + SourceKitOptionsRequest.swift + WaitForBuildSystemUpdates.swift) set_target_properties(BuildServerProtocol PROPERTIES INTERFACE_INCLUDE_DIRECTORIES ${CMAKE_Swift_MODULE_DIRECTORY}) target_link_libraries(BuildServerProtocol PRIVATE diff --git a/Sources/BuildServerProtocol/WaitForBuildSystemUpdates.swift b/Sources/BuildServerProtocol/WaitForBuildSystemUpdates.swift new file mode 100644 index 00000000..2da77353 --- /dev/null +++ b/Sources/BuildServerProtocol/WaitForBuildSystemUpdates.swift @@ -0,0 +1,25 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// + +import LanguageServerProtocol + +/// This request is a no-op and doesn't have any effects. +/// +/// If the build system is currently updating the build graph, this request should return after those updates have +/// finished processing. +public struct WaitForBuildSystemUpdatesRequest: RequestType, Hashable { + public typealias Response = VoidResponse + + public static let method: String = "workspace/waitForBuildSystemUpdates" + + public init() {} +} diff --git a/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift b/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift index 9425e33c..cd35fd81 100644 --- a/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift +++ b/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift @@ -330,7 +330,9 @@ extension BuildServerBuildSystem: BuiltInBuildSystem { ) } - package func waitForUpToDateBuildGraph() async {} + package func waitForUpBuildSystemUpdates(request: WaitForBuildSystemUpdatesRequest) async -> VoidResponse { + return VoidResponse() + } package func addSourceFilesDidChangeCallback(_ callback: @escaping () async -> Void) { // BuildServerBuildSystem does not support syntactic test discovery or background indexing. diff --git a/Sources/BuildSystemIntegration/BuildSystemManager.swift b/Sources/BuildSystemIntegration/BuildSystemManager.swift index 60ba94a2..f4516cf8 100644 --- a/Sources/BuildSystemIntegration/BuildSystemManager.swift +++ b/Sources/BuildSystemIntegration/BuildSystemManager.swift @@ -176,6 +176,8 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate { } await delegate.filesDependenciesUpdated(changedWatchedFiles) } + + // FIXME: (BSP migration) Forward file watch patterns from this initialize request to the client initializeResult = Task { () -> InitializeBuildResponse? in guard let buildSystem else { return nil @@ -511,7 +513,9 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate { } package func waitForUpToDateBuildGraph() async { - await self.buildSystem?.underlyingBuildSystem.waitForUpToDateBuildGraph() + await orLog("Waiting for build system updates") { + let _: VoidResponse? = try await self.buildSystem?.send(WaitForBuildSystemUpdatesRequest()) + } } /// The root targets of the project have depth of 0 and all target dependencies have a greater depth than the target diff --git a/Sources/BuildSystemIntegration/BuiltInBuildSystem.swift b/Sources/BuildSystemIntegration/BuiltInBuildSystem.swift index 59bcdf45..41ff1c80 100644 --- a/Sources/BuildSystemIntegration/BuiltInBuildSystem.swift +++ b/Sources/BuildSystemIntegration/BuiltInBuildSystem.swift @@ -103,7 +103,7 @@ package protocol BuiltInBuildSystem: AnyObject, Sendable { func sourceKitOptions(request: SourceKitOptionsRequest) async throws -> SourceKitOptionsResponse? /// Wait until the build graph has been loaded. - func waitForUpToDateBuildGraph() async + func waitForUpBuildSystemUpdates(request: WaitForBuildSystemUpdatesRequest) async -> VoidResponse /// Adds a callback that should be called when the value returned by `sourceFiles()` changes. /// diff --git a/Sources/BuildSystemIntegration/BuiltInBuildSystemAdapter.swift b/Sources/BuildSystemIntegration/BuiltInBuildSystemAdapter.swift index 89538f1a..682e4783 100644 --- a/Sources/BuildSystemIntegration/BuiltInBuildSystemAdapter.swift +++ b/Sources/BuildSystemIntegration/BuiltInBuildSystemAdapter.swift @@ -165,6 +165,8 @@ package actor BuiltInBuildSystemAdapter: BuiltInBuildSystemMessageHandler { return try await handle(request, underlyingBuildSystem.prepare) case let request as SourceKitOptionsRequest: return try await handle(request, underlyingBuildSystem.sourceKitOptions) + case let request as WaitForBuildSystemUpdatesRequest: + return try await handle(request, underlyingBuildSystem.waitForUpBuildSystemUpdates) default: throw ResponseError.methodNotFound(R.method) } diff --git a/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift b/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift index 4ca7e36b..28a9f227 100644 --- a/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift +++ b/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift @@ -153,7 +153,9 @@ extension CompilationDatabaseBuildSystem: BuiltInBuildSystem { ) } - package func waitForUpToDateBuildGraph() async {} + package func waitForUpBuildSystemUpdates(request: WaitForBuildSystemUpdatesRequest) async -> VoidResponse { + return VoidResponse() + } private func database(for uri: DocumentURI) -> CompilationDatabase? { if let url = uri.fileURL, let path = try? AbsolutePath(validating: url.path) { diff --git a/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift b/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift index 227ec258..a42f0807 100644 --- a/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift +++ b/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift @@ -629,8 +629,9 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem { return InverseSourcesResponse(targets: targets(for: request.textDocument.uri)) } - package func waitForUpToDateBuildGraph() async { + package func waitForUpBuildSystemUpdates(request: WaitForBuildSystemUpdatesRequest) async -> VoidResponse { await self.packageLoadingQueue.async {}.valuePropagatingCancellation + return VoidResponse() } package func prepare(request: PrepareTargetsRequest) async throws -> VoidResponse { diff --git a/Sources/BuildSystemIntegration/TestBuildSystem.swift b/Sources/BuildSystemIntegration/TestBuildSystem.swift index 2de58ee1..f0baad43 100644 --- a/Sources/BuildSystemIntegration/TestBuildSystem.swift +++ b/Sources/BuildSystemIntegration/TestBuildSystem.swift @@ -70,7 +70,9 @@ package actor TestBuildSystem: BuiltInBuildSystem { return buildSettingsByFile[request.textDocument.uri] } - package func waitForUpToDateBuildGraph() async {} + package func waitForUpBuildSystemUpdates(request: WaitForBuildSystemUpdatesRequest) async -> VoidResponse { + return VoidResponse() + } package func topologicalSort(of targets: [BuildTargetIdentifier]) -> [BuildTargetIdentifier]? { return nil diff --git a/Tests/BuildSystemIntegrationTests/SwiftPMBuildSystemTests.swift b/Tests/BuildSystemIntegrationTests/SwiftPMBuildSystemTests.swift index fa4afee9..75352924 100644 --- a/Tests/BuildSystemIntegrationTests/SwiftPMBuildSystemTests.swift +++ b/Tests/BuildSystemIntegrationTests/SwiftPMBuildSystemTests.swift @@ -37,6 +37,10 @@ fileprivate extension SwiftPMBuildSystem { request: SourceKitOptionsRequest(textDocument: TextDocumentIdentifier(uri: uri), target: target) ) } + + func waitForUpToDateBuildGraph() async { + let _: VoidResponse = await self.waitForUpBuildSystemUpdates(request: WaitForBuildSystemUpdatesRequest()) + } } final class SwiftPMBuildSystemTests: XCTestCase { From a96c0913ad2f79f6003231d03e81e01d9cc4cc6f Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Fri, 13 Sep 2024 11:07:55 -0700 Subject: [PATCH 4/4] Implement `addSourceFilesDidChangeCallback` in `BuildSystemManager` --- .../BuildServerBuildSystem.swift | 5 ----- .../BuildSystemDelegate.swift | 3 +++ .../BuildSystemManager.swift | 1 + .../BuiltInBuildSystem.swift | 5 ----- .../CompilationDatabaseBuildSystem.swift | 4 ---- .../SwiftPMBuildSystem.swift | 4 ---- .../BuildSystemIntegration/TestBuildSystem.swift | 2 -- Sources/SourceKitLSP/Workspace.swift | 14 +++++--------- .../BuildSystemManagerTests.swift | 2 ++ 9 files changed, 11 insertions(+), 29 deletions(-) diff --git a/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift b/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift index cd35fd81..ac9b9e4d 100644 --- a/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift +++ b/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift @@ -333,11 +333,6 @@ extension BuildServerBuildSystem: BuiltInBuildSystem { package func waitForUpBuildSystemUpdates(request: WaitForBuildSystemUpdatesRequest) async -> VoidResponse { return VoidResponse() } - - package func addSourceFilesDidChangeCallback(_ callback: @escaping () async -> Void) { - // BuildServerBuildSystem does not support syntactic test discovery or background indexing. - // (https://github.com/swiftlang/sourcekit-lsp/issues/1173). - } } private func loadBuildServerConfig(path: AbsolutePath, fileSystem: FileSystem) throws -> BuildServerConfig { diff --git a/Sources/BuildSystemIntegration/BuildSystemDelegate.swift b/Sources/BuildSystemIntegration/BuildSystemDelegate.swift index b37778e5..fc947f33 100644 --- a/Sources/BuildSystemIntegration/BuildSystemDelegate.swift +++ b/Sources/BuildSystemIntegration/BuildSystemDelegate.swift @@ -32,4 +32,7 @@ package protocol BuildSystemManagerDelegate: AnyObject, Sendable { /// Log the given message to the client's index log. func logMessageToIndexLog(taskID: IndexTaskID, message: String) + + /// Notify the delegate that the list of source files in the build system might have changed. + func sourceFilesDidChange() async } diff --git a/Sources/BuildSystemIntegration/BuildSystemManager.swift b/Sources/BuildSystemIntegration/BuildSystemManager.swift index f4516cf8..e0e2dadf 100644 --- a/Sources/BuildSystemIntegration/BuildSystemManager.swift +++ b/Sources/BuildSystemIntegration/BuildSystemManager.swift @@ -709,6 +709,7 @@ extension BuildSystemManager { // FIXME: (BSP Migration) Communicate that the build target has changed to the `BuildSystemManagerDelegate` and make // it responsible for figuring out which files are affected. await delegate?.fileBuildSettingsChanged(Set(watchedFiles.keys)) + await self.delegate?.sourceFilesDidChange() } private func logMessage(notification: BuildServerProtocol.LogMessageNotification) async { diff --git a/Sources/BuildSystemIntegration/BuiltInBuildSystem.swift b/Sources/BuildSystemIntegration/BuiltInBuildSystem.swift index 41ff1c80..0d9fd420 100644 --- a/Sources/BuildSystemIntegration/BuiltInBuildSystem.swift +++ b/Sources/BuildSystemIntegration/BuiltInBuildSystem.swift @@ -104,9 +104,4 @@ package protocol BuiltInBuildSystem: AnyObject, Sendable { /// Wait until the build graph has been loaded. func waitForUpBuildSystemUpdates(request: WaitForBuildSystemUpdatesRequest) async -> VoidResponse - - /// Adds a callback that should be called when the value returned by `sourceFiles()` changes. - /// - /// The callback might also be called without an actual change to `sourceFiles`. - func addSourceFilesDidChangeCallback(_ callback: @Sendable @escaping () async -> Void) async } diff --git a/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift b/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift index 28a9f227..2647332d 100644 --- a/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift +++ b/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift @@ -206,8 +206,4 @@ extension CompilationDatabaseBuildSystem: BuiltInBuildSystem { await testFilesDidChangeCallback() } } - - package func addSourceFilesDidChangeCallback(_ callback: @escaping () async -> Void) async { - testFilesDidChangeCallbacks.append(callback) - } } diff --git a/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift b/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift index a42f0807..95af08e6 100644 --- a/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift +++ b/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift @@ -795,10 +795,6 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem { } } - package func addSourceFilesDidChangeCallback(_ callback: @Sendable @escaping () async -> Void) async { - testFilesDidChangeCallbacks.append(callback) - } - /// Retrieve settings for a package manifest (Package.swift). private func settings(forPackageManifest path: AbsolutePath) throws -> SourceKitOptionsResponse? { let compilerArgs = workspace.interpreterFlags(for: path.parentDirectory) + [path.pathString] diff --git a/Sources/BuildSystemIntegration/TestBuildSystem.swift b/Sources/BuildSystemIntegration/TestBuildSystem.swift index f0baad43..d8f48d0a 100644 --- a/Sources/BuildSystemIntegration/TestBuildSystem.swift +++ b/Sources/BuildSystemIntegration/TestBuildSystem.swift @@ -77,6 +77,4 @@ package actor TestBuildSystem: BuiltInBuildSystem { package func topologicalSort(of targets: [BuildTargetIdentifier]) -> [BuildTargetIdentifier]? { return nil } - - package func addSourceFilesDidChangeCallback(_ callback: @escaping () async -> Void) async {} } diff --git a/Sources/SourceKitLSP/Workspace.swift b/Sources/SourceKitLSP/Workspace.swift index fa1fc6f7..c58ec8c1 100644 --- a/Sources/SourceKitLSP/Workspace.swift +++ b/Sources/SourceKitLSP/Workspace.swift @@ -130,15 +130,6 @@ package final class Workspace: Sendable, BuildSystemManagerDelegate { await indexDelegate?.addMainFileChangedCallback { [weak self] in await self?.buildSystemManager.mainFilesChanged() } - await buildSystemManager.buildSystem?.underlyingBuildSystem.addSourceFilesDidChangeCallback { [weak self] in - guard let self else { - return - } - guard let testFiles = await orLog("Getting test files", { try await self.buildSystemManager.testFiles() }) else { - return - } - await self.syntacticTestIndex.listOfTestFilesDidChange(testFiles) - } // Trigger an initial population of `syntacticTestIndex`. if let testFiles = await orLog("Getting initial test files", { try await self.buildSystemManager.testFiles() }) { await syntacticTestIndex.listOfTestFilesDidChange(testFiles) @@ -328,6 +319,11 @@ package final class Workspace: Sendable, BuildSystemManagerDelegate { package func logMessageToIndexLog(taskID: IndexTaskID, message: String) { self.logMessageToIndexLogCallback(taskID, message) } + + package func sourceFilesDidChange() async { + let testFiles = await orLog("Getting test files") { try await buildSystemManager.testFiles() } ?? [] + await syntacticTestIndex.listOfTestFilesDidChange(testFiles) + } } /// Wrapper around a workspace that isn't being retained. diff --git a/Tests/BuildSystemIntegrationTests/BuildSystemManagerTests.swift b/Tests/BuildSystemIntegrationTests/BuildSystemManagerTests.swift index b11a4858..3728ec59 100644 --- a/Tests/BuildSystemIntegrationTests/BuildSystemManagerTests.swift +++ b/Tests/BuildSystemIntegrationTests/BuildSystemManagerTests.swift @@ -406,4 +406,6 @@ private actor BSMDelegate: BuildSystemManagerDelegate { func buildTargetsChanged(_ changes: [BuildTargetEvent]?) async {} nonisolated func logMessageToIndexLog(taskID: BuildSystemIntegration.IndexTaskID, message: String) {} + + func sourceFilesDidChange() async {} }