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

reintroduce multi project setup #638

Merged
merged 10 commits into from
Sep 18, 2019
Merged

reintroduce multi project setup #638

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1e2e04c
reintroduce multi project setup
jannickj Aug 7, 2019
6684a08
apply prettier
jannickj Aug 7, 2019
c802552
tell about the new setting in the warning when Cargo.toml is not found.
jannickj Aug 13, 2019
60312e3
move nearestParentWorkspace into workspace_util
jannickj Sep 10, 2019
2a5ab78
remove defensive programming (as it's already being defended against)
jannickj Sep 10, 2019
0e1114b
make intent more clear by using easier to understand names in exchang…
jannickj Sep 10, 2019
6a580eb
Remove the double negative
jannickj Sep 10, 2019
3c957c1
add todo for future work
jannickj Sep 10, 2019
da6bb20
add coments to the "Cargo.toml" search
jannickj Sep 10, 2019
47c66be
fix linting errors
jannickj Sep 10, 2019
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
5 changes: 5 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Copy link

Choose a reason for hiding this comment

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

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 🙂

Copy link
Contributor Author

@jannickj jannickj Aug 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

"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)"
},
"rust.sysroot": {
"type": [
"string",
Expand Down
7 changes: 7 additions & 0 deletions src/configuration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,13 @@ export class RLSConfiguration {
return this.configuration.get<string>('rust-client.rlsPath');
}

public get multiProjectEnabled(): boolean {
return this.configuration.get<boolean>(
'rust-client.enableMultiProjectSetup',
false,
);
}

// Added ignoreChannel for readChannel function. Otherwise we end in an infinite loop.
public rustupConfig(ignoreChannel: boolean = false): RustupConfig {
return {
Expand Down
153 changes: 100 additions & 53 deletions src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,23 +69,36 @@ function didOpenTextDocument(
if (!folder) {
return;
}

if (
workspace
.getConfiguration()
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

.get<boolean>('rust-client.enableMultiProjectSetup', false)
) {
folder = getCargoTomlWorkspace(folder, document.uri.fsPath);
} else if (
workspace
.getConfiguration()
.get<boolean>('rust-client.nestedMultiRootConfigInOutermost', true)
) {
folder = getOuterMostWorkspaceFolder(folder);
}
// folder = getCargoTomlWorkspace(folder, document.uri.fsPath);

if (!folder) {
stopSpinner(`RLS: Cargo.toml missing`);
return;
}

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

if (!workspaces.has(folderPath)) {
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const workspace = new ClientWorkspace(folder);
workspaces.set(folder.uri, workspace);
activeWorkspace = workspace;
workspaces.set(folderPath, workspace);
workspace.start(context);
} else {
const ws = workspaces.get(folderPath);
activeWorkspace = typeof ws === 'undefined' ? null : ws;
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}

Expand All @@ -110,37 +123,40 @@ function sortedWorkspaceFolders(): string[] {
return _sortedWorkspaceFolders || [];
}

// function getCargoTomlWorkspace(cur_workspace: WorkspaceFolder, file_path: string): WorkspaceFolder {
// if (!cur_workspace) {
// return cur_workspace;
// }

// const workspace_root = path.parse(cur_workspace.uri.fsPath).dir;
// const root_manifest = path.join(workspace_root, 'Cargo.toml');
// if (fs.existsSync(root_manifest)) {
// return cur_workspace;
// }

// let current = file_path;

// while (true) {
// const old = current;
// current = path.dirname(current);
// if (old == current) {
// break;
// }
// if (workspace_root == path.parse(current).dir) {
// break;
// }

// const cargo_path = path.join(current, 'Cargo.toml');
// if (fs.existsSync(cargo_path)) {
// return { ...cur_workspace, uri: Uri.parse(current) };
// }
// }

// return cur_workspace;
// }
function getCargoTomlWorkspace(
Copy link
Member

Choose a reason for hiding this comment

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

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

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'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.

curWorkspace: WorkspaceFolder,
filePath: string,
): WorkspaceFolder {
if (!curWorkspace) {
return curWorkspace;
}

const workspaceRoot = path.parse(curWorkspace.uri.fsPath).dir;
const rootManifest = path.join(workspaceRoot, 'Cargo.toml');
if (fs.existsSync(rootManifest)) {
return curWorkspace;
}

let current = filePath;

while (true) {
const old = current;
current = path.dirname(current);
if (old === current) {
break;
}
if (workspaceRoot === path.parse(current).dir) {
break;
}

const cargoPath = path.join(current, 'Cargo.toml');
if (fs.existsSync(cargoPath)) {
return { ...curWorkspace, uri: Uri.parse(current) };
}
}

return curWorkspace;
}

function getOuterMostWorkspaceFolder(folder: WorkspaceFolder): WorkspaceFolder {
const sorted = sortedWorkspaceFolders();
Expand All @@ -166,13 +182,13 @@ function didChangeWorkspaceFolders(
// if not, and it is a Rust project (i.e., has a Cargo.toml), then create a new client.
for (let folder of e.added) {
folder = getOuterMostWorkspaceFolder(folder);
if (workspaces.has(folder.uri)) {
if (workspaces.has(folder.uri.toString())) {
continue;
}
for (const f of fs.readdirSync(folder.uri.fsPath)) {
if (f === 'Cargo.toml') {
const workspace = new ClientWorkspace(folder);
workspaces.set(folder.uri, workspace);
workspaces.set(folder.uri.toString(), workspace);
workspace.start(context);
break;
}
Expand All @@ -181,15 +197,18 @@ function didChangeWorkspaceFolders(

// If a workspace is removed which is a Rust workspace, kill the client.
for (const folder of e.removed) {
const ws = workspaces.get(folder.uri);
const ws = workspaces.get(folder.uri.toString());
if (ws) {
workspaces.delete(folder.uri);
workspaces.delete(folder.uri.toString());
ws.stop();
}
}
}

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;
Copy link

Choose a reason for hiding this comment

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

Typescript has a builtin for nullable variables :

let activeWorkspace?: ClientWorkspace;

Copy link
Member

Choose a reason for hiding this comment

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

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.

let commandsUnregistered: boolean = true;

// We run one RLS and one corresponding language client per workspace folder
// (VSCode workspace, not Cargo workspace). This class contains all the per-client
Expand All @@ -209,21 +228,31 @@ class ClientWorkspace {
}

public async start(context: ExtensionContext) {
warnOnMissingCargoToml();
if (!this.config.multiProjectEnabled) {
warnOnMissingCargoToml();
}

startSpinner('RLS', 'Starting');

const serverOptions: ServerOptions = async () => {
await this.autoUpdate();
return this.makeRlsProcess();
};

const pattern = this.config.multiProjectEnabled
? `${this.folder.uri.path}/**`
: undefined;
const collectionName = this.config.multiProjectEnabled
? `rust ${this.folder.uri.toString()}`
: 'rust';
const clientOptions: LanguageClientOptions = {
// Register the server for Rust files

documentSelector: [
{ language: 'rust', scheme: 'file' },
{ language: 'rust', scheme: 'untitled' },
{ language: 'rust', scheme: 'file', pattern },
{ language: 'rust', scheme: 'untitled', pattern },
],
diagnosticCollectionName: 'rust',
diagnosticCollectionName: collectionName,
synchronize: { configurationSection: 'rust' },
// Controls when to focus the channel rather than when to reveal it in the drop-down list
revealOutputChannelOn: this.config.revealOutputChannelOn,
Expand Down Expand Up @@ -259,13 +288,17 @@ class ClientWorkspace {
clientOptions,
);

const selector = this.config.multiProjectEnabled
? { language: 'rust', scheme: 'file', pattern }
: { language: 'rust' };

this.setupProgressCounter();
this.registerCommands(context);
this.registerCommands(context, this.config.multiProjectEnabled);
this.disposables.push(activateTaskProvider(this.folder));
this.disposables.push(this.lc.start());
this.disposables.push(
languages.registerSignatureHelpProvider(
{ language: 'rust' },
selector,
new SignatureHelpProvider(this.lc),
'(',
',',
Expand All @@ -279,31 +312,45 @@ class ClientWorkspace {
}

this.disposables.forEach(d => d.dispose());
commandsUnregistered = true;
}

private registerCommands(context: ExtensionContext) {
private registerCommands(
context: ExtensionContext,
multiProjectEnabled: boolean,
) {
if (!this.lc) {
return;
}
if (multiProjectEnabled && !commandsUnregistered) {
return;
}

commandsUnregistered = false;
Copy link
Member

Choose a reason for hiding this comment

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

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

const rustupUpdateDisposable = commands.registerCommand(
'rls.update',
() => {
return rustupUpdate(this.config.rustupConfig());
const ws =
multiProjectEnabled && activeWorkspace ? activeWorkspace : this;
return rustupUpdate(ws.config.rustupConfig());
Copy link

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

Same comment about ws -> workspace here 🙂

multiProjectEnabled && activeWorkspace ? activeWorkspace : this;
await ws.stop();
return ws.start(context);
});
this.disposables.push(restartServer);

this.disposables.push(
commands.registerCommand('rls.run', (cmd: Execution) =>
runRlsCommand(this.folder, cmd),
),
commands.registerCommand('rls.run', (cmd: Execution) => {
const ws =
Copy link

Choose a reason for hiding this comment

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

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.

multiProjectEnabled && activeWorkspace ? activeWorkspace : this;
runRlsCommand(ws.folder, cmd);
}),
);
}

Expand Down Expand Up @@ -471,7 +518,7 @@ async function warnOnMissingCargoToml() {

if (files.length < 1) {
window.showWarningMessage(
'A Cargo.toml file must be at the root of the workspace in order to support all features',
'A Cargo.toml file must be at the root of the workspace in order to support all features. Alternatively set rust-client.enableMultiProjectSetup=true in settings.',
);
}
}
Expand Down