Skip to content

internal: remove invocationLocation in favor of invocationStrategy #17888

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.

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

Merged
merged 1 commit into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 4 additions & 15 deletions crates/project-model/src/build_dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ use serde::Deserialize;
use toolchain::Tool;

use crate::{
utf8_stdout, CargoConfig, CargoFeatures, CargoWorkspace, InvocationLocation,
InvocationStrategy, ManifestPath, Package, Sysroot, TargetKind,
utf8_stdout, CargoConfig, CargoFeatures, CargoWorkspace, InvocationStrategy, ManifestPath,
Package, Sysroot, TargetKind,
};

/// Output of the build script and proc-macro building steps for a workspace.
Expand Down Expand Up @@ -63,10 +63,7 @@ impl WorkspaceBuildScripts {
progress: &dyn Fn(String),
sysroot: &Sysroot,
) -> io::Result<WorkspaceBuildScripts> {
let current_dir = match &config.invocation_location {
InvocationLocation::Root(root) if config.run_build_script_command.is_some() => root,
_ => workspace.workspace_root(),
};
let current_dir = workspace.workspace_root();

let allowed_features = workspace.workspace_features();
let cmd = Self::build_command(
Expand All @@ -89,15 +86,7 @@ impl WorkspaceBuildScripts {
) -> io::Result<Vec<WorkspaceBuildScripts>> {
assert_eq!(config.invocation_strategy, InvocationStrategy::Once);

let current_dir = match &config.invocation_location {
InvocationLocation::Root(root) => root,
InvocationLocation::Workspace => {
return Err(io::Error::new(
io::ErrorKind::Other,
"Cannot run build scripts from workspace with invocation strategy `once`",
))
}
};
let current_dir = workspace_root;
Copy link
Member

Choose a reason for hiding this comment

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

I am surprised by this; once should not be per-workspace... or does this refer to the vscode concept of a workspace rather than the cargo concent?

The argument list for this function is quite confusing since there are multiple "workspaces" but then just a single "workspace_root"...

Copy link
Member

Choose a reason for hiding this comment

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

The variable name here is a bit misleading (should be renamed into current_dir/working_dir, the sole caller of this passes the actual root here.

let cmd = Self::build_command(
config,
&Default::default(),
Expand Down
3 changes: 1 addition & 2 deletions crates/project-model/src/cargo_workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use serde_json::from_value;
use span::Edition;
use toolchain::Tool;

use crate::{utf8_stdout, InvocationLocation, ManifestPath, Sysroot};
use crate::{utf8_stdout, ManifestPath, Sysroot};
use crate::{CfgOverrides, InvocationStrategy};

/// [`CargoWorkspace`] represents the logical structure of, well, a Cargo
Expand Down Expand Up @@ -100,7 +100,6 @@ pub struct CargoConfig {
/// Extra env vars to set when invoking the cargo command
pub extra_env: FxHashMap<String, String>,
pub invocation_strategy: InvocationStrategy,
pub invocation_location: InvocationLocation,
/// Optional path to use instead of `target` when building
pub target_dir: Option<Utf8PathBuf>,
}
Expand Down
9 changes: 1 addition & 8 deletions crates/project-model/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,20 +186,13 @@ fn utf8_stdout(mut cmd: Command) -> anyhow::Result<String> {
Ok(stdout.trim().to_owned())
}

#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
#[derive(Clone, Debug, Default, PartialEq, Eq)]
pub enum InvocationStrategy {
Once,
#[default]
PerWorkspace,
}

#[derive(Clone, Debug, Default, PartialEq, Eq)]
pub enum InvocationLocation {
Root(AbsPathBuf),
#[default]
Workspace,
}

/// A set of cfg-overrides per crate.
#[derive(Default, Debug, Clone, Eq, PartialEq)]
pub struct CfgOverrides {
Expand Down
50 changes: 2 additions & 48 deletions crates/rust-analyzer/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,6 @@ config_data! {
pub(crate) cargo_autoreload: bool = true,
/// Run build scripts (`build.rs`) for more precise code analysis.
cargo_buildScripts_enable: bool = true,
/// Specifies the working directory for running build scripts.
/// - "workspace": run build scripts for a workspace in the workspace's root directory.
/// This is incompatible with `#rust-analyzer.cargo.buildScripts.invocationStrategy#` set to `once`.
/// - "root": run build scripts in the project's root directory.
/// This config only has an effect when `#rust-analyzer.cargo.buildScripts.overrideCommand#`
/// is set.
cargo_buildScripts_invocationLocation: InvocationLocation = InvocationLocation::Workspace,
/// Specifies the invocation strategy to use when running the build scripts command.
/// If `per_workspace` is set, the command will be executed for each workspace.
/// If `once` is set, the command will be executed once.
Copy link
Member

@RalfJung RalfJung Aug 19, 2024

Choose a reason for hiding this comment

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

This should document the working directory that the command will be executed in, for both cases.

Expand All @@ -101,8 +94,7 @@ config_data! {
/// If there are multiple linked projects/workspaces, this command is invoked for
/// each of them, with the working directory being the workspace root
/// (i.e., the folder containing the `Cargo.toml`). This can be overwritten
/// by changing `#rust-analyzer.cargo.buildScripts.invocationStrategy#` and
/// `#rust-analyzer.cargo.buildScripts.invocationLocation#`.
/// by changing `#rust-analyzer.cargo.buildScripts.invocationStrategy#`.
///
/// By default, a cargo invocation will be constructed for the configured
/// targets and features, with the following base command line:
Expand Down Expand Up @@ -182,14 +174,6 @@ config_data! {
///
/// For example for `cargo check`: `dead_code`, `unused_imports`, `unused_variables`,...
check_ignore: FxHashSet<String> = FxHashSet::default(),
/// Specifies the working directory for running checks.
/// - "workspace": run checks for workspaces in the corresponding workspaces' root directories.
// FIXME: Ideally we would support this in some way
/// This falls back to "root" if `#rust-analyzer.check.invocationStrategy#` is set to `once`.
/// - "root": run checks in the project's root directory.
/// This config only has an effect when `#rust-analyzer.check.overrideCommand#`
/// is set.
check_invocationLocation | checkOnSave_invocationLocation: InvocationLocation = InvocationLocation::Workspace,
/// Specifies the invocation strategy to use when running the check command.
/// If `per_workspace` is set, the command will be executed for each workspace.
/// If `once` is set, the command will be executed once.
Expand All @@ -212,8 +196,7 @@ config_data! {
/// If there are multiple linked projects/workspaces, this command is invoked for
/// each of them, with the working directory being the workspace root
/// (i.e., the folder containing the `Cargo.toml`). This can be overwritten
/// by changing `#rust-analyzer.check.invocationStrategy#` and
/// `#rust-analyzer.check.invocationLocation#`.
/// by changing `#rust-analyzer.check.invocationStrategy#`.
///
/// If `$saved_file` is part of the command, rust-analyzer will pass
/// the absolute path of the saved file to the provided command. This is
Expand Down Expand Up @@ -1868,12 +1851,6 @@ impl Config {
InvocationStrategy::Once => project_model::InvocationStrategy::Once,
InvocationStrategy::PerWorkspace => project_model::InvocationStrategy::PerWorkspace,
},
invocation_location: match self.cargo_buildScripts_invocationLocation(None) {
InvocationLocation::Root => {
project_model::InvocationLocation::Root(self.root_path.clone())
}
InvocationLocation::Workspace => project_model::InvocationLocation::Workspace,
},
run_build_script_command: self.cargo_buildScripts_overrideCommand(None).clone(),
extra_args: self.cargo_extraArgs(None).clone(),
extra_env: self.cargo_extraEnv(None).clone(),
Expand Down Expand Up @@ -1930,14 +1907,6 @@ impl Config {
crate::flycheck::InvocationStrategy::PerWorkspace
}
},
invocation_location: match self.check_invocationLocation(None) {
InvocationLocation::Root => {
crate::flycheck::InvocationLocation::Root(self.root_path.clone())
}
InvocationLocation::Workspace => {
crate::flycheck::InvocationLocation::Workspace
}
},
}
}
Some(_) | None => FlycheckConfig::CargoCommand {
Expand Down Expand Up @@ -2348,13 +2317,6 @@ pub(crate) enum InvocationStrategy {
#[derive(Serialize, Deserialize, Debug, Clone)]
struct CheckOnSaveTargets(#[serde(with = "single_or_array")] Vec<String>);

#[derive(Serialize, Deserialize, Debug, Clone)]
#[serde(rename_all = "snake_case")]
enum InvocationLocation {
Root,
Workspace,
}

#[derive(Serialize, Deserialize, Debug, Clone)]
#[serde(rename_all = "snake_case")]
enum LifetimeElisionDef {
Expand Down Expand Up @@ -3196,14 +3158,6 @@ fn field_props(field: &str, ty: &str, doc: &[&str], default: &str) -> serde_json
"The command will be executed once."
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to also mention the effect on the working directory here.

],
},
"InvocationLocation" => set! {
"type": "string",
"enum": ["workspace", "root"],
"enumDescriptions": [
"The command will be executed in the corresponding workspace root.",
"The command will be executed in the project root."
],
},
"Option<CheckOnSaveTargets>" => set! {
"anyOf": [
{
Expand Down
37 changes: 8 additions & 29 deletions crates/rust-analyzer/src/flycheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,13 @@ use toolchain::Tool;

use crate::command::{CommandHandle, ParseFromLine};

#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
#[derive(Clone, Debug, Default, PartialEq, Eq)]
pub(crate) enum InvocationStrategy {
Once,
#[default]
PerWorkspace,
}

#[derive(Clone, Debug, Default, PartialEq, Eq)]
pub(crate) enum InvocationLocation {
Root(AbsPathBuf),
#[default]
Workspace,
}

#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) struct CargoOptions {
pub(crate) target_triples: Vec<String>,
Expand Down Expand Up @@ -79,7 +72,6 @@ pub(crate) enum FlycheckConfig {
args: Vec<String>,
extra_env: FxHashMap<String, String>,
invocation_strategy: InvocationStrategy,
invocation_location: InvocationLocation,
},
}

Expand Down Expand Up @@ -424,30 +416,17 @@ impl FlycheckActor {
cmd.args(&options.extra_args);
Some(cmd)
}
FlycheckConfig::CustomCommand {
command,
args,
extra_env,
invocation_strategy,
invocation_location,
} => {
FlycheckConfig::CustomCommand { command, args, extra_env, invocation_strategy } => {
let mut cmd = Command::new(command);
cmd.envs(extra_env);

match invocation_location {
InvocationLocation::Workspace => {
match invocation_strategy {
InvocationStrategy::Once => {
cmd.current_dir(&self.root);
}
InvocationStrategy::PerWorkspace => {
// FIXME: cmd.current_dir(&affected_workspace);
cmd.current_dir(&self.root);
}
}
match invocation_strategy {
InvocationStrategy::Once => {
cmd.current_dir(&self.root);
}
InvocationLocation::Root(root) => {
cmd.current_dir(root);
InvocationStrategy::PerWorkspace => {
// FIXME: cmd.current_dir(&affected_workspace);
cmd.current_dir(&self.root);
}
}

Expand Down
22 changes: 13 additions & 9 deletions crates/rust-analyzer/src/reload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -764,18 +764,22 @@ impl GlobalState {
FlycheckConfig::CargoCommand { .. } => {
crate::flycheck::InvocationStrategy::PerWorkspace
}
FlycheckConfig::CustomCommand { invocation_strategy, .. } => invocation_strategy,
FlycheckConfig::CustomCommand { ref invocation_strategy, .. } => {
invocation_strategy.clone()
}
};

self.flycheck = match invocation_strategy {
crate::flycheck::InvocationStrategy::Once => vec![FlycheckHandle::spawn(
0,
sender,
config,
None,
self.config.root_path().clone(),
None,
)],
crate::flycheck::InvocationStrategy::Once => {
vec![FlycheckHandle::spawn(
0,
sender,
config,
None,
self.config.root_path().clone(),
None,
)]
}
crate::flycheck::InvocationStrategy::PerWorkspace => {
self.workspaces
.iter()
Expand Down
26 changes: 2 additions & 24 deletions docs/user/generated_config.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,6 @@ Automatically refresh project info via `cargo metadata` on
--
Run build scripts (`build.rs`) for more precise code analysis.
--
[[rust-analyzer.cargo.buildScripts.invocationLocation]]rust-analyzer.cargo.buildScripts.invocationLocation (default: `"workspace"`)::
+
--
Specifies the working directory for running build scripts.
- "workspace": run build scripts for a workspace in the workspace's root directory.
This is incompatible with `#rust-analyzer.cargo.buildScripts.invocationStrategy#` set to `once`.
- "root": run build scripts in the project's root directory.
This config only has an effect when `#rust-analyzer.cargo.buildScripts.overrideCommand#`
is set.
--
[[rust-analyzer.cargo.buildScripts.invocationStrategy]]rust-analyzer.cargo.buildScripts.invocationStrategy (default: `"per_workspace"`)::
+
--
Expand All @@ -75,8 +65,7 @@ option.
If there are multiple linked projects/workspaces, this command is invoked for
each of them, with the working directory being the workspace root
(i.e., the folder containing the `Cargo.toml`). This can be overwritten
by changing `#rust-analyzer.cargo.buildScripts.invocationStrategy#` and
`#rust-analyzer.cargo.buildScripts.invocationLocation#`.
by changing `#rust-analyzer.cargo.buildScripts.invocationStrategy#`.

By default, a cargo invocation will be constructed for the configured
targets and features, with the following base command line:
Expand Down Expand Up @@ -209,16 +198,6 @@ List of `cargo check` (or other command specified in `check.command`) diagnostic

For example for `cargo check`: `dead_code`, `unused_imports`, `unused_variables`,...
--
[[rust-analyzer.check.invocationLocation]]rust-analyzer.check.invocationLocation (default: `"workspace"`)::
+
--
Specifies the working directory for running checks.
- "workspace": run checks for workspaces in the corresponding workspaces' root directories.
This falls back to "root" if `#rust-analyzer.check.invocationStrategy#` is set to `once`.
- "root": run checks in the project's root directory.
This config only has an effect when `#rust-analyzer.check.overrideCommand#`
is set.
--
[[rust-analyzer.check.invocationStrategy]]rust-analyzer.check.invocationStrategy (default: `"per_workspace"`)::
+
--
Expand Down Expand Up @@ -250,8 +229,7 @@ Cargo, you might also want to change
If there are multiple linked projects/workspaces, this command is invoked for
each of them, with the working directory being the workspace root
(i.e., the folder containing the `Cargo.toml`). This can be overwritten
by changing `#rust-analyzer.check.invocationStrategy#` and
`#rust-analyzer.check.invocationLocation#`.
by changing `#rust-analyzer.check.invocationStrategy#`.

If `$saved_file` is part of the command, rust-analyzer will pass
the absolute path of the saved file to the provided command. This is
Expand Down
Loading