Skip to content

Commit

Permalink
Auto merge of #13466 - Veykril:invocation-location, r=Veykril
Browse files Browse the repository at this point in the history
Implement invocation location config

This allows setting the working directory for build-scripts on flycheck
Complements #13128

This will be followed up by one more PR that adds a few simple interpolation vars for `overrideCommand`, with that we should cover the needs for most build systems I believe.
  • Loading branch information
bors committed Oct 22, 2022
2 parents 19efa0b + 0f8904e commit b25f657
Show file tree
Hide file tree
Showing 9 changed files with 188 additions and 57 deletions.
48 changes: 38 additions & 10 deletions crates/flycheck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@ pub enum InvocationStrategy {
PerWorkspace,
}

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

#[derive(Clone, Debug, PartialEq, Eq)]
pub enum FlycheckConfig {
CargoCommand {
Expand All @@ -39,13 +46,13 @@ pub enum FlycheckConfig {
features: Vec<String>,
extra_args: Vec<String>,
extra_env: FxHashMap<String, String>,
invocation_strategy: InvocationStrategy,
},
CustomCommand {
command: String,
args: Vec<String>,
extra_env: FxHashMap<String, String>,
invocation_strategy: InvocationStrategy,
invocation_location: InvocationLocation,
},
}

Expand Down Expand Up @@ -275,7 +282,7 @@ impl FlycheckActor {
}

fn check_command(&self) -> Command {
let (mut cmd, args, invocation_strategy) = match &self.config {
let (mut cmd, args) = match &self.config {
FlycheckConfig::CargoCommand {
command,
target_triple,
Expand All @@ -285,7 +292,6 @@ impl FlycheckActor {
extra_args,
features,
extra_env,
invocation_strategy,
} => {
let mut cmd = Command::new(toolchain::cargo());
cmd.arg(command);
Expand All @@ -309,18 +315,40 @@ impl FlycheckActor {
}
}
cmd.envs(extra_env);
(cmd, extra_args, invocation_strategy)
(cmd, extra_args)
}
FlycheckConfig::CustomCommand { command, args, extra_env, invocation_strategy } => {
FlycheckConfig::CustomCommand {
command,
args,
extra_env,
invocation_strategy,
invocation_location,
} => {
let mut cmd = Command::new(command);
cmd.envs(extra_env);
(cmd, args, invocation_strategy)

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);
}
}
}
InvocationLocation::Root(root) => {
cmd.current_dir(root);
}
}

(cmd, args)
}
};
match invocation_strategy {
InvocationStrategy::PerWorkspace => cmd.current_dir(&self.root),
InvocationStrategy::Once => cmd.args(args),
};

cmd.args(args);
cmd
}

Expand Down
41 changes: 24 additions & 17 deletions crates/project-model/src/build_scripts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ use semver::Version;
use serde::Deserialize;

use crate::{
cfg_flag::CfgFlag, CargoConfig, CargoFeatures, CargoWorkspace, InvocationStrategy, Package,
cfg_flag::CfgFlag, CargoConfig, CargoFeatures, CargoWorkspace, InvocationLocation,
InvocationStrategy, Package,
};

#[derive(Debug, Default, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -55,10 +56,7 @@ impl BuildScriptOutput {
}

