Add support for a compile database file not at the root of a workspace #915

GeorgeLyon · 2023-10-20T19:48:53Z

Introduce a new command-line flag (--compile-db-search-path) to sourcekit-lsp, which can be used to specify which subdirectories in a workspace should be searched for a clangd-style compile database (compile_commands.json or compile_flags.txt). Originally, I was going to add a clangd-like --compile-commands-dir flag, but upon further investigation decided this wasn't a good approach, as sourcekit-lsp supports multiple workspaces and clangd does not. In a world with multiple workspaces it does not make sense to specify a single absolute path to a compile-database, so instead I opted for a set of relative search paths, which apply to all databases managed by a particular instance of sourcekit-lsp.

Sources/SKCore/CompilationDatabase.swift

GeorgeLyon · 2023-10-20T19:56:30Z

Sources/SKTestSupport/SKSwiftPMTestWorkspace.swift

@@ -55,7 +55,7 @@ public final class SKSwiftPMTestWorkspace {
  /// Connection to the language server.
  public let testClient: TestSourceKitLSPClient

-  /// When `testServer` is not `nil`, the workspace will be opened in that server, otherwise a new server will be created for the workspace
+  /// When `testClient` is not `nil`, the workspace will be opened in that client's server, otherwise a new client will be created for the workspace


Not really related, but I was confused by the wording here when browsing the codebase.

Oh, I recently changed testServer to testClient and looks like I forgot that comment. Thanks for updating it.

GeorgeLyon · 2023-10-20T20:12:48Z

Sources/sourcekit-lsp/SourceKitLSP.swift

+  var compilationDatabaseSearchPaths = try! [
+    // These defaults match the behavior of clangd
+    RelativePath(validating: "."),
+    RelativePath(validating: "build"),


It may make sense to pull these out and make the search paths "additional" so the defaults persist even in the presence of custom search paths.

Yes, I think that would make sense. IMO the search precedence should be user-defined search paths, ., build.

ahoppen

Nice. Thanks for opening a PR for this so quickly 🤩.

I would like to think about the naming of the command line option a little bit more. I’m not unconditionally happy with it but don’t have any better ideas either, so maybe it’s fine. @bnbarham Maybe you’ve got an opinion here as well.

Sources/SKCore/CompilationDatabase.swift

ahoppen · 2023-10-20T22:11:57Z

Sources/SKCore/CompilationDatabase.swift

+      try?
+        (JSONCompilationDatabase(directory: $0, fileSystem)
+        ?? FixedCompilationDatabase(directory: $0, fileSystem))


If the initializers now return nil when the file doesn’t exist, I think we no longer need to do try? here and can change it to try. That way we don’t silently ignore errors anymore.

I looked into this but it is actually a bigger change than it seems as the throws will propagate all the way up to things like the BuildSystem protocol, so you may want to do that separately (if at all). In the meantime I made it try! since decoding errors should be very infrequent and shouldn't fail silently.

ahoppen · 2023-10-20T22:12:56Z

Sources/SKTestSupport/SKSwiftPMTestWorkspace.swift

@@ -55,7 +55,7 @@ public final class SKSwiftPMTestWorkspace {
  /// Connection to the language server.
  public let testClient: TestSourceKitLSPClient

-  /// When `testServer` is not `nil`, the workspace will be opened in that server, otherwise a new server will be created for the workspace
+  /// When `testClient` is not `nil`, the workspace will be opened in that client's server, otherwise a new client will be created for the workspace


Oh, I recently changed testServer to testClient and looks like I forgot that comment. Thanks for updating it.

ahoppen · 2023-10-20T22:16:29Z

Sources/sourcekit-lsp/SourceKitLSP.swift

@@ -137,6 +150,18 @@ struct SourceKitLSP: ParsableCommand {
  )
  var indexPrefixMappings = [PathPrefixMapping]()

+  @Option(
+    name: .customLong("compilation-db-search-path", withSingleDash: false),
+    parsing: .unconditionalSingleValue,


Any reason why you decided to choose unconditionalSingleValue here instead of the default singleValue?

No strong reason, though now that I think about it singleValue might be better as --compilation-db-search-path --some-other-flag may have a better compilation error.

That’s what I thought as well.

ahoppen · 2023-10-20T22:18:28Z

Tests/SourceKitLSPTests/CompilationDatabaseTests.swift

+      let compilationDatabase = try JSONCompilationDatabase(
+        file: AbsolutePath(validating: compilationDatabaseUrl.path)
+      )!
      let newCommands = compilationDatabase.allCommands.map {


Could you use XCTUnwrap instead of ! here so the test doesn’t crash if the database is nil?

ahoppen · 2023-10-20T22:19:28Z

Sources/sourcekit-lsp/SourceKitLSP.swift

@@ -137,6 +150,18 @@ struct SourceKitLSP: ParsableCommand {
  )
  var indexPrefixMappings = [PathPrefixMapping]()

+  @Option(
+    name: .customLong("compilation-db-search-path", withSingleDash: false),


withSingleDash defaults to false, so we don’t need to specify it.

ahoppen · 2023-10-20T22:21:35Z

Sources/sourcekit-lsp/SourceKitLSP.swift

+  var compilationDatabaseSearchPaths = try! [
+    // These defaults match the behavior of clangd
+    RelativePath(validating: "."),
+    RelativePath(validating: "build"),


Yes, I think that would make sense. IMO the search precedence should be user-defined search paths, ., build.

Sources/sourcekit-lsp/SourceKitLSP.swift

bnbarham · 2023-10-20T23:20:25Z

I would like to think about the naming of the command line option a little bit more. I’m not unconditionally happy with it but don’t have any better ideas either, so maybe it’s fine. @bnbarham Maybe you’ve got an opinion here as well.

No particularly strong ones. compile-commands-search-path maybe?

GeorgeLyon · 2023-10-20T23:47:29Z

No particularly strong ones. compile-commands-search-path maybe?

This confused me when I was looking into this. I was used to "compile_commands.json", but the term "compile database" is more accurate (and what is used in the sourcekit-lsp code itself), since you could use "compile_flags.txt" as well. If we were being 100% pedantic (and we are all Swift developers, no? 😜) it would be something like "clang-style-compile-database-search-path", but even that may not be 100% correct. I think the current form is a good balance between being specific and being correct and not being too wordy, and the help text is pretty explicit which helps.

Sources/SKCore/CompilationDatabase.swift

ahoppen · 2023-10-21T01:22:52Z

Sources/sourcekit-lsp/SourceKitLSP.swift

@@ -137,6 +150,18 @@ struct SourceKitLSP: ParsableCommand {
  )
  var indexPrefixMappings = [PathPrefixMapping]()

+  @Option(
+    name: .customLong("compilation-db-search-path", withSingleDash: false),
+    parsing: .unconditionalSingleValue,


That’s what I thought as well.

ahoppen · 2023-10-21T01:24:22Z

No particularly strong ones. compile-commands-search-path maybe?

I think compilation-db-search-path is better because it also handles compile_flags.txt. For the lack of better ideas, let’s leave it for now.

ahoppen

I have a couple more minor formatting comments inline.

ahoppen

Just one more note on the doc comments.

Also: Could you squash your commits? It makes for a nicer git history.

ahoppen · 2023-10-21T04:31:34Z

Sources/SKCore/CompilationDatabase.swift

    let path = directory.appending(component: "compile_flags.txt")
    try self.init(file: path, fileSystem)
  }

-  public init(file: AbsolutePath, _ fileSystem: FileSystem = localFileSystem) throws {
+  /// Loads the compilation database from `file`
+  /// returns: `nil` if the file does not exist


Could you also update the other returns: lines to use docc markup?

GeorgeLyon · 2023-10-21T05:18:11Z

Updated comments, and squash-force0pushed... do you guys not use GitHub's "squash and merge"?

ahoppen · 2023-10-23T16:08:54Z

Updated comments, and squash-force0pushed... do you guys not use GitHub's "squash and merge"?

I prefer not to squash and merge for the reasons outlined here: https://github.com/apple/swift-syntax/blob/main/CONTRIBUTING.md#authoring-commits

ahoppen

Nice. Let’s get this in 🚢

ahoppen · 2023-10-23T16:09:13Z

@swift-ci Please test

ahoppen · 2023-10-23T17:45:35Z

@swift-ci Please test Windows

GeorgeLyon · 2023-10-24T14:45:43Z

Thanks for working with me on this @ahoppen!

ahoppen · 2023-10-24T16:13:49Z

Thanks for your contribution!

GeorgeLyon commented Oct 20, 2023

View reviewed changes

GeorgeLyon marked this pull request as ready for review October 20, 2023 19:59

GeorgeLyon requested review from benlangmuir and ahoppen as code owners October 20, 2023 19:59

GeorgeLyon mentioned this pull request Oct 20, 2023

Add a VSCode devcontainer definition #916

Closed

GeorgeLyon commented Oct 20, 2023

View reviewed changes

ahoppen reviewed Oct 20, 2023

View reviewed changes

ahoppen reviewed Oct 21, 2023

View reviewed changes

Add support for compile databases not at the root of the workspace

170b44b

GeorgeLyon force-pushed the dev/george/compilation-db-search-paths branch from 65b2571 to 170b44b Compare October 21, 2023 05:17

ahoppen approved these changes Oct 23, 2023

View reviewed changes

ahoppen merged commit 645b511 into apple:main Oct 24, 2023
3 checks passed

GeorgeLyon deleted the dev/george/compilation-db-search-paths branch October 24, 2023 14:45

GeorgeLyon mentioned this pull request Oct 25, 2023

sourcekit-lsp server does not start if opening a C++ file. swift-server/vscode-swift#630

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for a compile database file not at the root of a workspace #915

Add support for a compile database file not at the root of a workspace #915

GeorgeLyon commented Oct 20, 2023

GeorgeLyon Oct 20, 2023

ahoppen Oct 20, 2023

GeorgeLyon Oct 20, 2023

ahoppen Oct 20, 2023

GeorgeLyon Oct 20, 2023

ahoppen left a comment

ahoppen Oct 20, 2023

GeorgeLyon Oct 20, 2023

ahoppen Oct 20, 2023

ahoppen Oct 20, 2023

GeorgeLyon Oct 20, 2023

ahoppen Oct 21, 2023

ahoppen Oct 20, 2023

ahoppen Oct 20, 2023

ahoppen Oct 20, 2023

bnbarham commented Oct 20, 2023

GeorgeLyon commented Oct 20, 2023

ahoppen Oct 21, 2023

ahoppen commented Oct 21, 2023

ahoppen left a comment

ahoppen left a comment

ahoppen Oct 21, 2023

GeorgeLyon commented Oct 21, 2023

ahoppen commented Oct 23, 2023

ahoppen left a comment

ahoppen commented Oct 23, 2023

ahoppen commented Oct 23, 2023

GeorgeLyon commented Oct 24, 2023

ahoppen commented Oct 24, 2023

Add support for a compile database file not at the root of a workspace #915

Add support for a compile database file not at the root of a workspace #915

Conversation

GeorgeLyon commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnbarham commented Oct 20, 2023

GeorgeLyon commented Oct 20, 2023

Choose a reason for hiding this comment

ahoppen commented Oct 21, 2023

ahoppen left a comment

Choose a reason for hiding this comment

ahoppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeLyon commented Oct 21, 2023

ahoppen commented Oct 23, 2023

ahoppen left a comment

Choose a reason for hiding this comment

ahoppen commented Oct 23, 2023

ahoppen commented Oct 23, 2023

GeorgeLyon commented Oct 24, 2023

ahoppen commented Oct 24, 2023