neovim / nvim-lspconfig

Quickstart configs for Nvim LSP
Apache License 2.0
10.47k stars 2.06k forks source link

sourcekit-lsp in monorepos #3191

Closed dirtyhabits97 closed 4 months ago

dirtyhabits97 commented 4 months ago

Description

With the changes introduced in #3066 sourcekit-lsp fails in monorepos with multiple Package.swift files.

For example, I have the following structure:

coding_challenges/
  foo/Package.swift
  bar/Package.swift
  baz/Package.swift

When doing cd coding_challenges/foo and nvim Package.swift I'm seeing this in the lsp.log file:

[ERROR][2024-06-04 11:41:34] .../vim/lsp/rpc.lua:770    "rpc"   "/usr/bin/sourcekit-lsp"    "stderr"    "[2024-06-04 11:41:34.862] could not find manifest, or not a SwiftPM package: /Users/gonzalorehu/other_projects/coding_challenges\n"
[ERROR][2024-06-04 11:41:34] .../vim/lsp/rpc.lua:770    "rpc"   "/usr/bin/sourcekit-lsp"    "stderr"    "[2024-06-04 11:41:34.868] could not open compilation database for /Users/gonzalorehu/other_projects/coding_challenges/wc/Sources/WordCount.swift\n"

This is because the plugin now searches for Package.swift at the top of the git (mono)repo and there is none. It is not clear from #3066 why this order was modified, but reverting to the old order solves my issue:

root_dir = util.root_pattern('Package.swift', 'buildServer.json', 'compile_commands.json', '.git')

Moving or util.find_git_ancestor(filename) after Package.swift also seems to work:

    root_dir = function(filename, _)
      return util.root_pattern 'buildServer.json'(filename)
        or util.root_pattern('*.xcodeproj', '*.xcworkspace')(filename)
        -- better to keep it at the end, because some modularized apps contain multiple Package.swift files
        or util.root_pattern('compile_commands.json', 'Package.swift')(filename)
     -> or util.find_git_ancestor(filename)
    end,

In my opinion, we should maintain the original order, or at the very least push the .git search to the very end. @wojciech-kulik thoughts? I saw you're running a complex setup here, can you test if something breaks if we make this change?

wojciech-kulik commented 4 months ago

Hi,

It is not clear from https://github.com/neovim/nvim-lspconfig/pull/3066 why this order was modified

I left the comment here:

image

It's common approach that you have the root app (for example iOS app) which must have xcodeproj and then you modularize it with SPM packages, having multiple Package.swift as you mentioned.

In this case, if we look for Package.swift first, the LSP wouldn't understand the whole project, it would consider some module to be the root, which is incorrect.

In general, I'm not sure if there is any generic fix for this issue because the expected behavior depends on what you do. If you want to work on a single module you can just change cwd and the LSP will then properly detect your Package.swift.

So I think the current solution allows to work with both cases: a single SPM package and the whole project with SPM modules. The only difference is where you set the root / where you open your Neovim. At least that was the assumption.

However, as you mentioned, it seems like it searches for git root below the current working directory. Maybe my assumption about this function find_git_ancestor was incorrect. So I guess the proposed solution by you should work :).

wojciech-kulik commented 4 months ago

Oh, I've just remembered why I moved git root above Package.swift. Because sometimes the xcodeproj can be in a different node like:

- .git
- dir
-- project.xcodeproj
- some_dir
-- file.swift

In this case I assumed that if we can't find the project file, we should use the git root. However, this is an edge case.

I'm not sure if it is possible to guess what is the expected root in this case, because even if we somehow find the nested xcodeproj we can't tell whether it is the root project, or some library, or something else.

So I guess, we could assume that this is an edge case that should be handled manually by changing the config on the user's side and go with your proposed solution, assuming that if we couldn't find buildServer.json, or xcodeproj/xcworkspace then we try with Package.swift

dirtyhabits97 commented 4 months ago

It's common approach that you have the root app (for example iOS app) which must have xcodeproj and then you modularize it with SPM packages, having multiple Package.swift as you mentioned.

In this case, if we look for Package.swift first, the LSP wouldn't understand the whole project, it would consider some module to be the root, which is incorrect.

In this scenario, we could have some command-line-tool written in Swift unrelated to the iOS project:

MyProject.xcworkspace
tools/
  my-command-line-tool/
    Package.swift

In this case, the current search order would also override the cwd even if it's set to tools/my-command-line-tool no? Package.swift should be the first file we search for because it is the smaller unit. A swift package cannot depend on an xcodeproj but an xcodeproj can depend on a swift package.

