Skip to content
This repository was archived by the owner on Nov 18, 2022. It is now read-only.

Fix workspace path when multiProjectSetup is enabled #759

Merged
merged 12 commits into from
Apr 17, 2020
Merged

Fix workspace path when multiProjectSetup is enabled #759

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8f8aa8f
fix incorrectly stripping the root workspace folder
jannickj Apr 5, 2020
1890d87
fix completely incorrect use of parse (by me)
jannickj Apr 5, 2020
16d594b
pattern was not picked up by windows.
jannickj Apr 5, 2020
d34dff9
change the name to just be the unix format path
jannickj Apr 5, 2020
954edc6
fix bug where commands would be re-registered on restart
jannickj Apr 5, 2020
bffee19
fix grammar mistake
jannickj Apr 5, 2020
5be2fe3
fix bug where it was checking against double stripped
jannickj Apr 13, 2020
19cdaef
give the fake workspace the name of the folder it's acting as workspa…
jannickj Apr 13, 2020
4581581
set cwd to be the workspace folder, fixes cargo build command for "en…
jannickj Apr 13, 2020
12378b1
apply prettifier
jannickj Apr 13, 2020
1a761d8
undo collectionName change
jannickj Apr 16, 2020
65480c5
apply hack only to windows + add clarifying comment
jannickj Apr 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@
"rust-client.enableMultiProjectSetup": {
"type": "boolean",
"default": false,
"description": "Allow multiple projects in the same folder, along with remove the constraint that the cargo.toml must be located at the root. (Experimental: might not work for certain setups)"
"description": "Allow multiple projects in the same folder, along with removing the constraint that the cargo.toml must be located at the root. (Experimental: might not work for certain setups)"
},
"rust.sysroot": {
"type": [
Expand Down
4 changes: 3 additions & 1 deletion src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,9 @@ class ClientWorkspace {
};

const pattern = this.config.multiProjectEnabled
? `${this.folder.uri.path}/**`
? `**${this.folder.uri.path}/**`
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jannickj jannickj Apr 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :).

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

: undefined;

const collectionName = this.config.multiProjectEnabled
? `rust ${this.folder.uri.toString()}`
: 'rust';
Expand Down Expand Up @@ -309,6 +310,7 @@ class ClientWorkspace {
const ws =
multiProjectEnabled && activeWorkspace ? activeWorkspace : this;
await ws.stop();
commandsRegistered = true;
return ws.start(context);
});
this.disposables.push(restartServer);
Expand Down
6 changes: 5 additions & 1 deletion src/tasks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,11 @@ function detectCargoTasks(target: WorkspaceFolder): Task[] {
.map(({ subcommand, group }) => ({
definition: { subcommand, type: TASK_TYPE },
label: `cargo ${subcommand}`,
execution: createShellExecution({ command: 'cargo', args: [subcommand] }),
execution: createShellExecution({
command: 'cargo',
args: [subcommand],
cwd: target.uri.fsPath,
}),
group,
problemMatchers: ['$rustc'],
}))
Expand Down
12 changes: 8 additions & 4 deletions src/workspace_util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export function nearestParentWorkspace(
filePath: string,
): WorkspaceFolder {
// check that the workspace folder already contains the "Cargo.toml"
const workspaceRoot = path.parse(curWorkspace.uri.fsPath).dir;
const workspaceRoot = curWorkspace.uri.fsPath;
const rootManifest = path.join(workspaceRoot, 'Cargo.toml');
if (fs.existsSync(rootManifest)) {
return curWorkspace;
Expand All @@ -25,16 +25,20 @@ export function nearestParentWorkspace(
break;
}

// break in case the strip folder has not changed
if (workspaceRoot === path.parse(current).dir) {
// break in case the strip folder reached the workspace root
if (workspaceRoot === current) {
break;
}

// check if "Cargo.toml" is present in the parent folder
const cargoPath = path.join(current, 'Cargo.toml');
if (fs.existsSync(cargoPath)) {
// ghetto change the uri on Workspace folder to make vscode think it's located elsewhere
return { ...curWorkspace, uri: Uri.parse(current) };
return {
...curWorkspace,
name: path.basename(current),
uri: Uri.file(current),
};
}
}

Expand Down