Skip to content
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.

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

Incorrect path passing for LSP server #256

Closed
to266 opened this issue Jun 14, 2021 · 15 comments
Closed

Incorrect path passing for LSP server #256

to266 opened this issue Jun 14, 2021 · 15 comments
Labels
A-language-server Area: Language server client C-bug Category: This is a bug

Comments

@to266
Copy link

to266 commented Jun 14, 2021

Reproduction steps

  • Create (in my case rust) project with the name appearing somewhere higher in the path tree. e.g. /home/to266/projects/funny/some/folder/workspace/funny. If workspace literally corresponds to rust workspace, and funny inside is a rust package withing the workspace.
  • Start hx inside funny the package
  • Observe that LSP is not initialized. The logs show
2021-06-14T09:40:14.641 helix_lsp::transport [ERROR] err <- [ERROR rust_analyzer] failed to find any projects in [AbsPathBuf("/home/to266/projects/funny")]
2021-06-14T09:40:14.642 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"window/showMessage","params":{"type":1,"message":"rust-analyzer failed to discover workspace"}}

Environment

  • Platform: Linux
  • Helix version: 0.2.0
@to266 to266 added the C-bug Category: This is a bug label Jun 14, 2021
@archseer
Copy link
Member

I think this is a rust-analyzer issue: rust-lang/rust-analyzer#1798

We try to detect a .git repository root in any of the parent directories, if that fails we fall back to passing None to the LSP and letting it figure out the correct setting.

@nrdxp
Copy link
Contributor

nrdxp commented Jun 14, 2021

It's not just rust-analyzer, as rls failed with the same issue. I have roots = ["Cargo.toml"] specified in languages.toml as well. (see my duplicate issue above for more details).

@archseer
Copy link
Member

Yeah currently we don't consume those roots yet. I think we need to change find_roots to accept a markers list, then modify this:

for ancestor in root.ancestors() {
// TODO: also use defined roots if git isn't found
if ancestor.join(".git").is_dir() {
return Some(ancestor.to_path_buf());
}

The reason I haven't done this yet is because I don't know what the behavior should be here. If we stop at the first parent dir with a Cargo.toml we might miss the fact that we're in a workspace. On the other hand, if this is a multi-language repository, searching from the root -> cwd and finding .git might not be what we want (instead we want the subfolder with Cargo.toml)

@pickfire
Copy link
Contributor

pickfire commented Jun 15, 2021

I think if we look at .git recursively we might hit $HOME as our root which may not be what the user want.

@archseer
Copy link
Member

$HOME doesn't contain .git though, it contains a .gitconfig/.gitignore. I think likely the correct way to do this is to scan for a git repo first, then scan for defined roots next.

@pickfire
Copy link
Contributor

$HOME doesn't contain .git though, it contains a .gitconfig/.gitignore. I think likely the correct way to do this is to scan for a git repo first, then scan for defined roots next.

I used to have it until it broke some applications so I use an alias home git --work-tree=/home/ivan --git-dir=/home/ivan/.home, so we may want to beware of it. Technically it is possible to have .git/ it home for say dotfiles so we may want to avoid it.

@nrdxp
Copy link
Contributor

nrdxp commented Jun 15, 2021

Perhaps, at least for rust, we could search for a directory containing both a Cargo.toml and a .git directory. Seems like a fairly clear indication of the top level rust crate.

@to266
Copy link
Author

to266 commented Jun 15, 2021

Perhaps, at least for rust, we could search for a directory containing both a Cargo.toml and a .git directory. Seems like a fairly clear indication of the top level rust crate.

Perhaps for rust-only projects. In my original example it's not the case

/home/
    to266/
        projects/
            funny/                       # git root
                some/                # just folder
                    folder/          # just folder
                        workspace/       # rust workspace
                            funny        # Crate

To me it seems the proper solution is actually reading the Cargo.toml files. If it contains the keyword [workspace], then it's a workspace, and no other (at least related) will be above that (because nested workspaces are not a thing rust-lang/cargo#5042).

if additional verification is wanted, then of course it's possible to check that the Cargo.toml in workspace contains the member crate, which is presumably where we started in the first place.

@archseer
Copy link
Member

nvim-lsp uses cargo metadata --no-deps --format-version 1 to dump data then search for workspace_root. Wish it was more straightforward.

@nrdxp
Copy link
Contributor

nrdxp commented Jun 16, 2021

Since both are rust projects, perhaps we could just steal some of the code

@archseer archseer added the A-language-server Area: Language server client label Jun 20, 2021
@kirawi
Copy link
Member

kirawi commented Oct 12, 2021

Is this still an issue since the assert seems to have been changed on master?

@matoous
Copy link
Contributor

matoous commented Dec 15, 2021

I think this is still an issue, and not just for Julia but any mono-repo. Our structure looks like this

repo/
    .git
    apps/
        ui/
            package.json
        backend/
            go.mod

gopls in the repo/apps/backend doesn't work either currently.

I think (as mentioned above) the issue is with the find_root function

pub fn find_root(root: Option<&str>) -> Option<std::path::PathBuf> {
. The question is what should be the optimal behavior:

  • still look for root where the .git is but initialize lsp in directory that matches roots.
  • look for roots or .git
  • always keep the current folder as the root and only initialize the LSP in the first parent directory that matches roots or stop at .git
  • something else?

If I understand it correctly, ideally this call to find_root https://github.com/helix-editor/helix/blob/master/helix-lsp/src/client.rs#L228 would take the roots configuration into consideration.

@archseer
Copy link
Member

I think the procedure should be:

  1. look for the highest directory with the marker file (we have these in languages l.toml, they're just not used currently)
  2. else look for a .git folder
  3. fallback to using current directory

@amousset
Copy link
Contributor

I opened #1370 with a slightly different logic (stop looking for markers when reaching git repo boundary, but not sure it makes more sense)

@archseer
Copy link
Member

archseer commented Jan 6, 2022

Resolved by #1370

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-server Area: Language server client C-bug Category: This is a bug
Projects
None yet
Development

No branches or pull requests

7 participants