Fix workspace path when multiProjectSetup is enabled #759

jannickj · 2020-04-05T22:25:25Z

When enableMultiProjectSetup=true the following bugs will be fixed

Fix a minor bug where workspace folder was stripped from short-circuit check
Use correct pathing format (Fixes pathing for windows)
Fix document selector for windows. (windows wouldn't trigger rls hover/actions)
Fix cargo build as it would previously point to the root folder (pointed out by @jli in Handle cases when the outer workspace is not a rust project #419).

The branch has been tested on windows 10 & ubuntu 18.04

Will fix #713

jannickj · 2020-04-05T22:51:50Z

There seems to be build server problem but I don't think it's related to my branch. @nrc / @Xanewok is that an incorrect assumption?

jannickj · 2020-04-05T23:25:19Z

Applied the security fixes, just to get a building branch :), I can remove them if you'd rather do it yourself

Xanewok · 2020-04-16T18:34:55Z

Sorry for taking so long. Would you mind rebasing against master again? It'd be great to also have some kind of a regression test for this :)

Xanewok · 2020-04-16T18:52:51Z

Applied the security fixes, just to get a building branch :), I can remove them if you'd rather do it yourself

Just did that in the meantime, so it'd be good to skip the relevant commits as well.

Xanewok · 2020-04-16T18:56:53Z

src/extension.ts

@@ -90,7 +90,7 @@ function whenOpeningTextDocument(
    return;
  }

-  const folderPath = folder.uri.toString();
+  const folderPath = folder.uri.path;


Does this change give us anything? I feel like we shouldn't use unix-only path if there's not a clear reason; If we should, then we ideally need to somehow document the rationale as well

I think it was just to make hashing identical across systems. I can't remember if it was actually necessary (probably not, so I'll remove it)

parse obviously only takes URIs meaning it should be `file:///[path]` using the file command does that as well as fixing from windows format to file uri format.

Only when applying `**` at both ends does it pick it up, shouldn't really affect anyone, given it's still using the full path for matching.

…ce for

…ableMultiProjectSetup" feature.

src/extension.ts

Xanewok · 2020-04-16T19:34:45Z

src/extension.ts

@@ -208,8 +208,9 @@ class ClientWorkspace {
    };

    const pattern = this.config.multiProjectEnabled
-      ? `${this.folder.uri.path}/**`
+      ? `**${this.folder.uri.path}/**`


So I'm still wondering about this change... What exactly is the output for both .path and .fsPath and why do we need to prefix that with a wildcard?

Is the wildcard supposed to make the pattern drive-letter-agnostic, e.g. it'd match both C:\Some\Path and D:\Some\Path with a **/some/path/** pattern?

right that's a really good point.

for fsPath it's what you would expect just standard operative system paths
windows: c:\Some\Path
linux: /some/path

path is what is interesting (it's basically the path part of a FILE uri):
windows: /c:/Some/Path with the file uri: file:///c:/Some/Path
linux: /Some/Path with the file uri: file:///Some/Path

Now the pattern matcher uses file uri paths for matching, my guess is they wanted to have a OS agnostic pattern matcher.

The ** is because windows puts something else infront of the files when matching and I have no clue what however the only way to get around it was ** which will catch it.

Maybe a better solution would be to only apply this for windows

Thanks for clarifying! What do you mean by something else? Do you mean like c%3A (which is percent-encoded c:)?

No, I meant they put something [something]/c:/... I don't know what that something is and I couldn't figure it out, I thought it was fine however but I understand wanting to fix it the correct way :).

So I did some digging around vscode and vscode-languageserver-node and it seems this should also accept RelativePattern, which encapsulates better what we want to achieve. The typings, however, only accept string, so I'd be fine landing this as-is :)

Xanewok · 2020-04-17T11:29:36Z

Thank you! I don't want to block this PR further, really appreciate your time and effort to land these fixes 🙇

jannickj force-pushed the fix-pathing-for-windows branch from c9edfdf to 7445b96 Compare April 5, 2020 22:55

jannickj changed the title ~~Fix pathing for windows when multiProjectSetup is enabled~~ Fix workspace path when multiProjectSetup is enabled Apr 13, 2020

This was referenced Apr 13, 2020

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

Open

Request to support run from this file function #745

Open

Xanewok reviewed Apr 16, 2020

View reviewed changes

jannickj added 10 commits April 16, 2020 21:07

fix incorrectly stripping the root workspace folder

8f8aa8f

fix completely incorrect use of parse (by me)

1890d87

parse obviously only takes URIs meaning it should be `file:///[path]` using the file command does that as well as fixing from windows format to file uri format.

pattern was not picked up by windows.

16d594b

Only when applying `**` at both ends does it pick it up, shouldn't really affect anyone, given it's still using the full path for matching.

change the name to just be the unix format path

d34dff9

fix bug where commands would be re-registered on restart

954edc6

fix grammar mistake

bffee19

fix bug where it was checking against double stripped

5be2fe3

give the fake workspace the name of the folder it's acting as workspa…

19cdaef

…ce for

set cwd to be the workspace folder, fixes cargo build command for "en…

4581581

…ableMultiProjectSetup" feature.

apply prettifier

12378b1

jannickj force-pushed the fix-pathing-for-windows branch from 2cb4974 to 12378b1 Compare April 16, 2020 19:08

Xanewok reviewed Apr 16, 2020

View reviewed changes

src/extension.ts Outdated Show resolved Hide resolved

undo collectionName change

1a761d8

Xanewok reviewed Apr 16, 2020

View reviewed changes

apply hack only to windows + add clarifying comment

65480c5

jannickj force-pushed the fix-pathing-for-windows branch from 3a6466d to 65480c5 Compare April 16, 2020 21:35

Xanewok merged commit c3d5421 into rust-lang:master Apr 17, 2020

Xanewok mentioned this pull request Apr 17, 2020

Change Uri.parse to Uri.file #469

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix workspace path when multiProjectSetup is enabled #759

Fix workspace path when multiProjectSetup is enabled #759

jannickj commented Apr 5, 2020 •

edited

Loading

jannickj commented Apr 5, 2020

jannickj commented Apr 5, 2020

Xanewok commented Apr 16, 2020

Xanewok commented Apr 16, 2020

Xanewok Apr 16, 2020

jannickj Apr 16, 2020

Xanewok Apr 16, 2020

jannickj Apr 16, 2020

jannickj Apr 16, 2020 •

edited

Loading

Xanewok Apr 16, 2020

jannickj Apr 16, 2020

Xanewok Apr 17, 2020

Xanewok commented Apr 17, 2020

Fix workspace path when multiProjectSetup is enabled #759

Fix workspace path when multiProjectSetup is enabled #759

Conversation

jannickj commented Apr 5, 2020 • edited Loading

jannickj commented Apr 5, 2020

jannickj commented Apr 5, 2020

Xanewok commented Apr 16, 2020

Xanewok commented Apr 16, 2020

Xanewok Apr 16, 2020

Choose a reason for hiding this comment

jannickj Apr 16, 2020

Choose a reason for hiding this comment

Xanewok Apr 16, 2020

Choose a reason for hiding this comment

jannickj Apr 16, 2020

Choose a reason for hiding this comment

jannickj Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Xanewok Apr 16, 2020

Choose a reason for hiding this comment

jannickj Apr 16, 2020

Choose a reason for hiding this comment

Xanewok Apr 17, 2020

Choose a reason for hiding this comment

Xanewok commented Apr 17, 2020

jannickj commented Apr 5, 2020 •

edited

Loading

jannickj Apr 16, 2020 •

edited

Loading