Skip to content

Commit

Permalink
Auto merge of rust-lang#17275 - roife:fix-issue-17012, r=Veykril
Browse files Browse the repository at this point in the history
Fix inconsistent cwd of `run` and `debug` command in client

Fix rust-lang#17012. Also related to rust-lang#13022 and rust-lang#15993.

When the `kind` of runnable is `bin`, Cargo would use the workspace root as the cwd for the `run` command; otherwise, Cargo defaults to the package root as the cwd for `run`.

Initially, r-a assumed the workspace root as the cwd for all runnables in `debug` command, which led to issue rust-lang#13022. In this case, during unit testing, the `run` command would use the package root while `debug` would use the workspace root, causing inconsistency.

PR rust-lang#15993 addressed this problem by using the package root as the cwd for `debug` command. However, it also resulted in an inconsistency: when executing the `run` command within the main fn of a package (whose target is `bin`), Cargo would use the workspace root, whereas `debug` would use the package root, leading to issue rust-lang#17012.

The preferable approach is to determine the cwd based on the runnable's type. To resolve this, this PR introduces a new `cwd` field within `CargoRunnable`, allowing r-a to decide the appropriate cwd depending on the specific kind of the runnable.
  • Loading branch information
bors committed May 24, 2024
2 parents 6259991 + 3c7a13d commit 56d77b9
Show file tree
Hide file tree
Showing 8 changed files with 122 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -860,6 +860,11 @@ pub(crate) fn handle_runnables(
if cmd == "run" && spec.target_kind != TargetKind::Bin {
continue;
}
let cwd = if cmd != "test" || spec.target_kind == TargetKind::Bin {
spec.workspace_root.clone()
} else {
spec.cargo_toml.parent().to_path_buf()
};
let mut cargo_args =
vec![cmd.to_owned(), "--package".to_owned(), spec.package.clone()];
let all_targets = cmd != "run" && !is_crate_no_std;
Expand All @@ -876,6 +881,7 @@ pub(crate) fn handle_runnables(
kind: lsp_ext::RunnableKind::Cargo,
args: lsp_ext::CargoRunnable {
workspace_root: Some(spec.workspace_root.clone().into()),
cwd: Some(cwd.into()),
override_cargo: config.override_cargo.clone(),
cargo_args,
cargo_extra_args: config.cargo_extra_args.clone(),
Expand All @@ -893,6 +899,7 @@ pub(crate) fn handle_runnables(
kind: lsp_ext::RunnableKind::Cargo,
args: lsp_ext::CargoRunnable {
workspace_root: None,
cwd: None,
override_cargo: config.override_cargo,
cargo_args: vec!["check".to_owned(), "--workspace".to_owned()],
cargo_extra_args: config.cargo_extra_args,
Expand Down
2 changes: 2 additions & 0 deletions src/tools/rust-analyzer/crates/rust-analyzer/src/lsp/ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,8 @@ pub struct CargoRunnable {
pub override_cargo: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
pub workspace_root: Option<PathBuf>,
#[serde(skip_serializing_if = "Option::is_none")]
pub cwd: Option<PathBuf>,
// command, --package and --lib stuff
pub cargo_args: Vec<String>,
// user-specified additional cargo args, like `--release`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1360,6 +1360,10 @@ pub(crate) fn runnable(
let config = snap.config.runnables();
let spec = CargoTargetSpec::for_file(snap, runnable.nav.file_id)?;
let workspace_root = spec.as_ref().map(|it| it.workspace_root.clone());
let cwd = match runnable.kind {
ide::RunnableKind::Bin { .. } => workspace_root.clone().map(|it| it.into()),
_ => spec.as_ref().map(|it| it.cargo_toml.parent().into()),
};
let target = spec.as_ref().map(|s| s.target.clone());
let (cargo_args, executable_args) =
CargoTargetSpec::runnable_args(snap, spec, &runnable.kind, &runnable.cfg);
Expand All @@ -1372,6 +1376,7 @@ pub(crate) fn runnable(
kind: lsp_ext::RunnableKind::Cargo,
args: lsp_ext::CargoRunnable {
workspace_root: workspace_root.map(|it| it.into()),
cwd,
override_cargo: config.override_cargo,
cargo_args,
cargo_extra_args: config.cargo_extra_args,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ fn main() {}
"executableArgs": ["test_eggs", "--exact", "--show-output"],
"cargoExtraArgs": [],
"overrideCargo": null,
"cwd": server.path().join("foo"),
"workspaceRoot": server.path().join("foo")
},
"kind": "cargo",
Expand All @@ -279,6 +280,7 @@ fn main() {}
{
"args": {
"overrideCargo": null,
"cwd": server.path().join("foo"),
"workspaceRoot": server.path().join("foo"),
"cargoArgs": [
"test",
Expand Down Expand Up @@ -325,6 +327,7 @@ fn main() {}
"executableArgs": [],
"cargoExtraArgs": [],
"overrideCargo": null,
"cwd": server.path().join("foo"),
"workspaceRoot": server.path().join("foo")
},
"kind": "cargo",
Expand All @@ -336,6 +339,7 @@ fn main() {}
"executableArgs": [],
"cargoExtraArgs": [],
"overrideCargo": null,
"cwd": server.path().join("foo"),
"workspaceRoot": server.path().join("foo")
},
"kind": "cargo",
Expand Down Expand Up @@ -415,6 +419,7 @@ mod tests {
"args": {
"overrideCargo": null,
"workspaceRoot": server.path().join(runnable),
"cwd": server.path().join(runnable),
"cargoArgs": [
"test",
"--package",
Expand All @@ -432,6 +437,94 @@ mod tests {
}
}

// The main fn in packages should be run from the workspace root
#[test]
fn test_runnables_cwd() {
if skip_slow_tests() {
return;
}

let server = Project::with_fixture(
r#"
//- /foo/Cargo.toml
[workspace]
members = ["mainpkg", "otherpkg"]
//- /foo/mainpkg/Cargo.toml
[package]
name = "mainpkg"
version = "0.1.0"
//- /foo/mainpkg/src/main.rs
fn main() {}
//- /foo/otherpkg/Cargo.toml
[package]
name = "otherpkg"
version = "0.1.0"
//- /foo/otherpkg/src/lib.rs
#[test]
fn otherpkg() {}
"#,
)
.root("foo")
.server()
.wait_until_workspace_is_loaded();

server.request::<Runnables>(
RunnablesParams { text_document: server.doc_id("foo/mainpkg/src/main.rs"), position: None },
json!([
"{...}",
{
"label": "cargo test -p mainpkg --all-targets",
"kind": "cargo",
"args": {
"overrideCargo": null,
"workspaceRoot": server.path().join("foo"),
"cwd": server.path().join("foo"),
"cargoArgs": [
"test",
"--package",
"mainpkg",
"--all-targets"
],
"cargoExtraArgs": [],
"executableArgs": []
},
},
"{...}",
"{...}"
]),
);

server.request::<Runnables>(
RunnablesParams { text_document: server.doc_id("foo/otherpkg/src/lib.rs"), position: None },
json!([
"{...}",
{
"label": "cargo test -p otherpkg --all-targets",
"kind": "cargo",
"args": {
"overrideCargo": null,
"workspaceRoot": server.path().join("foo"),
"cwd": server.path().join("foo").join("otherpkg"),
"cargoArgs": [
"test",
"--package",
"otherpkg",
"--all-targets"
],
"cargoExtraArgs": [],
"executableArgs": []
},
},
"{...}",
"{...}"
]),
);
}

#[test]
fn test_format_document() {
if skip_slow_tests() {
Expand Down
3 changes: 2 additions & 1 deletion src/tools/rust-analyzer/docs/dev/lsp-extensions.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!---
lsp/ext.rs hash: 422dcc22c2e56166
lsp/ext.rs hash: 1babf76a3c2cef3b
If you need to change the above hash to make the test pass, please check if you
need to adjust this doc as well and ping this issue:
Expand Down Expand Up @@ -377,6 +377,7 @@ rust-analyzer supports only one `kind`, `"cargo"`. The `args` for `"cargo"` look
```typescript
{
workspaceRoot?: string;
cwd?: string;
cargoArgs: string[];
cargoExtraArgs: string[];
executableArgs: string[];
Expand Down
30 changes: 10 additions & 20 deletions src/tools/rust-analyzer/editors/code/src/debug.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as vscode from "vscode";
import * as path from "path";
import type * as ra from "./lsp_ext";

import { Cargo, type ExecutableInfo, getRustcId, getSysroot } from "./toolchain";
import { Cargo, getRustcId, getSysroot } from "./toolchain";
import type { Ctx } from "./ctx";
import { prepareEnv } from "./run";
import { unwrapUndefinable } from "./undefinable";
Expand All @@ -12,7 +12,6 @@ const debugOutput = vscode.window.createOutputChannel("Debug");
type DebugConfigProvider = (
config: ra.Runnable,
executable: string,
cargoWorkspace: string,
env: Record<string, string>,
sourceFileMap?: Record<string, string>,
) => vscode.DebugConfiguration;
Expand Down Expand Up @@ -134,7 +133,7 @@ async function getDebugConfiguration(
}

const env = prepareEnv(runnable, ctx.config.runnablesExtraEnv);
const { executable, workspace: cargoWorkspace } = await getDebugExecutableInfo(runnable, env);
const executable = await getDebugExecutable(runnable, env);
let sourceFileMap = debugOptions.sourceFileMap;
if (sourceFileMap === "auto") {
// let's try to use the default toolchain
Expand All @@ -148,13 +147,7 @@ async function getDebugConfiguration(
}

const provider = unwrapUndefinable(knownEngines[debugEngine.id]);
const debugConfig = provider(
runnable,
simplifyPath(executable),
cargoWorkspace,
env,
sourceFileMap,
);
const debugConfig = provider(runnable, simplifyPath(executable), env, sourceFileMap);
if (debugConfig.type in debugOptions.engineSettings) {
const settingsMap = (debugOptions.engineSettings as any)[debugConfig.type];
for (var key in settingsMap) {
Expand All @@ -176,21 +169,20 @@ async function getDebugConfiguration(
return debugConfig;
}

async function getDebugExecutableInfo(
async function getDebugExecutable(
runnable: ra.Runnable,
env: Record<string, string>,
): Promise<ExecutableInfo> {
): Promise<string> {
const cargo = new Cargo(runnable.args.workspaceRoot || ".", debugOutput, env);
const executableInfo = await cargo.executableInfoFromArgs(runnable.args.cargoArgs);
const executable = await cargo.executableFromArgs(runnable.args.cargoArgs);

// if we are here, there were no compilation errors.
return executableInfo;
return executable;
}

function getCCppDebugConfig(
runnable: ra.Runnable,
executable: string,
cargoWorkspace: string,
env: Record<string, string>,
sourceFileMap?: Record<string, string>,
): vscode.DebugConfiguration {
Expand All @@ -200,7 +192,7 @@ function getCCppDebugConfig(
name: runnable.label,
program: executable,
args: runnable.args.executableArgs,
cwd: cargoWorkspace || runnable.args.workspaceRoot,
cwd: runnable.args.cwd || runnable.args.workspaceRoot || ".",
sourceFileMap,
env,
// See https://github.com/rust-lang/rust-analyzer/issues/16901#issuecomment-2024486941
Expand All @@ -213,7 +205,6 @@ function getCCppDebugConfig(
function getCodeLldbDebugConfig(
runnable: ra.Runnable,
executable: string,
cargoWorkspace: string,
env: Record<string, string>,
sourceFileMap?: Record<string, string>,
): vscode.DebugConfiguration {
Expand All @@ -223,7 +214,7 @@ function getCodeLldbDebugConfig(
name: runnable.label,
program: executable,
args: runnable.args.executableArgs,
cwd: cargoWorkspace || runnable.args.workspaceRoot,
cwd: runnable.args.cwd || runnable.args.workspaceRoot || ".",
sourceMap: sourceFileMap,
sourceLanguages: ["rust"],
env,
Expand All @@ -233,7 +224,6 @@ function getCodeLldbDebugConfig(
function getNativeDebugConfig(
runnable: ra.Runnable,
executable: string,
cargoWorkspace: string,
env: Record<string, string>,
_sourceFileMap?: Record<string, string>,
): vscode.DebugConfiguration {
Expand All @@ -244,7 +234,7 @@ function getNativeDebugConfig(
target: executable,
// See https://github.com/WebFreak001/code-debug/issues/359
arguments: quote(runnable.args.executableArgs),
cwd: cargoWorkspace || runnable.args.workspaceRoot,
cwd: runnable.args.cwd || runnable.args.workspaceRoot || ".",
env,
valuesFormatting: "prettyPrinters",
};
Expand Down
1 change: 1 addition & 0 deletions src/tools/rust-analyzer/editors/code/src/lsp_ext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ export type Runnable = {
kind: "cargo";
args: {
workspaceRoot?: string;
cwd?: string;
cargoArgs: string[];
cargoExtraArgs: string[];
executableArgs: string[];
Expand Down
14 changes: 2 additions & 12 deletions src/tools/rust-analyzer/editors/code/src/toolchain.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,11 @@ import { unwrapUndefinable } from "./undefinable";

interface CompilationArtifact {
fileName: string;
workspace: string;
name: string;
kind: string;
isTest: boolean;
}

export interface ExecutableInfo {
executable: string;
workspace: string;
}

export interface ArtifactSpec {
cargoArgs: string[];
filter?: (artifacts: CompilationArtifact[]) => CompilationArtifact[];
Expand Down Expand Up @@ -74,7 +68,6 @@ export class Cargo {
artifacts.push({
fileName: message.executable,
name: message.target.name,
workspace: path.dirname(message.manifest_path),
kind: message.target.kind[0],
isTest: message.profile.test,
});
Expand All @@ -93,7 +86,7 @@ export class Cargo {
return spec.filter?.(artifacts) ?? artifacts;
}

async executableInfoFromArgs(args: readonly string[]): Promise<ExecutableInfo> {
async executableFromArgs(args: readonly string[]): Promise<string> {
const artifacts = await this.getArtifacts(Cargo.artifactSpec(args));

if (artifacts.length === 0) {
Expand All @@ -103,10 +96,7 @@ export class Cargo {
}

const artifact = unwrapUndefinable(artifacts[0]);
return {
executable: artifact.fileName,
workspace: artifact.workspace,
};
return artifact.fileName;
}

private async runCargo(
Expand Down

0 comments on commit 56d77b9

Please sign in to comment.