impl WorkspaceBuildScripts {
fn build_command(
config: &CargoConfig,
workspace_root: Option<&path::Path>,
) -> io::Result<Command> {
fn build_command(config: &CargoConfig, current_dir: &path::Path) -> io::Result<Command> {
let mut cmd = match config.run_build_script_command.as_deref() {
Some([program, args @ ..]) => {
let mut cmd = Command::new(program);
Expand Down Expand Up @@ -94,14 +92,11 @@ impl WorkspaceBuildScripts {
}
}

if let Some(workspace_root) = workspace_root {
cmd.current_dir(workspace_root);
}

cmd
}
};

cmd.current_dir(current_dir);
cmd.envs(&config.extra_env);
if config.wrap_rustc_in_build_scripts {
// Setup RUSTC_WRAPPER to point to `rust-analyzer` binary itself. We use
Expand All @@ -124,19 +119,21 @@ impl WorkspaceBuildScripts {
) -> io::Result<WorkspaceBuildScripts> {
const RUST_1_62: Version = Version::new(1, 62, 0);

let workspace_root: &path::Path = &workspace.workspace_root().as_ref();
let current_dir = match &config.invocation_location {
InvocationLocation::Root(root) if config.run_build_script_command.is_some() => {
root.as_path()
}
_ => &workspace.workspace_root(),
}
.as_ref();

match Self::run_per_ws(
Self::build_command(config, Some(workspace_root))?,
workspace,
progress,
) {
match Self::run_per_ws(Self::build_command(config, current_dir)?, workspace, progress) {
Ok(WorkspaceBuildScripts { error: Some(error), .. })
if toolchain.as_ref().map_or(false, |it| *it >= RUST_1_62) =>
{
// building build scripts failed, attempt to build with --keep-going so
// that we potentially get more build data
let mut cmd = Self::build_command(config, Some(workspace_root))?;
let mut cmd = Self::build_command(config, current_dir)?;
cmd.args(&["-Z", "unstable-options", "--keep-going"]).env("RUSTC_BOOTSTRAP", "1");
let mut res = Self::run_per_ws(cmd, workspace, progress)?;
res.error = Some(error);
Expand All @@ -154,7 +151,17 @@ impl WorkspaceBuildScripts {
progress: &dyn Fn(String),
) -> io::Result<Vec<WorkspaceBuildScripts>> {
assert_eq!(config.invocation_strategy, InvocationStrategy::Once);
let cmd = Self::build_command(config, None)?;

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 cmd = Self::build_command(config, current_dir.as_path().as_ref())?;
// NB: Cargo.toml could have been modified between `cargo metadata` and
// `cargo check`. We shouldn't assume that package ids we see here are
// exactly those from `config`.
Expand Down
3 changes: 2 additions & 1 deletion crates/project-model/src/cargo_workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use rustc_hash::FxHashMap;
use serde::Deserialize;
use serde_json::from_value;

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

/// [`CargoWorkspace`] represents the logical structure of, well, a Cargo
Expand Down Expand Up @@ -107,6 +107,7 @@ 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,
}

impl CargoConfig {
Expand Down
7 changes: 7 additions & 0 deletions crates/project-model/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,3 +164,10 @@ pub enum InvocationStrategy {
#[default]
PerWorkspace,
}

#[derive(Clone, Debug, Default, PartialEq, Eq)]
pub enum InvocationLocation {
Root(AbsPathBuf),
#[default]
Workspace,
}
4 changes: 3 additions & 1 deletion crates/project-model/src/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,9 @@ impl ProjectWorkspace {
config: &CargoConfig,
progress: &dyn Fn(String),
) -> Vec<Result<WorkspaceBuildScripts>> {
if let InvocationStrategy::PerWorkspace = config.invocation_strategy {
if matches!(config.invocation_strategy, InvocationStrategy::PerWorkspace)
|| config.run_build_script_command.is_some()
{
return workspaces.iter().map(|it| it.run_build_scripts(config, progress)).collect();
}

Expand Down
68 changes: 54 additions & 14 deletions crates/rust-analyzer/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,16 @@ config_data! {
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 = "\"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 from the
/// corresponding workspace root.
/// If `once` is set, the command will be executed once in the project root.
/// If `per_workspace` is set, the command will be executed for each workspace.
/// If `once` is set, the command will be executed once.
/// This config only has an effect when `#rust-analyzer.cargo.buildScripts.overrideCommand#`
/// is set.
cargo_buildScripts_invocationStrategy: InvocationStrategy = "\"per_workspace\"",
Expand Down Expand Up @@ -129,10 +135,17 @@ config_data! {
///
/// Set to `"all"` to pass `--all-features` to Cargo.
checkOnSave_features: Option<CargoFeaturesDef> = "null",
/// 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.cargo.checkOnSave.invocationStrategy#` is set to `once`.
/// - "root": run checks in the project's root directory.
/// This config only has an effect when `#rust-analyzer.cargo.buildScripts.overrideCommand#`
/// is set.
checkOnSave_invocationLocation: InvocationLocation = "\"workspace\"",
/// Specifies the invocation strategy to use when running the checkOnSave command.
/// If `per_workspace` is set, the command will be executed for each workspace from the
/// corresponding workspace root.
/// If `once` is set, the command will be executed once in the project root.
/// If `per_workspace` is set, the command will be executed for each workspace.
/// If `once` is set, the command will be executed once.
/// This config only has an effect when `#rust-analyzer.cargo.buildScripts.overrideCommand#`
/// is set.
checkOnSave_invocationStrategy: InvocationStrategy = "\"per_workspace\"",
Expand Down Expand Up @@ -1074,6 +1087,12 @@ impl Config {
InvocationStrategy::Once => project_model::InvocationStrategy::Once,
InvocationStrategy::PerWorkspace => project_model::InvocationStrategy::PerWorkspace,
},
invocation_location: match self.data.cargo_buildScripts_invocationLocation {
InvocationLocation::Root => {
project_model::InvocationLocation::Root(self.root_path.clone())
}
InvocationLocation::Workspace => project_model::InvocationLocation::Workspace,
},
run_build_script_command: self.data.cargo_buildScripts_overrideCommand.clone(),
extra_env: self.data.cargo_extraEnv.clone(),
}
Expand All @@ -1097,10 +1116,6 @@ impl Config {
if !self.data.checkOnSave_enable {
return None;
}
let invocation_strategy = match self.data.checkOnSave_invocationStrategy {
InvocationStrategy::Once => flycheck::InvocationStrategy::Once,
InvocationStrategy::PerWorkspace => flycheck::InvocationStrategy::PerWorkspace,
};
let flycheck_config = match &self.data.checkOnSave_overrideCommand {
Some(args) if !args.is_empty() => {
let mut args = args.clone();
Expand All @@ -1109,7 +1124,18 @@ impl Config {
command,
args,
extra_env: self.check_on_save_extra_env(),
invocation_strategy,
invocation_strategy: match self.data.checkOnSave_invocationStrategy {
InvocationStrategy::Once => flycheck::InvocationStrategy::Once,
InvocationStrategy::PerWorkspace => {
flycheck::InvocationStrategy::PerWorkspace
}
},
invocation_location: match self.data.checkOnSave_invocationLocation {
InvocationLocation::Root => {
flycheck::InvocationLocation::Root(self.root_path.clone())
}
InvocationLocation::Workspace => flycheck::InvocationLocation::Workspace,
},
}
}
Some(_) | None => FlycheckConfig::CargoCommand {
Expand Down Expand Up @@ -1139,7 +1165,6 @@ impl Config {
},
extra_args: self.data.checkOnSave_extraArgs.clone(),
extra_env: self.check_on_save_extra_env(),
invocation_strategy,
},
};
Some(flycheck_config)
Expand Down Expand Up @@ -1618,6 +1643,13 @@ enum InvocationStrategy {
PerWorkspace,
}

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

#[derive(Deserialize, Debug, Clone)]
#[serde(untagged)]
enum LifetimeElisionDef {
Expand Down Expand Up @@ -2036,8 +2068,16 @@ fn field_props(field: &str, ty: &str, doc: &[&str], default: &str) -> serde_json
"type": "string",
"enum": ["per_workspace", "once"],
"enumDescriptions": [
"The command will be executed for each workspace from the corresponding workspace root.",
"The command will be executed once in the project root."
"The command will be executed for each workspace.",
"The command will be executed once."
],
},
"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."
],
},
_ => panic!("missing entry for {}: {}", ty, default),
Expand Down
6 changes: 4 additions & 2 deletions crates/rust-analyzer/src/reload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,8 +473,10 @@ impl GlobalState {
};

let sender = self.flycheck_sender.clone();
let (FlycheckConfig::CargoCommand { invocation_strategy, .. }
| FlycheckConfig::CustomCommand { invocation_strategy, .. }) = config;
let invocation_strategy = match config {
FlycheckConfig::CargoCommand { .. } => flycheck::InvocationStrategy::PerWorkspace,
FlycheckConfig::CustomCommand { invocation_strategy, .. } => invocation_strategy,
};

self.flycheck = match invocation_strategy {
flycheck::InvocationStrategy::Once => vec![FlycheckHandle::spawn(
Expand Down
Loading

0 comments on commit b25f657

Please sign in to comment.