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

feature: add multi-root projects support #5033

Merged
merged 25 commits into from
Apr 28, 2023
Merged

Conversation

kasiaMarek
Copy link
Contributor

@kasiaMarek kasiaMarek commented Mar 7, 2023

This PR adds support for multi-root project (workspace folders). It adds support for:

  1. starting server with initial workspace folders (instead of root workspace),
  2. adding/removing workspace folders using didChangeWorkspaceFolders.

Added server capabilities:

workspaceCapabilitiesOptions.setSupported(true)
workspaceCapabilitiesOptions.setChangeNotifications(true)

Metals architecutre for multi-root projects.

      |---      Single server       ---|      //we want to redirect the requests
      |                                |      //to the correct service
      |                                |      //based on the file
Service (per folder)           Service (per folder)

Tasks:

  • fake support for multiple folders
    • change of the architecture with redirecton
    • still support one root
  • change logger to support multiple folders (follow up in a different task)
  • move to multiple workspaces
    • add server capabitily
    • handle didChangeWorkspaceFolders
  • multiple folders test

Docs:

  1. Lsp workspace folders - https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_workspaceFolders
  2. Multi root VSCode - https://code.visualstudio.com/docs/editor/multi-root-workspaces
  3. Adopting multi-root - https://github.com/microsoft/vscode/wiki/Adopting-Multi-Root-Workspace-APIs

In followups we should:

  • change logging, so it logs to the current folder (currently we always log to all),
  • have a single http server and collect information from all doctors to one page,
  • add an artificial (default) folder, that handles single files,
  • possibly consider what settings we'd like to have per workspace folder.

Resolves: #5188

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most difficult issue might be the MetalsLogger, I wonder if we could inherit from scribe.file.FileWriter and use LogRecord to figure out which workspace to log to. There is filename there but it might be just the name.scala part. We might need to just log either to first workspace or to all of them.

Potentially, we could have activeWorkspace and only log to the last active one, but that might not work very well 🤔

@kasiaMarek
Copy link
Contributor Author

The most difficult issue might be the MetalsLogger, I wonder if we could inherit from scribe.file.FileWriter and use LogRecord to figure out which workspace to log to. There is filename there but it might be just the name.scala part. We might need to just log either to first workspace or to all of them.

Potentially, we could have activeWorkspace and only log to the last active one, but that might not work very well 🤔

It feels like it would be best to figure out which one to log to for current file and log to all for global things. Last active feels like it could be confusing and flaky.

@kasiaMarek kasiaMarek force-pushed the multi-root branch 4 times, most recently from fe12683 to 7abfaa9 Compare March 28, 2023 16:20
@kasiaMarek kasiaMarek force-pushed the multi-root branch 2 times, most recently from d2bbb72 to 17b5964 Compare March 29, 2023 12:19
@kasiaMarek kasiaMarek changed the title refactor: prepare arch for multi-root projects [skip ci] feature: add multi-root projects support Mar 29, 2023
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I added a bunch of comments, but most of your changes are really great and we only need to polish it.

tests/unit/src/main/scala/tests/TestingServer.scala Outdated Show resolved Hide resolved
}
}

class MetalsTreeFolderViewProvider(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you show an example of how does the tree view look like currently? With multiple folders and without?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With just metals open:
Screenshot 2023-04-04 at 14 47 57
With metals and coursier open as workspace folders:
Screenshot 2023-04-04 at 14 49 41

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could use a bit more granularity, but the only thing I would change is not to add the name if we only have a single one and maybe do it like Projects (coursier)

val libraries = new ClasspathTreeView[AbsolutePath, AbsolutePath](
definitionIndex,
TreeViewProvider.Project,
s"libraries-${folder.uri.toString()}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should have another folder level and libraries/project underneath? Possibly without that level if we only have one folder. We can change it in a later PR if this proves difficult.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the screenshots, I think it's okay as a flattened structure. But I can change it, if you think it'll be better.

@ckipp01
Copy link
Member

ckipp01 commented Apr 4, 2023

This is fantastic @kasiaMarek! I'd love to try this out with nvim-metals to also ensure that this will work with other editors, especially ones like neovim that sort of have an artificial concept of a workspace. Could you maybe outline what the intended flow of an editor is to use this? For example will this fully support changeNotifications? I'm pretty sure that's how it will need to work with nvim since a single workspaceFolder will be passed in during intialize, and then others will need to be "added" or triggered manually by the user. So having support for workspace/didChangeWorkspaceFolders will be important. So to summarize, just having a small outline of what is all supported and not supported would be super helpful for other clients to test this out without having to dig through the PR 😬 .

Looking forward to trying this out!

@kasiaMarek
Copy link
Contributor Author

@ckipp01 yes, it adds suport for workspace/didChangeWorkspaceFolders too. I wrote that explicitly in the PR description now. Do you think I should document it somewhere else too?


private def createServices(folders: List[Folder]): List[MetalsLspService] =
folders
.withFilter { case Folder(uri, _) => !BuildTools.default(uri).isEmpty }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filter out the folders that don't have a build tool defined. @tgodzik, does this seem reasonable? My intuition was that you may have also non-Scala projects. All the scala files from such folders will be handled by the default service.

@kasiaMarek kasiaMarek requested a review from tgodzik April 5, 2023 08:05
@kasiaMarek kasiaMarek marked this pull request as ready for review April 5, 2023 08:06
@ckipp01
Copy link
Member

ckipp01 commented Apr 5, 2023

@ckipp01 yes, it adds suport for workspace/didChangeWorkspaceFolders too. I wrote that explicitly in the PR description now. Do you think I should document it somewhere else too?

This might be tricky, we might want to put a note about it in here under a section explaining how we use didChangeWorkspaceFolders because we'll for sure get questions about it from other clients. Btw here is a little vid showing it working with nvim-metals!

multi-root.mp4

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! This was some great work! I took the liberty to push a couple of small changes since @kasiaMarek is out today.

@tgodzik tgodzik merged commit 6bacf42 into scalameta:main Apr 28, 2023
@ckipp01
Copy link
Member

ckipp01 commented May 1, 2023

Really really fantastic work @kasiaMarek 🚀

Thinking ahead a bit, do you think we should document this a bit somewhere? I'm mainly thinking not only for users, but also for client maintainers we're likely going to get questions like:

  • what is the expected flow look like?
  • does this spin up two Metals instances or use the same one?
  • as a user should I change my workflow now?
  • etc

It might be a good idea to answer some of these before the next release to help people out that want to try this out. Maybe a section on the website? Or even outline it on the architecture.md document?

@kubukoz
Copy link
Contributor

kubukoz commented May 4, 2023

just spotted this, amazing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multiple roots in a workspace
5 participants