Merge pull request #831 from ahoppen/ahoppen/clean-up-build-system

ahoppen · web-flow · commit 8f9fb53e8f8d · 2023-09-26T13:50:39.000-07:00
Clean up `BuildSystemManager`
diff --git a/Sources/SKCore/BuildServerBuildSystem.swift b/Sources/SKCore/BuildServerBuildSystem.swift
@@ -43,14 +43,15 @@ func executable(_ name: String) -> String {
 ///
 /// Provides build settings from a build server launched based on a
 /// `buildServer.json` configuration file provided in the repo root.
-public final class BuildServerBuildSystem {
+public final class BuildServerBuildSystem: MessageHandler {
+  /// The handler's request queue. `delegate` will always be called on this queue.
+  public let queue: DispatchQueue = DispatchQueue(label: "language-server-queue", qos: .userInitiated)
 
   let projectRoot: AbsolutePath
   let buildFolder: AbsolutePath?
   let serverConfig: BuildServerConfig
   let requestQueue: DispatchQueue
 
-  var handler: BuildServerHandler?
   var buildServer: JSONRPCConnection?
 
   let searchPaths: [AbsolutePath]
@@ -62,10 +63,7 @@ public final class BuildServerBuildSystem {
   public var indexPrefixMappings: [PathPrefixMapping] { return [] }
 
   /// Delegate to handle any build system events.
-  public weak var delegate: BuildSystemDelegate? {
-    get { return self.handler?.delegate }
-    set { self.handler?.delegate = newValue }
-  }
+  public weak var delegate: BuildSystemDelegate?
 
   public init(projectRoot: AbsolutePath, buildFolder: AbsolutePath?, fileSystem: FileSystem = localFileSystem) throws {
     let configPath = projectRoot.appending(component: "buildServer.json")
@@ -146,8 +144,7 @@ public final class BuildServerBuildSystem {
       rootUri: URI(self.projectRoot.asURL),
       capabilities: BuildClientCapabilities(languageIds: languages))
 
-    let handler = BuildServerHandler()
-    let buildServer = try makeJSONRPCBuildServer(client: handler, serverPath: serverPath, serverFlags: flags)
+    let buildServer = try makeJSONRPCBuildServer(client: self, serverPath: serverPath, serverFlags: flags)
     let response = try buildServer.sendSync(initializeRequest)
     buildServer.send(InitializedBuildNotification())
     log("initialized build server \(response.displayName)")
@@ -160,34 +157,13 @@ public final class BuildServerBuildSystem {
       self.indexStorePath = try AbsolutePath(validating: indexStorePath, relativeTo: self.projectRoot)
     }
     self.buildServer = buildServer
-    self.handler = handler
-  }
-}
-
-private func readReponseDataKey(data: LSPAny?, key: String) -> String? {
-  if case .dictionary(let dataDict)? = data,
-    case .string(let stringVal)? = dataDict[key] {
-    return stringVal
   }
 
-  return nil
-}
-
-/// A handler that receives messages from the build server. 
-///
-/// In practice, it listens for updated build settings and informs the 
-/// ``BuildSystemDelegate`` about these changes.
-final class BuildServerHandler: MessageHandler {
-  /// The handler's request queue. `delegate` will always be called on this queue.
-  public let queue: DispatchQueue = DispatchQueue(label: "language-server-queue", qos: .userInitiated)
-
-  public weak var delegate: BuildSystemDelegate? = nil
-
   /// Handler for notifications received **from** the builder server, ie.
   /// the build server has sent us a notification.
   ///
   /// We need to notify the delegate about any updated build settings.
-  func handle(_ params: some NotificationType, from clientID: ObjectIdentifier) {
+  public func handle(_ params: some NotificationType, from clientID: ObjectIdentifier) {
     queue.async {
       if let params = params as? BuildTargetsChangedNotification {
         self.handleBuildTargetsChanged(Notification(params, clientID: clientID))
@@ -200,7 +176,7 @@ final class BuildServerHandler: MessageHandler {
   /// Handler for requests received **from** the build server.
   ///
   /// We currently can't handle any requests sent from the build server to us.
-  func handle<R: RequestType>(
+  public func handle<R: RequestType>(
     _ params: R,
     id: RequestID,
     from clientID: ObjectIdentifier,
@@ -223,6 +199,15 @@ final class BuildServerHandler: MessageHandler {
   }
 }
 
+private func readReponseDataKey(data: LSPAny?, key: String) -> String? {
+  if case .dictionary(let dataDict)? = data,
+    case .string(let stringVal)? = dataDict[key] {
+    return stringVal
+  }
+
+  return nil
+}
+
 extension BuildServerBuildSystem {
   /// Exposed for *testing*.
   public func _settings(for uri: DocumentURI) -> FileBuildSettings? {
@@ -261,41 +246,6 @@ extension BuildServerBuildSystem: BuildSystem {
     })
   }
 
-  public func buildTargets(reply: @escaping (LSPResult<[BuildTarget]>) -> Void) {
-    _ = self.buildServer?.send(BuildTargets(), queue: requestQueue) { response in
-      switch response {
-      case .success(let result):
-        reply(.success(result.targets))
-      case .failure(let error):
-        reply(.failure(error))
-      }
-    }
-  }
-
-  public func buildTargetSources(targets: [BuildTargetIdentifier], reply: @escaping (LSPResult<[SourcesItem]>) -> Void) {
-    let req = BuildTargetSources(targets: targets)
-    _ = self.buildServer?.send(req, queue: requestQueue) { response in
-      switch response {
-      case .success(let result):
-        reply(.success(result.items))
-      case .failure(let error):
-        reply(.failure(error))
-      }
-    }
-  }
-
-  public func buildTargetOutputPaths(targets: [BuildTargetIdentifier], reply: @escaping (LSPResult<[OutputsItem]>) -> Void) {
-    let req = BuildTargetOutputPaths(targets: targets)
-    _ = self.buildServer?.send(req, queue: requestQueue) { response in
-      switch response {
-      case .success(let result):
-        reply(.success(result.items))
-      case .failure(let error):
-        reply(.failure(error))
-      }
-    }
-  }
-
   public func filesDidChange(_ events: [FileEvent]) {}
 
   public func fileHandlingCapability(for uri: DocumentURI) -> FileHandlingCapability {
diff --git a/Sources/SKCore/BuildSystem.swift b/Sources/SKCore/BuildSystem.swift
@@ -63,19 +63,6 @@ public protocol BuildSystem: AnyObject {
   /// such as command line flag changes, dependency changes, etc.
   func unregisterForChangeNotifications(for: DocumentURI)
 
-  /// Returns the build targets in the workspace. If the build system does not
-  /// support build targets the `buildTargetsNotSupported` error should be
-  /// returned.
-  func buildTargets(reply: @escaping (LSPResult<[BuildTarget]>) -> Void)
-
-  /// Returns the sources for the requested build targets
-  func buildTargetSources(targets: [BuildTargetIdentifier],
-                          reply: @escaping (LSPResult<[SourcesItem]>) -> Void)
-
-  /// Returns the output paths for the requested build targets
-  func buildTargetOutputPaths(targets: [BuildTargetIdentifier],
-                              reply: @escaping (LSPResult<[OutputsItem]>) -> Void)
-
   /// Called when files in the project change.
   func filesDidChange(_ events: [FileEvent])
 
diff --git a/Sources/SKCore/BuildSystemManager.swift b/Sources/SKCore/BuildSystemManager.swift
@@ -135,14 +135,7 @@ public final class BuildSystemManager {
   }
 }
 
-extension BuildSystemManager: BuildSystem {
-
-  public var indexStorePath: AbsolutePath? {  queue.sync { buildSystem?.indexStorePath } }
-
-  public var indexDatabasePath: AbsolutePath? { queue.sync { buildSystem?.indexDatabasePath } }
-
-  public var indexPrefixMappings: [PathPrefixMapping] { queue.sync { buildSystem?.indexPrefixMappings ?? [] } }
-
+extension BuildSystemManager {
   public var delegate: BuildSystemDelegate? {
     get { queue.sync { _delegate } }
     set { queue.sync { _delegate = newValue } }
@@ -319,42 +312,6 @@ extension BuildSystemManager: BuildSystem {
     }
   }
 
-  public func buildTargets(reply: @escaping (LSPResult<[BuildTarget]>) -> Void) {
-    queue.async {
-      if let buildSystem = self.buildSystem {
-        buildSystem.buildTargets(reply: reply)
-      } else {
-        reply(.success([]))
-      }
-    }
-  }
-
-  public func buildTargetSources(
-    targets: [BuildTargetIdentifier],
-    reply: @escaping (LSPResult<[SourcesItem]>) -> Void)
-  {
-    queue.async {
-      if let buildSystem = self.buildSystem {
-        buildSystem.buildTargetSources(targets: targets, reply: reply)
-      } else {
-        reply(.success([]))
-      }
-    }
-  }
-
-  public func buildTargetOutputPaths(
-    targets: [BuildTargetIdentifier],
-    reply: @escaping (LSPResult<[OutputsItem]>) -> Void)
-  {
-    queue.async {
-      if let buildSystem = self.buildSystem {
-        buildSystem.buildTargetOutputPaths(targets: targets, reply: reply)
-      } else {
-        reply(.success([]))
-      }
-    }
-  }
-
   public func fileHandlingCapability(for uri: DocumentURI) -> FileHandlingCapability {
     return max(buildSystem?.fileHandlingCapability(for: uri) ?? .unhandled, fallbackBuildSystem?.fileHandlingCapability(for: uri) ?? .unhandled)
   }
diff --git a/Sources/SKCore/CompilationDatabaseBuildSystem.swift b/Sources/SKCore/CompilationDatabaseBuildSystem.swift
@@ -111,18 +111,6 @@ extension CompilationDatabaseBuildSystem: BuildSystem {
     }
   }
 
-  public func buildTargets(reply: @escaping (LSPResult<[BuildTarget]>) -> Void) {
-    reply(.failure(buildTargetsNotSupported))
-  }
-
-  public func buildTargetSources(targets: [BuildTargetIdentifier], reply: @escaping (LSPResult<[SourcesItem]>) -> Void) {
-    reply(.failure(buildTargetsNotSupported))
-  }
-
-  public func buildTargetOutputPaths(targets: [BuildTargetIdentifier], reply: @escaping (LSPResult<[OutputsItem]>) -> Void) {
-    reply(.failure(buildTargetsNotSupported))
-  }
-
   /// Must be invoked on `queue`.
   private func database(for url: URL) -> CompilationDatabase? {
     dispatchPrecondition(condition: .onQueue(queue))
diff --git a/Sources/SKCore/FallbackBuildSystem.swift b/Sources/SKCore/FallbackBuildSystem.swift
@@ -67,18 +67,6 @@ public final class FallbackBuildSystem: BuildSystem {
   /// We don't support change watching.
   public func unregisterForChangeNotifications(for: DocumentURI) {}
 
-  public func buildTargets(reply: @escaping (LSPResult<[BuildTarget]>) -> Void) {
-    reply(.failure(buildTargetsNotSupported))
-  }
-
-  public func buildTargetSources(targets: [BuildTargetIdentifier], reply: @escaping (LSPResult<[SourcesItem]>) -> Void) {
-    reply(.failure(buildTargetsNotSupported))
-  }
-
-  public func buildTargetOutputPaths(targets: [BuildTargetIdentifier], reply: @escaping (LSPResult<[OutputsItem]>) -> Void) {
-    reply(.failure(buildTargetsNotSupported))
-  }
-
   func settingsSwift(_ file: String) -> FileBuildSettings {
     var args: [String] = []
     args.append(contentsOf: self.buildSetup.flags.swiftCompilerFlags)
diff --git a/Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift b/Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift
@@ -347,19 +347,6 @@ extension SwiftPMWorkspace: SKCore.BuildSystem {
     }
   }
 
-  public func buildTargets(reply: @escaping (LSPResult<[BuildTarget]>) -> Void) {
-    // TODO: Support for build targets
-    reply(.failure(buildTargetsNotSupported))
-  }
-
-  public func buildTargetSources(targets: [BuildTargetIdentifier], reply: @escaping (LSPResult<[SourcesItem]>) -> Void) {
-    reply(.failure(buildTargetsNotSupported))
-  }
-
-  public func buildTargetOutputPaths(targets: [BuildTargetIdentifier], reply: @escaping (LSPResult<[OutputsItem]>) -> Void) {
-    reply(.failure(buildTargetsNotSupported))
-  }
-
   /// Returns the resolved target description for the given file, if one is known.
   /// Must only be called on `queue`.
   private func targetDescription(for file: AbsolutePath) throws -> TargetBuildDescription? {
diff --git a/Tests/SKCoreTests/BuildServerBuildSystemTests.swift b/Tests/SKCoreTests/BuildServerBuildSystemTests.swift
@@ -74,92 +74,6 @@ final class BuildServerBuildSystemTests: XCTestCase {
     XCTAssertEqual(XCTWaiter.wait(for: [expectation], timeout: defaultTimeout), .completed)
   }
 
-  func testBuildTargets() throws {
-    let buildSystem = try BuildServerBuildSystem(projectRoot: root, buildFolder: buildFolder)
-
-    let expectation = XCTestExpectation(description: "build target expectation")
-
-    buildSystem.buildTargets(reply: { response in
-      switch(response) {
-      case .success(let targets):
-        XCTAssertEqual(targets, [
-                       BuildTarget(id: BuildTargetIdentifier(uri: DocumentURI(string: "target:first_target")),
-                                   displayName: "First Target",
-                                   baseDirectory: DocumentURI(URL(fileURLWithPath: "/some/dir")),
-                                   tags: [BuildTargetTag.library, BuildTargetTag.test],
-                                   capabilities: BuildTargetCapabilities(canCompile: true, canTest: true, canRun: false),
-                                   languageIds: [Language.objective_c, Language.swift],
-                                   dependencies: []),
-                       BuildTarget(id: BuildTargetIdentifier(uri: DocumentURI(string: "target:second_target")),
-                                   displayName: "Second Target",
-                                   baseDirectory: DocumentURI(URL(fileURLWithPath: "/some/dir")),
-                                   tags: [BuildTargetTag.library, BuildTargetTag.test],
-                                   capabilities: BuildTargetCapabilities(canCompile: true, canTest: false, canRun: false),
-                                   languageIds: [Language.objective_c, Language.swift],
-                                   dependencies: [BuildTargetIdentifier(uri: DocumentURI(string: "target:first_target"))]),
-                       ])
-        expectation.fulfill()
-      case .failure(let error):
-        XCTFail(error.message)
-      }
-    })
-    XCTAssertEqual(XCTWaiter.wait(for: [expectation], timeout: defaultTimeout), .completed)
-  }
-
-  func testBuildTargetSources() throws {
-    let buildSystem = try BuildServerBuildSystem(projectRoot: root, buildFolder: buildFolder)
-
-    let expectation = XCTestExpectation(description: "build target sources expectation")
-    let targets = [
-      BuildTargetIdentifier(uri: DocumentURI(string: "build://target/a")),
-      BuildTargetIdentifier(uri: DocumentURI(string: "build://target/b")),
-    ]
-    buildSystem.buildTargetSources(targets: targets, reply: { response in
-      switch(response) {
-      case .success(let items):
-        XCTAssertNotNil(items)
-        XCTAssertEqual(items[0].target.uri, targets[0].uri)
-        XCTAssertEqual(items[1].target.uri, targets[1].uri)
-        XCTAssertEqual(items[0].sources[0].uri, DocumentURI(URL(fileURLWithPath: "/path/to/a/file")))
-        XCTAssertEqual(items[0].sources[0].kind, SourceItemKind.file)
-        XCTAssertEqual(items[0].sources[1].uri, DocumentURI(URL(fileURLWithPath: "/path/to/a/folder", isDirectory: true)))
-        XCTAssertEqual(items[0].sources[1].kind, SourceItemKind.directory)
-        XCTAssertEqual(items[1].sources[0].uri, DocumentURI(URL(fileURLWithPath: "/path/to/b/file")))
-        XCTAssertEqual(items[1].sources[0].kind, SourceItemKind.file)
-        XCTAssertEqual(items[1].sources[1].uri, DocumentURI(URL(fileURLWithPath: "/path/to/b/folder", isDirectory: true)))
-        XCTAssertEqual(items[1].sources[1].kind, SourceItemKind.directory)
-        expectation.fulfill()
-      case .failure(let error):
-        XCTFail(error.message)
-      }
-    })
-    XCTAssertEqual(XCTWaiter.wait(for: [expectation], timeout: defaultTimeout), .completed)
-  }
-
-  func testBuildTargetOutputs() throws {
-    let buildSystem = try BuildServerBuildSystem(projectRoot: root, buildFolder: buildFolder)
-
-    let expectation = XCTestExpectation(description: "build target output expectation")
-    let targets = [
-      BuildTargetIdentifier(uri: DocumentURI(string: "build://target/a")),
-    ]
-    buildSystem.buildTargetOutputPaths(targets: targets, reply: { response in
-      switch(response) {
-      case .success(let items):
-        XCTAssertNotNil(items)
-        XCTAssertEqual(items[0].target.uri, targets[0].uri)
-        XCTAssertEqual(items[0].outputPaths, [
-          DocumentURI(URL(fileURLWithPath: "/path/to/a/file")),
-          DocumentURI(URL(fileURLWithPath: "/path/to/a/file2")),
-        ])
-        expectation.fulfill()
-      case .failure(let error):
-        XCTFail(error.message)
-      }
-    })
-    XCTAssertEqual(XCTWaiter.wait(for: [expectation], timeout: defaultTimeout), .completed)
-  }
-
   func testBuildTargetsChanged() throws {
     let buildSystem = try BuildServerBuildSystem(projectRoot: root, buildFolder: buildFolder)
 
diff --git a/Tests/SKCoreTests/BuildSystemManagerTests.swift b/Tests/SKCoreTests/BuildSystemManagerTests.swift
@@ -491,20 +491,6 @@ class ManualBuildSystem: BuildSystem {
   var indexDatabasePath: AbsolutePath? { nil }
   var indexPrefixMappings: [PathPrefixMapping] { return [] }
 
-  func buildTargets(reply: @escaping (LSPResult<[BuildTarget]>) -> Void) {
-    fatalError()
-  }
-
-  func buildTargetSources(targets: [BuildTargetIdentifier],
-    reply: @escaping (LSPResult<[SourcesItem]>) -> Void) {
-    fatalError()
-  }
-
-  func buildTargetOutputPaths(targets: [BuildTargetIdentifier],
-    reply: @escaping (LSPResult<[OutputsItem]>) -> Void) {
-    fatalError()
-  }
-
   func filesDidChange(_ events: [FileEvent]) {}
 
   public func fileHandlingCapability(for uri: DocumentURI) -> FileHandlingCapability {
diff --git a/Tests/SourceKitLSPTests/BuildSystemTests.swift b/Tests/SourceKitLSPTests/BuildSystemTests.swift
