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

Implement Missing Parts of Multiple Content Roots #1780

Closed
8 tasks done
radeusgd opened this issue Jun 2, 2021 · 10 comments · Fixed by #1821
Closed
8 tasks done

Implement Missing Parts of Multiple Content Roots #1780

radeusgd opened this issue Jun 2, 2021 · 10 comments · Fixed by #1821
Assignees
Labels
p-high Should be completed in the next sprint

Comments

@radeusgd
Copy link
Member

radeusgd commented Jun 2, 2021

Summary

We have a concept of multiple content roots, but it is not yet fully implemented.

This task consists of adding the missing parts so that we can use content roots for editing libraries.

Value

  • The user can easily browse and edit libraries.

Specification

  • Extend the protocol messages.
    • Include some metadata related to each content root. For now it may just be an associated human-readable name and whether it is read-only (upstream libraries will be read-only to avoid breaking them).
  • Implement the missing/extended messages.
    • file/rootAdded, file/rootRemoved
  • Provide a complete initial set of content roots at startup (including FS roots and home).
  • Modify the content root system to allow dynamically changing the set of the content roots.
  • Whenever the runtime imports a library it should notify the language server which will in turn notify the IDE that a content root has been added. It should recognize if it is read-only based on the location of the library.
    • As we are implementing this before the imports are finished we may just expose an API in the runtime for registering these new content roots.
  • Send the notification when a content root is added.
    • We need to forward the notification to the IDE.

Acceptance Criteria & Test Cases

  • The specification above has been completed.
  • Add a test where one actor subscribes for content root notifications and another one adds a library.
  • If reasonably possible, add a test for the scenario that a library import is added and then such a library is edited, to see if the update is propagated properly. (This may be added once libraries are implemented)
@iamrecursion
Copy link
Contributor

iamrecursion commented Jun 14, 2021

From the engine side we need to have the following supported:

  • Each content root sent to the IDE should contain a UUID, name, and a type (one of "project", "root", "library", "home", "custom).
  • When booting the language server, it should send roots that correspond to the current project, the user home, and teh system root(s) (drives on windows).

The complexity comes from the Windows side of things. I've already done some work to support multiple content roots in the boot response, so I can likely merge that even if I don't continue with the task.

@farmaazon
Copy link
Contributor

IDE needs a way to represent Content Root in a path passed to File.read node. In case of:

  • Project: I can use Enso_Project.root
  • Home: File.home
  • Root: I can just put / on unix, however on Windows I need to know the drive letter
  • Library : <Library name>.Enso_Project.root, but I need to know the library name of given content root
  • Custom: not used, and IDE probably can rely on listening for root/added messages.

So the root and library content roots need to have additional info, maybe a special field with necessary info.

@radeusgd
Copy link
Member Author

IDE needs a way to represent Content Root in a path passed to File.read node. In case of:

  • Project: I can use Enso_Project.root
  • Home: File.home
  • Root: I can just put / on unix, however on Windows I need to know the drive letter
  • Library : <Library name>.Enso_Project.root, but I need to know the library name of given content root
  • Custom: not used, and IDE probably can rely on listening for root/added messages.

So the root and library content roots need to have additional info, maybe a special field with necessary info.

@farmaazon What if instead all content roots would include an optional path component which would be its filesystem path? So then the root on Linux would be getting /, on Windows it would be C:\ (or other letters), we would get the Home path and libraries could also include their paths. I think it would be simpler instead of doing it defferently depending on the root type.

cc: @kustosz

@radeusgd
Copy link
Member Author

Also wanted to clarify that we can remove the file/addRoot and file/removeRoot messages from the spec, right? They were not implemented and I think the new implementation does not expect them either, so until something like this is needed, maybe it can be removed as it may need to have a different shape anyway when it is needed?

@farmaazon
Copy link
Contributor

@radeusgd

What if instead all content roots would include an optional path component which would be its filesystem path? So then the root on Linux would be getting /, on Windows it would be C:\ (or other letters), we would get the Home path and libraries could also include their paths. I think it would be simpler instead of doing it differently depending on the root type.

Well, from IDE perspective that's ok. However, the initial reasoning for content roots was "we cannot provide the full paths/tree in cloud environment". I assumed that was why the FilePath structure is based on content root, and is not just a single string.

Also wanted to clarify that we can remove the file/addRoot and file/removeRoot messages from the spec, right? They were not implemented and I think the new implementation does not expect them either, so until something like this is needed, maybe it can be removed as it may need to have a different shape anyway when it is needed?

I don't know any plans for using them (@wdanilo do you know any?), so they may be removed.

@radeusgd
Copy link
Member Author

@farmaazon

What if instead all content roots would include an optional path component which would be its filesystem path? So then the root on Linux would be getting /, on Windows it would be C:\ (or other letters), we would get the Home path and libraries could also include their paths. I think it would be simpler instead of doing it differently depending on the root type.

Well, from IDE perspective that's ok. However, the initial reasoning for content roots was "we cannot provide the full paths/tree in cloud environment". I assumed that was why the FilePath structure is based on content root, and is not just a single string.

Well, as the path is needed for the File.read node, the local fs path should also apply in the cloud environment I think, is that right @lolczak ? From what I understand the project storage is currently mounted as a regular filesystem. If that ever changes somehow, the logic for adapting File.read will need to be changed anyway, so IMO it is safe to do it this way for now, until we get reasons that disallow it.

@radeusgd
Copy link
Member Author

@farmaazon The path is being added in a separate PR (#1827).

I just wanted to confirm, @lolczak @4e6, as I understand that the user data (for now the project root, in the future we'll modify the structure so that the libraries will also sit next to it) in the cloud is mounted from some network storage, is the path for the user data stable? I.e. will it not change between whenever the project is opened? I want to make sure that we can rely on this filesystem path to be stable (so that a File.read node does not became outdated after the project is restarted) or if some other approach is necessary.

@radeusgd
Copy link
Member Author

radeusgd commented Jul 1, 2021

@farmaazon The path is being added in a separate PR (#1827).

I just wanted to confirm, @lolczak @4e6, as I understand that the user data (for now the project root, in the future we'll modify the structure so that the libraries will also sit next to it) in the cloud is mounted from some network storage, is the path for the user data stable? I.e. will it not change between whenever the project is opened? I want to make sure that we can rely on this filesystem path to be stable (so that a File.read node does not became outdated after the project is restarted) or if some other approach is necessary.

The Dockerfile sets the workspace to /volumes/workspace, so I assume the path can be considered stable.

@lolczak
Copy link
Contributor

lolczak commented Jul 1, 2021

@radeusgd So far we are not going to change the absolute path. But it can change in the future. You shouldn't rely on absolute paths.

@radeusgd
Copy link
Member Author

radeusgd commented Jul 1, 2021

@radeusgd So far we are not going to change the absolute path. But it can change in the future. You shouldn't rely on absolute paths.

@farmaazon In this case I think you will need to implement it the way you described it above.

I guess I should then change it so that the absolute path is provided only for the filesystem roots, and for libraries I will add a library name field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-high Should be completed in the next sprint
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants