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.

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

Add $manifest_path substitution for cargo override commands #13528

Closed
wants to merge 1 commit into from
Closed
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
70 changes: 39 additions & 31 deletions crates/flycheck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,9 @@ impl FlycheckHandle {
sender: Box<dyn Fn(Message) + Send>,
config: FlycheckConfig,
workspace_root: AbsPathBuf,
project_file: AbsPathBuf,
) -> FlycheckHandle {
let actor = FlycheckActor::new(id, sender, config, workspace_root);
let actor = FlycheckActor::new(id, sender, config, workspace_root, project_file);
let (sender, receiver) = unbounded::<StateChange>();
let thread = stdx::thread::Builder::new(stdx::thread::ThreadIntent::Worker)
.name("Flycheck".to_owned())
Expand All @@ -100,8 +101,8 @@ impl FlycheckHandle {
}

/// Schedule a re-start of the cargo check worker.
pub fn restart(&self) {
self.sender.send(StateChange::Restart).unwrap();
pub fn restart(&self, saved_file: Option<AbsPathBuf>) {
self.sender.send(StateChange::Restart { saved_file }).unwrap();
}

/// Stop this cargo check worker.
Expand Down Expand Up @@ -152,7 +153,7 @@ pub enum Progress {
}

enum StateChange {
Restart,
Restart { saved_file: Option<AbsPathBuf> },
Cancel,
}

Expand All @@ -165,6 +166,8 @@ struct FlycheckActor {
/// Either the workspace root of the workspace we are flychecking,
/// or the project root of the project.
root: AbsPathBuf,
/// The Cargo.toml or rust-project.json file of the project.
project_file: AbsPathBuf,
Comment on lines +169 to +170
Copy link
Contributor

Choose a reason for hiding this comment

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

naive question: while i think it's reasonable to make this required to start with, do you think this can be expended to not be a file and instead load this rust-project.json over some process that dynamically creates this?

(idk if this question is better addressed in a dedicated issue.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you mean, but for flychecking to occur, we already need to have a project structure which implies that there is at least one rust-project.json or Cargo.toml loaded

Copy link
Contributor

Choose a reason for hiding this comment

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

...I think I mixed up what this PR was doing, I'm sorry about that! Bit tired.

Anyways: while I get that rust-project.json does need to be loaded for flycheck (and any analysis from rust-analyzer), I'm wondering if it needs to be loaded from file, as opposed to be read in via stdout or UDS, or anything along those lines.

Copy link
Member Author

@Veykril Veykril Nov 2, 2022

Choose a reason for hiding this comment

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

(I assume this is in regards to project loading, otherwise I am not sure I understand) not necessarily, I could imagine an lsp extension for fetching the project structure maybe?

/// CargoHandle exists to wrap around the communication needed to be able to
/// run `cargo check` without blocking. Currently the Rust standard library
/// doesn't provide a way to read sub-process output without blocking, so we
Expand All @@ -184,9 +187,17 @@ impl FlycheckActor {
sender: Box<dyn Fn(Message) + Send>,
config: FlycheckConfig,
workspace_root: AbsPathBuf,
project_file: AbsPathBuf,
) -> FlycheckActor {
tracing::info!(%id, ?workspace_root, "Spawning flycheck");
FlycheckActor { id, sender, config, root: workspace_root, command_handle: None }
FlycheckActor {
id,
sender,
config,
root: workspace_root,
project_file,
command_handle: None,
}
}

fn report_progress(&self, progress: Progress) {
Expand All @@ -212,7 +223,7 @@ impl FlycheckActor {
tracing::debug!(flycheck_id = self.id, "flycheck cancelled");
self.cancel_check_process();
}
Event::RequestStateChange(StateChange::Restart) => {
Event::RequestStateChange(StateChange::Restart { saved_file }) => {
// Cancel the previously spawned process
self.cancel_check_process();
while let Ok(restart) = inbox.recv_timeout(Duration::from_millis(50)) {
Expand All @@ -222,7 +233,7 @@ impl FlycheckActor {
}
}

let command = self.check_command();
let command = self.check_command(saved_file);
let formatted_command = format!("{:?}", command);

tracing::debug!(?command, "will restart flycheck");
Expand Down Expand Up @@ -296,8 +307,10 @@ impl FlycheckActor {
}
}

fn check_command(&self) -> Command {
let (mut cmd, args) = match &self.config {
fn check_command(&self, _saved_file: Option<AbsPathBuf>) -> Command {
// FIXME: Figure out the story for exposing the saved file to the custom flycheck command
// as it can be absent
match &self.config {
FlycheckConfig::CargoCommand {
command,
target_triples,
Expand All @@ -312,7 +325,7 @@ impl FlycheckActor {
let mut cmd = Command::new(toolchain::cargo());
cmd.arg(command);
cmd.current_dir(&self.root);
cmd.arg("--workspace");
cmd.args(&["--workspace", "--manifest-path"]).arg(self.project_file.as_os_str());

cmd.arg(if *ansi_color_output {
"--message-format=json-diagnostic-rendered-ansi"
Expand Down Expand Up @@ -341,7 +354,8 @@ impl FlycheckActor {
}
}
cmd.envs(extra_env);
(cmd, extra_args)
cmd.args(extra_args);
cmd
}
FlycheckConfig::CustomCommand {
command,
Expand All @@ -352,30 +366,24 @@ impl FlycheckActor {
} => {
let mut cmd = Command::new(command);
cmd.envs(extra_env);
args.iter().for_each(|arg| {
match invocation_strategy {
InvocationStrategy::Once => cmd.arg(arg),
InvocationStrategy::PerWorkspace => cmd.arg(arg.replace(
"$manifest_path",
&self.project_file.as_os_str().to_string_lossy(),
)),
};
});

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

(cmd, args)
cmd
}
};

cmd.args(args);
cmd
}
}

fn send(&self, check_task: Message) {
Expand Down
3 changes: 3 additions & 0 deletions crates/paths/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,9 @@ impl AbsPath {
pub fn exists(&self) -> bool {
self.0.exists()
}
pub fn with_extension<S: AsRef<OsStr>>(&self, extension: S) -> AbsPathBuf {
AbsPathBuf::try_from(self.0.with_extension(extension)).unwrap()
}
// endregion:delegate-methods
}

Expand Down
28 changes: 23 additions & 5 deletions crates/project-model/src/build_scripts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,21 @@ impl WorkspaceBuildScripts {
fn build_command(
config: &CargoConfig,
allowed_features: &FxHashSet<String>,
manifest_path: Option<AbsPathBuf>,
) -> io::Result<Command> {
let mut cmd = match config.run_build_script_command.as_deref() {
Some([program, args @ ..]) => {
let mut cmd = Command::new(program);
cmd.args(args);
for arg in args {
if let Some(manifest_path) = &manifest_path {
cmd.arg(arg.replace(
"$manifest_path",
&manifest_path.as_os_str().to_string_lossy(),
));
} else {
cmd.arg(arg);
}
}
cmd
}
_ => {
Expand Down Expand Up @@ -139,7 +149,11 @@ impl WorkspaceBuildScripts {
let allowed_features = workspace.workspace_features();

match Self::run_per_ws(
Self::build_command(config, &allowed_features)?,
Self::build_command(
config,
&allowed_features,
Some(workspace.workspace_root().join("Cargo.toml")),
)?,
workspace,
current_dir,
progress,
Expand All @@ -149,8 +163,12 @@ impl WorkspaceBuildScripts {
{
// 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, &allowed_features)?;
cmd.args(["-Z", "unstable-options", "--keep-going"]).env("RUSTC_BOOTSTRAP", "1");
let mut cmd = Self::build_command(
config,
&allowed_features,
Some(workspace.workspace_root().join("Cargo.toml")),
)?;
cmd.args(&["-Z", "unstable-options", "--keep-going"]).env("RUSTC_BOOTSTRAP", "1");
let mut res = Self::run_per_ws(cmd, workspace, current_dir, progress)?;
res.error = Some(error);
Ok(res)
Expand All @@ -177,7 +195,7 @@ impl WorkspaceBuildScripts {
))
}
};
let cmd = Self::build_command(config, &Default::default())?;
let cmd = Self::build_command(config, &Default::default(), None)?;
// 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
1 change: 1 addition & 0 deletions crates/project-model/src/project_json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ pub struct ProjectJson {
pub(crate) sysroot: Option<AbsPathBuf>,
/// e.g. `path/to/sysroot/lib/rustlib/src/rust`
pub(crate) sysroot_src: Option<AbsPathBuf>,
/// the folder containing `rust-project.json`)
project_root: AbsPathBuf,
crates: Vec<Crate>,
}
Expand Down
12 changes: 8 additions & 4 deletions crates/rust-analyzer/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,10 @@ config_data! {
/// 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.
/// If `once` is set, the command will be executed once.
/// - `per_workspace`: Executes the command for each workspace separately.
/// Allows the use of the `$manifest_path` substitution variable in the override command
/// which will be replaced by the path of the workspace's Cargo.toml.
/// - `once`: Executes the command 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 @@ -164,8 +166,10 @@ config_data! {
/// is set.
check_invocationLocation | checkOnSave_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.
/// - `per_workspace`: Executes the command for each workspace separately.
/// Allows the use of the `$manifest_path` substitution variable in the override command
/// which will be replaced by the path of the workspace's Cargo.toml or rust-project.json.
/// - `once`: Executes the command once.
/// This config only has an effect when `#rust-analyzer.cargo.check.overrideCommand#`
/// is set.
check_invocationStrategy | checkOnSave_invocationStrategy: InvocationStrategy = "\"per_workspace\"",
Expand Down
22 changes: 12 additions & 10 deletions crates/rust-analyzer/src/handlers/notification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use lsp_types::{
DidOpenTextDocumentParams, DidSaveTextDocumentParams, WorkDoneProgressCancelParams,
};
use triomphe::Arc;
use vfs::{AbsPathBuf, ChangeKind, VfsPath};
use vfs::{AbsPath, AbsPathBuf, ChangeKind, VfsPath};

use crate::{
config::Config,
Expand Down Expand Up @@ -132,21 +132,22 @@ pub(crate) fn handle_did_save_text_document(
) -> anyhow::Result<()> {
if let Ok(vfs_path) = from_proto::vfs_path(&params.text_document.uri) {
// Re-fetch workspaces if a workspace related file has changed
if let Some(abs_path) = vfs_path.as_path() {
let abs_path = vfs_path.as_path();
if let Some(abs_path) = abs_path {
if reload::should_refresh_for_change(abs_path, ChangeKind::Modify) {
state
.fetch_workspaces_queue
.request_op(format!("DidSaveTextDocument {abs_path}"), false);
}
}

if !state.config.check_on_save() || run_flycheck(state, vfs_path) {
if !state.config.check_on_save() || run_flycheck(state, &vfs_path, abs_path) {
return Ok(());
}
} else if state.config.check_on_save() {
// No specific flycheck was triggered, so let's trigger all of them.
for flycheck in state.flycheck.iter() {
flycheck.restart();
flycheck.restart(None);
}
}
Ok(())
Expand Down Expand Up @@ -232,10 +233,11 @@ pub(crate) fn handle_did_change_watched_files(
Ok(())
}

fn run_flycheck(state: &mut GlobalState, vfs_path: VfsPath) -> bool {
fn run_flycheck(state: &mut GlobalState, vfs_path: &VfsPath, abs_path: Option<&AbsPath>) -> bool {
let _p = profile::span("run_flycheck");

let file_id = state.vfs.read().0.file_id(&vfs_path);
let file_id = state.vfs.read().0.file_id(vfs_path);
let abs_path = abs_path.map(AbsPath::to_owned);
if let Some(file_id) = file_id {
let world = state.snapshot();
let mut updated = false;
Expand Down Expand Up @@ -287,15 +289,15 @@ fn run_flycheck(state: &mut GlobalState, vfs_path: VfsPath) -> bool {
for (id, _) in workspace_ids.clone() {
if id == flycheck.id() {
updated = true;
flycheck.restart();
flycheck.restart(abs_path.clone());
continue;
}
}
}
// No specific flycheck was triggered, so let's trigger all of them.
if !updated {
for flycheck in world.flycheck.iter() {
flycheck.restart();
flycheck.restart(abs_path.clone());
}
}
Ok(())
Expand Down Expand Up @@ -330,14 +332,14 @@ pub(crate) fn handle_run_flycheck(
let _p = profile::span("handle_run_flycheck");
if let Some(text_document) = params.text_document {
if let Ok(vfs_path) = from_proto::vfs_path(&text_document.uri) {
if run_flycheck(state, vfs_path) {
if run_flycheck(state, &vfs_path, vfs_path.as_path()) {
return Ok(());
}
}
}
// No specific flycheck was triggered, so let's trigger all of them.
for flycheck in state.flycheck.iter() {
flycheck.restart();
flycheck.restart(None);
}
Ok(())
}
3 changes: 1 addition & 2 deletions crates/rust-analyzer/src/main_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use std::{

use always_assert::always;
use crossbeam_channel::{select, Receiver};
use flycheck::FlycheckHandle;
use ide_db::base_db::{SourceDatabaseExt, VfsPath};
use lsp_server::{Connection, Notification, Request};
use lsp_types::notification::Notification as _;
Expand Down Expand Up @@ -299,7 +298,7 @@ impl GlobalState {
if became_quiescent {
if self.config.check_on_save() {
// Project has loaded properly, kick off initial flycheck
self.flycheck.iter().for_each(FlycheckHandle::restart);
self.flycheck.iter().for_each(|flycheck| flycheck.restart(None));
}
if self.config.prefill_caches() {
self.prime_caches_queue.request_op("became quiescent".to_string(), ());
Expand Down
Loading