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.

Sign up for GitHub

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

Refactor extension to support arbitrary shell command runnables #16839

Merged
merged 3 commits into from
Mar 18, 2024
Merged

Refactor extension to support arbitrary shell command runnables #16839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d472fd9
refactor: Rename CargoTask to RustTask in extension
Wilfred Mar 13, 2024
2e109c7
refactor: Use a single CLI args array rather than a separate subcomma…
Wilfred Mar 13, 2024
4422a90
refactor: Store the CLI command directly in RustTargetDefinition
Wilfred Mar 14, 2024
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
28 changes: 19 additions & 9 deletions editors/code/src/run.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as vscode from "vscode";
import type * as lc from "vscode-languageclient";
import * as ra from "./lsp_ext";
import * as tasks from "./tasks";
import * as toolchain from "./toolchain";

import type { CtxInit } from "./ctx";
import { makeDebugConfig } from "./debug";
Expand Down Expand Up @@ -111,35 +112,44 @@ export async function createTask(runnable: ra.Runnable, config: Config): Promise
throw `Unexpected runnable kind: ${runnable.kind}`;
}

const args = createArgs(runnable);
let program: string;
let args = createArgs(runnable);
if (runnable.args.overrideCargo) {
// Split on spaces to allow overrides like "wrapper cargo".
const cargoParts = runnable.args.overrideCargo.split(" ");

const definition: tasks.CargoTaskDefinition = {
program = unwrapUndefinable(cargoParts[0]);
args = [...cargoParts.slice(1), ...args];
} else {
program = await toolchain.cargoPath();
}

const definition: tasks.RustTargetDefinition = {
type: tasks.TASK_TYPE,
command: args[0], // run, test, etc...
args: args.slice(1),
program,
args,
cwd: runnable.args.workspaceRoot || ".",
env: prepareEnv(runnable, config.runnablesExtraEnv),
overrideCargo: runnable.args.overrideCargo,
};

// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
const target = vscode.workspace.workspaceFolders![0]; // safe, see main activate()
const cargoTask = await tasks.buildCargoTask(
const task = await tasks.buildRustTask(
target,
definition,
runnable.label,
args,
config.problemMatcher,
config.cargoRunner,
true,
);

cargoTask.presentationOptions.clear = true;
task.presentationOptions.clear = true;
// Sadly, this doesn't prevent focus stealing if the terminal is currently
// hidden, and will become revealed due to task execution.
cargoTask.presentationOptions.focus = false;
task.presentationOptions.focus = false;

return cargoTask;
return task;
}

export function createArgs(runnable: ra.Runnable): string[] {
Expand Down
50 changes: 21 additions & 29 deletions editors/code/src/tasks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,20 @@ import * as vscode from "vscode";
import * as toolchain from "./toolchain";
import type { Config } from "./config";
import { log } from "./util";
import { unwrapUndefinable } from "./undefinable";

// This ends up as the `type` key in tasks.json. RLS also uses `cargo` and
// our configuration should be compatible with it so use the same key.
export const TASK_TYPE = "cargo";
export const TASK_SOURCE = "rust";

export interface CargoTaskDefinition extends vscode.TaskDefinition {
command?: string;
args?: string[];
export interface RustTargetDefinition extends vscode.TaskDefinition {
program: string;
args: string[];
cwd?: string;
env?: { [key: string]: string };
overrideCargo?: string;
}

class CargoTaskProvider implements vscode.TaskProvider {
class RustTaskProvider implements vscode.TaskProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'm not sure there will be any other kind of Task run by rust-analyzer, so I wouldn't qualify with Rust, but I also don't feel too strongly.

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 think it needs some name, class TaskProvider implements vscode.TaskProvider would be confusing. I don't feel strongly about the name though. Do you have any suggestions? RustAnalyzerTaskProvider is maybe a little long.

private readonly config: Config;

constructor(config: Config) {
Expand All @@ -39,14 +37,15 @@ class CargoTaskProvider implements vscode.TaskProvider {
{ command: "run", group: undefined },
];

const cargoPath = await toolchain.cargoPath();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this should happen in this PR, but I wonder whether toolchain.ts should even exist or if the toolchain contents shouldn't be synced over from rust-analyzer itself over a request, given the work that Lukas did to make toolchain discovery more reliable. there's two separate implementations of this discovery mechanism, and the one in the extension is substantially less robust.

if we're to switch over, i think it's worth considering keeping the old and new versions of runnables around before removing the older version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR (especially with #16840 being a larger change) is probably better as a small focused PR, but I agree it would be nice to follow up on how toolchains are handled :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should ideally replicate the toolchain discovery here / allow clients to query the server for the correct toolchain stuff. Not doing that can cause recompilations and random failures. (Agree that that can go into a separate PR though)


const tasks: vscode.Task[] = [];
for (const workspaceTarget of vscode.workspace.workspaceFolders || []) {
for (const def of defs) {
const vscodeTask = await buildCargoTask(
const vscodeTask = await buildRustTask(
workspaceTarget,
{ type: TASK_TYPE, command: def.command },
{ type: TASK_TYPE, program: cargoPath, args: [def.command] },
`cargo ${def.command}`,
[def.command],
this.config.problemMatcher,
this.config.cargoRunner,
);
Expand All @@ -63,15 +62,13 @@ class CargoTaskProvider implements vscode.TaskProvider {
// we need to inform VSCode how to execute that command by creating
// a ShellExecution for it.

const definition = task.definition as CargoTaskDefinition;
const definition = task.definition as RustTargetDefinition;

if (definition.type === TASK_TYPE && definition.command) {
const args = [definition.command].concat(definition.args ?? []);
return await buildCargoTask(
if (definition.type === TASK_TYPE) {
return await buildRustTask(
task.scope,
definition,
task.name,
args,
this.config.problemMatcher,
this.config.cargoRunner,
);
Expand All @@ -81,11 +78,10 @@ class CargoTaskProvider implements vscode.TaskProvider {
}
}

export async function buildCargoTask(
export async function buildRustTask(
scope: vscode.WorkspaceFolder | vscode.TaskScope | undefined,
definition: CargoTaskDefinition,
definition: RustTargetDefinition,
name: string,
args: string[],
problemMatcher: string[],
customRunner?: string,
throwOnError: boolean = false,
Expand All @@ -95,7 +91,12 @@ export async function buildCargoTask(
if (customRunner) {
const runnerCommand = `${customRunner}.buildShellExecution`;
try {
const runnerArgs = { kind: TASK_TYPE, args, cwd: definition.cwd, env: definition.env };
const runnerArgs = {
kind: TASK_TYPE,
args: definition.args,
cwd: definition.cwd,
env: definition.env,
};
const customExec = await vscode.commands.executeCommand(runnerCommand, runnerArgs);
if (customExec) {
if (customExec instanceof vscode.ShellExecution) {
Expand All @@ -113,16 +114,7 @@ export async function buildCargoTask(
}

if (!exec) {
// Check whether we must use a user-defined substitute for cargo.
// Split on spaces to allow overrides like "wrapper cargo".
const overrideCargo = definition.overrideCargo ?? definition.overrideCargo;
const cargoPath = await toolchain.cargoPath();
const cargoCommand = overrideCargo?.split(" ") ?? [cargoPath];

const fullCommand = [...cargoCommand, ...args];

const processName = unwrapUndefinable(fullCommand[0]);
exec = new vscode.ProcessExecution(processName, fullCommand.slice(1), definition);
exec = new vscode.ProcessExecution(definition.program, definition.args, definition);
}

return new vscode.Task(
Expand All @@ -138,6 +130,6 @@ export async function buildCargoTask(
}

export function activateTaskProvider(config: Config): vscode.Disposable {
const provider = new CargoTaskProvider(config);
const provider = new RustTaskProvider(config);
return vscode.tasks.registerTaskProvider(TASK_TYPE, provider);
}
Loading