reintroduce multi project setup #638

jannickj · 2019-08-07T02:44:21Z

This will allow you to have project structures like this:

projectA/Cargo.toml
projectA/src
projectB/Cargo.toml
projectB/src
subProjects/projectD/Cargo.toml
subProjects/projectD/src
subProjects/projectE/Cargo.toml
subProjects/projectE/src

when you open a file it will search up parent folders until it finds a Cargo.toml, and either reuse the RLS instance for that one or create one if it doesn't exists.

Changes

enable multiple projects with multiple rls running
change the URI key back to string for workspaces (as it's unreliable)
add selectors such that only the correct rls is used for a given project
take great care to ensure the old setup works as normal
add a configuration and turn it off by default.

Second attempt at: #457

* enable multiple projects with multiple rls running * change the URI key back to string for workspaces (as it's unreliable) * add selectors such that only the correct rls is used for a given project * take great care to ensure the old setup works normally * add a configuration and turn it off by default.

jannickj · 2019-08-07T07:43:16Z

forced pushed to make it more clear what I am actually changing :)

MonsieurMan

I suggested some really small changes, I don't know if they make sense regarding to tslint and variable scope as I did not have time to clone the repo.

I also suggested another workflow I'd like to discuss if possible.

MonsieurMan · 2019-08-07T19:07:38Z

src/extension.ts

-        runRlsCommand(this.folder, cmd),
-      ),
+      commands.registerCommand('rls.run', (cmd: Execution) => {
+        const ws =


Reading this took me some second to understand that ws was workspace, also personal preference but I find ternary easier to read like so :

const workspace = (multiProjectEnabled && activeWorkspace) ? activeWorkspace : this; runRlsCommand(workspace.folder, cmd);

I don't know if it's ok with the tslint configs.

MonsieurMan · 2019-08-07T19:08:15Z

src/extension.ts

      },
    );
    this.disposables.push(rustupUpdateDisposable);

    const restartServer = commands.registerCommand('rls.restart', async () => {
-      await this.stop();
-      return this.start(context);
+      const ws =


Same comment about ws -> workspace here 🙂

MonsieurMan · 2019-08-07T19:12:26Z

package.json

@@ -219,6 +219,11 @@
                    "default": true,
                    "description": "If one root workspace folder is nested in another root folder, look for the Rust config in the outermost root."
                },
+                "rust-client.enableMultiProjectSetup": {


I don't know much about the extension, didn't have the time to clone it yet, but wouldn't it be possible to make the extension work like :

Normal cargo.toml at root of the project is tried

If it fails the extension tries to determine if it's not a monorepo

I'd like your opinion on this, if it is quite some work I'd happily lend you a hand 🙂

Normal cargo.toml at root of the project is tried

With this change the cargo.toml at root should also just work.

If it fails the extension tries to determine if it's not a monorepo

How would you detect that? I could imagine some wtf moments where you add a folder and all of the sudden the tooling stops working. I would rather make configuration for stuff like that.

I'd like your opinion on this, if it is quite some work I'd happily lend you a hand

In general I would estimate that 99% will just always have it turned on. So mostly this is about not breaking it for the 1%.

and thx for the offer :)

MonsieurMan · 2019-08-07T19:13:54Z

src/extension.ts

  if (
+    workspace
+      .getConfiguration()


workspace.getConfiguration is also called later on, maybe consider storing it into a const configuration and reusing the same variable ?

ye, the problem is the function was clearly never meant to take configuration arguments. However it would take some re-architecture to make it nice, and I didn't want to push for too many changes.

MonsieurMan · 2019-08-07T19:20:29Z

src/extension.ts

    workspace.start(context);
+  } else {
+    const ws = workspaces.get(folderPath);
+    activeWorkspace = typeof ws === 'undefined' ? null : ws;


const workspace = workspaces.get(folderPath); activeWorkspace = (typeof workspace === 'undefined') ? null : workspace;

the formatter, which is build server checked, decides stuff like that.

MonsieurMan · 2019-08-07T19:22:50Z

src/extension.ts

-const workspaces: Map<Uri, ClientWorkspace> = new Map();
+// Don't use URI as it's unreliable the same path might not become the same URI.
+const workspaces: Map<string, ClientWorkspace> = new Map();
+let activeWorkspace: ClientWorkspace | null;


Typescript has a builtin for nullable variables :

let activeWorkspace?: ClientWorkspace;

As per above, it'd be great if we could extract this 'global' state somehow to a dedicated module, similar to any other code that manages/fetches appropriate workspace servers.

MonsieurMan · 2019-08-07T19:27:53Z

src/extension.ts

-        return rustupUpdate(this.config.rustupConfig());
+        const ws =
+          multiProjectEnabled && activeWorkspace ? activeWorkspace : this;
+        return rustupUpdate(ws.config.rustupConfig());


const workspace = (multiProjectEnabled && activeWorkspace) ? activeWorkspace : this; return rustupUpdate(workspace.config.rustupConfig());

jannickj · 2019-08-12T22:35:48Z

@nrc / @Xanewok do you have time to look it over?

Xanewok · 2019-08-12T22:38:58Z

Yep! Before merging, I'd like to release a 0.6.2 first with a fix for #639

jannickj · 2019-08-19T23:14:16Z

@Xanewok Are you ready to review now?

wyze · 2019-08-20T00:44:52Z

I've built your branch from source and have been using it for over a week without any issues, in both a multi-project setup and single project.

jannickj · 2019-09-03T13:12:19Z

👋

Xanewok

Thank you for working on it, it's a highly requested feature as you can tell!

I left some comments below - it'd be great to extract the workspace managing bits outside before we delve into details here since it's getting a bit conflated and harder to see which path we should take with the details of the implementation.

Are URIs unreliable for workspaces because they can be not normalized/canonicalized?

Xanewok · 2019-09-07T18:52:34Z

src/extension.ts


+    commandsUnregistered = false;


nit: I think it'd be more clear to have commandsRegistered instead if we're tracking state like that

Xanewok · 2019-09-07T18:54:52Z

src/extension.ts

-
-//     return cur_workspace;
-// }
+function getCargoTomlWorkspace(


It'd be good to document this briefly since it's not immediately obvious (at least for me) what is this doing without analyzing the body thoroughly

I'll add some comments.
However even though the body might look a bit big, I actually kept it like that to make it simplistic, mostly because it's a function that will run fairly often and I didn't want to risk busy loops.

Xanewok · 2019-09-07T18:56:17Z

src/extension.ts

-  if (!workspaces.has(folder.uri)) {
+  const folderPath = folder.uri.toString();
+
+  if (!workspaces.has(folderPath)) {


We should definitely move this logic out of didOpenTextDocument. Anything related to organizing and identifying instances across different folders/workspaces should be under src/workspace.ts perhaps? (name open for bikeshedding)

While I'm not against refactoring some of the code out, I'm afraid that this will delay this PR even more, as it's now doing two things at once.

Xanewok · 2019-09-07T18:58:55Z

src/extension.ts

-const workspaces: Map<Uri, ClientWorkspace> = new Map();
+// Don't use URI as it's unreliable the same path might not become the same URI.
+const workspaces: Map<string, ClientWorkspace> = new Map();
+let activeWorkspace: ClientWorkspace | null;


As per above, it'd be great if we could extract this 'global' state somehow to a dedicated module, similar to any other code that manages/fetches appropriate workspace servers.

jannickj · 2019-09-09T22:20:28Z

Are URIs unreliable for workspaces because they can be not normalized/canonicalized?

No, I tried with identical URI's but I noticed that there was some caching going on because when I ran the code in debug I could see that the some private field were being set the second time I used a URI for the same string. Which I concluded was why they hash'ed differently.

I couldn't get it to work on my machine but maybe I was using it wrong, so if someone knows the answer I'm happy to hear it.

jannickj · 2019-09-09T22:37:44Z

I left some comments below - it'd be great to extract the workspace managing bits outside before we delve into details here since it's getting a bit conflated and harder to see which path we should take with the details of the implementation.

It's good comments and I don't disagree, however I feel a requirement of refactoring some of the designs decisions already made in the code base is a bit much. I'm not against refactoring some of the code however I am very afraid I'll spent a good few hours doing that only to have my PR linger for the next 6 months.

I'll do it but I'll need your word on it not happening.

…e of cargo cult

jannickj · 2019-09-10T21:18:09Z

I looked into it: I started the work on moving to a "workspace" module the problem is that almost 80% of extensions is about the workspace, more than that the code is littered with global state. Now If one must have global state at least keep it contained to one file. As such I ended up just moving my own function, along with writing some TODOs.

With that said I do think it's a good idea to look at this code and do some refactoring however I don't think this is the PR for it.

jannickj · 2019-09-18T14:11:34Z

👋

Xanewok · 2019-09-18T14:31:36Z

Changes look good and I won't block this any longer - sorry for the delay!
And many thanks for taking a look at this, I think a lot of people will be happy =)

jli · 2020-04-12T20:21:28Z

@jannickj thanks so much for the contribution!

Everything appears to be working with my nested subdirectory. However, running the "cargo build" task doesn't seem to work. Do you know if this is meant to be supported?

(more details here: #419 (comment) )

jannickj force-pushed the allow-multiple-rls-projects branch from 3f814f0 to 3eb24b2 Compare August 7, 2019 07:26

jannickj force-pushed the allow-multiple-rls-projects branch from 3eb24b2 to a8b652a Compare August 7, 2019 07:35

apply prettier

6684a08

jannickj force-pushed the allow-multiple-rls-projects branch from a8b652a to 6684a08 Compare August 7, 2019 07:44

MonsieurMan suggested changes Aug 7, 2019

View reviewed changes

MonsieurMan mentioned this pull request Aug 7, 2019

support for opening a parent directory of the rust project #507

Open

tell about the new setting in the warning when Cargo.toml is not found.

c802552

Xanewok suggested changes Sep 7, 2019

View reviewed changes

Xanewok added this to the 0.7 milestone Sep 7, 2019

jannickj added 7 commits September 10, 2019 23:09

move nearestParentWorkspace into workspace_util

60312e3

remove defensive programming (as it's already being defended against)

2a5ab78

make intent more clear by using easier to understand names in exchang…

0e1114b

…e of cargo cult

Remove the double negative

6a580eb

add todo for future work

3c957c1

add coments to the "Cargo.toml" search

da6bb20

fix linting errors

47c66be

jannickj force-pushed the allow-multiple-rls-projects branch from 4c2c97a to 47c66be Compare September 10, 2019 21:13

Xanewok merged commit ffe36f3 into rust-lang:master Sep 18, 2019

jli mentioned this pull request Apr 12, 2020

Handle cases when the outer workspace is not a rust project #419

Open

jannickj mentioned this pull request Apr 14, 2020

Add multi project support vscode rust-lang/rust-analyzer#3972

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reintroduce multi project setup #638

reintroduce multi project setup #638

jannickj commented Aug 7, 2019 •

edited

Loading

jannickj commented Aug 7, 2019

MonsieurMan left a comment

MonsieurMan Aug 7, 2019

MonsieurMan Aug 7, 2019

MonsieurMan Aug 7, 2019

jannickj Aug 8, 2019 •

edited

Loading

MonsieurMan Aug 7, 2019

jannickj Aug 8, 2019

MonsieurMan Aug 7, 2019

jannickj Aug 8, 2019

MonsieurMan Aug 7, 2019

Xanewok Sep 7, 2019

MonsieurMan Aug 7, 2019

jannickj commented Aug 12, 2019

Xanewok commented Aug 12, 2019

jannickj commented Aug 19, 2019

wyze commented Aug 20, 2019

jannickj commented Sep 3, 2019

Xanewok left a comment

Xanewok Sep 7, 2019

Xanewok Sep 7, 2019

jannickj Sep 9, 2019

Xanewok Sep 7, 2019

jannickj Sep 9, 2019

Xanewok Sep 7, 2019

jannickj commented Sep 9, 2019

jannickj commented Sep 9, 2019

jannickj commented Sep 10, 2019

jannickj commented Sep 18, 2019

Xanewok commented Sep 18, 2019

jli commented Apr 12, 2020

reintroduce multi project setup #638

reintroduce multi project setup #638

Conversation

jannickj commented Aug 7, 2019 • edited Loading

jannickj commented Aug 7, 2019

MonsieurMan 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

jannickj Aug 8, 2019 • edited Loading

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

jannickj commented Aug 12, 2019

Xanewok commented Aug 12, 2019

jannickj commented Aug 19, 2019

wyze commented Aug 20, 2019

jannickj commented Sep 3, 2019

Xanewok 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

jannickj commented Sep 9, 2019

jannickj commented Sep 9, 2019

jannickj commented Sep 10, 2019

jannickj commented Sep 18, 2019

Xanewok commented Sep 18, 2019

jli commented Apr 12, 2020

jannickj commented Aug 7, 2019 •

edited

Loading

jannickj Aug 8, 2019 •

edited

Loading