I think we should respect the user's cwd. If cwd is set to a folder with Package.swift, that's what we should use for the lsp. If cwd is pointing to the root folder with an xcodeproj, then we should use the xcodeproj.

The scenario where we might not want to do this would be the following:

MyProject.xcworkspace
Package.swift
Sources/
Tests/

But I'm not sure if this is common, usually if you have a Package.swift at the top level is if the entire project is a package.

wojciech-kulik commented 4 months ago

In this case, the current search order would also override the cwd even if it's set to tools/my-command-line-tool no? Package.swift should be the first file we search for because it is the smaller unit.

The root is searched per each individual file. So you can be in a file within a module, but you are working on a whole project (cwd set to the place where xcodeproj is), so it's crucial then to detect the project, not the Package.swift, even though this is closer to the file being edited.

Regarding the last case, I think we don't have to handle it, it would be a super edge case to have Package.swift and xcodeproj at the same level.

I think we can go with the solution you suggested:

    root_dir = function(filename, _)
      return util.root_pattern 'buildServer.json'(filename)
        or util.root_pattern('*.xcodeproj', '*.xcworkspace')(filename)
        or util.root_pattern('compile_commands.json', 'Package.swift')(filename)
        or util.find_git_ancestor(filename)
    end,

It will respect the cwd. If the user is within SPM package, it won't find xcodeproj but it will find Package.swift. If the user is in the root dir with xcodeproj it will find it as expected. Git root will be used as the last resort.

At least if I understand correctly that root_pattern doesn't go below cwd.

wojciech-kulik commented 4 months ago

A swift package cannot depend on an xcodeproj but an xcodeproj can depend on a swift package.

Hmm, I know what you mean, that theoretically while editing a file from a SPM Package, the LSP doesn't have to know about the whole project. However, I think there was some problem while working on an iOS/macOS app modularized with SPM packages, but I don't remember exactly what the problem was.

Maybe something with detecting changes in other places? In general, it is a little bit complicated, because to develop iOS apps we need to use xcode-build-server, so that the LSP understands the project structure.

dirtyhabits97 commented 4 months ago

I don't mind creating the PR with my initial proposal because it solves my original issue.

But keep in mind that it will not solve this other case:

MyProject.xcworkspace
tools/
  my-command-line-tool/
    Package.swift

I just tested this with cwd pointing to the folder with Package.swift

[ERROR][2024-06-04 14:17:19] .../vim/lsp/rpc.lua:770    "rpc"   "/usr/bin/sourcekit-lsp"    "stderr"    "[2024-06-04 14:17:19.571] could not find manifest, or not a SwiftPM package: /Users/gonzalorehu/other_projects/ios/apps/ContactsApp\n"
[ERROR][2024-06-04 14:17:19] .../vim/lsp/rpc.lua:770    "rpc"   "/usr/bin/sourcekit-lsp"    "stderr"    "[2024-06-04 14:17:19.576] could not open compilation database for /Users/gonzalorehu/other_projects/ios/apps/ContactsApp/tools/HelloWorld/Sources/main.swift\n"
[ERROR][2024-06-04 14:17:24] .../vim/lsp/rpc.lua:770    "rpc"   "/usr/bin/sourcekit-lsp"    "stderr"    "[2024-06-04 14:17:24.029] could not open compilation database for /Users/gonzalorehu/other_projects/ios/apps/ContactsApp/tools/HelloWorld/Sources/Hello.swift\n"

I think this can only be solved by searching for Package.swift before the xcodeproj.

wojciech-kulik commented 4 months ago

This is weird because I checked the code and it looks like it stops searching for root_pattern once it hits cwd. But I just quickly checked the code and it is quite nested so maybe I missed something. If root_pattern doesn't go below cwd, it would cover both cases, because xcodeproj would not be found in your example.

On the other hand, maybe some people navigate to random folder and open a file there, it would break LSP then. So it looks like there is no perfect solution if the user has a freedom to set cwd.

dirtyhabits97 commented 4 months ago

From what I understand this is what is happening:

cwd = ./tools/my-command-line-tool/

search for *.xcodeproj
looking at: ./tools/my-command-line-tool
looking at: ./tools
looking at: ./

- Found *.xcodeproj at / 

It finds the .xcodeproj in a parent directory and doesn't need to search for Package.swift. What we need is a bread-first-search I believe, where we check for `.xcodeproj,*.xcworkspaceandPackage.swift` at every level.

wojciech-kulik commented 4 months ago

Yeah, probably, but it would be very slow as it happens for every file. Going straight down is much faster, because usually you will have only a few levels.

For now, I think we can go with your solution as it resolves your issue and shouldn't break anything while working with xcodeproj.