From 59ba386bee974b56b546eb7e8fdec6721ab0d08a Mon Sep 17 00:00:00 2001 From: veetaha Date: Wed, 18 Mar 2020 22:49:14 +0200 Subject: ra_cargo_watch: log more errors --- crates/ra_cargo_watch/src/lib.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index bffe5eb00..deb763af9 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -259,7 +259,7 @@ pub fn run_cargo( let mut child = command .args(args) .stdout(Stdio::piped()) - .stderr(Stdio::null()) + .stderr(Stdio::piped()) .stdin(Stdio::null()) .spawn() .expect("couldn't launch cargo"); @@ -352,8 +352,21 @@ impl WatchThread { // It is okay to ignore the result, as it only errors if the process is already dead let _ = child.kill(); - // Again, we don't care about the exit status so just ignore the result - let _ = child.wait(); + // Again, we are resilient to errors, so we don't try to panic here + match child.wait_with_output() { + Ok(output) => match output.status.code() { + Some(0) | None => {} + Some(exit_code) => { + let output = + std::str::from_utf8(&output.stderr).unwrap_or(""); + + if !output.contains("could not compile") { + log::error!("Cargo failed with exit code {} {}", exit_code, output); + } + } + }, + Err(err) => log::error!("Cargo io error: {:?}", err), + } })) } else { None -- cgit v1.2.3 From ce73c43848aa394530693967b8a6e9343f7b99b8 Mon Sep 17 00:00:00 2001 From: veetaha Date: Sat, 21 Mar 2020 23:30:33 +0200 Subject: ra_cargo_watch: return Result<> from run_cargo(), and don't read stderr for now As stated by matklad, reading the stderr should be done alngside with stdout via select() (or I guess poll()), there is no such implementation in stdlib, since it is quite low level and platform-dependent and it also requires quite a bit of unrelated code we don't use it for now. As referenced by bjorn3, there is an implementation of the needed read2() function in rustc compiletest. The better solution will be to extract this function to a separate crate in future: https://github.com/rust-analyzer/rust-analyzer/pull/3632#discussion_r395605298 --- crates/ra_cargo_watch/src/lib.rs | 78 ++++++++++++++------------ crates/ra_project_model/src/cargo_workspace.rs | 27 +++++---- 2 files changed, 56 insertions(+), 49 deletions(-) diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index deb763af9..35b0cb608 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -8,9 +8,10 @@ use lsp_types::{ WorkDoneProgressEnd, WorkDoneProgressReport, }; use std::{ + error, fmt, io::{BufRead, BufReader}, path::{Path, PathBuf}, - process::{Child, Command, Stdio}, + process::{Command, Stdio}, thread::JoinHandle, time::Instant, }; @@ -70,10 +71,10 @@ impl std::ops::Drop for CheckWatcher { fn drop(&mut self) { if let Some(handle) = self.handle.take() { // Take the sender out of the option - let recv = self.cmd_send.take(); + let cmd_send = self.cmd_send.take(); // Dropping the sender finishes the thread loop - drop(recv); + drop(cmd_send); // Join the thread, it should finish shortly. We don't really care // whether it panicked, so it is safe to ignore the result @@ -246,11 +247,21 @@ enum CheckEvent { End, } +#[derive(Debug)] +pub struct CargoError(String); + +impl fmt::Display for CargoError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "Cargo failed: {}", self.0) + } +} +impl error::Error for CargoError {} + pub fn run_cargo( args: &[String], current_dir: Option<&Path>, on_message: &mut dyn FnMut(cargo_metadata::Message) -> bool, -) -> Child { +) -> Result<(), CargoError> { let mut command = Command::new("cargo"); if let Some(current_dir) = current_dir { command.current_dir(current_dir); @@ -259,7 +270,7 @@ pub fn run_cargo( let mut child = command .args(args) .stdout(Stdio::piped()) - .stderr(Stdio::piped()) + .stderr(Stdio::null()) .stdin(Stdio::null()) .spawn() .expect("couldn't launch cargo"); @@ -273,6 +284,8 @@ pub fn run_cargo( // simply skip a line if it doesn't parse, which just ignores any // erroneus output. let stdout = BufReader::new(child.stdout.take().unwrap()); + let mut read_at_least_one_message = false; + for line in stdout.lines() { let line = match line { Ok(line) => line, @@ -291,12 +304,27 @@ pub fn run_cargo( } }; + read_at_least_one_message = true; + if !on_message(message) { break; } } - child + // It is okay to ignore the result, as it only errors if the process is already dead + let _ = child.kill(); + + let err_msg = match child.wait() { + Ok(exit_code) if !exit_code.success() && !read_at_least_one_message => { + // FIXME: Read the stderr to display the reason, see `read2()` reference in PR comment: + // https://github.com/rust-analyzer/rust-analyzer/pull/3632#discussion_r395605298 + format!("the command produced no valid metadata:\n cargo {}", args.join(" ")) + } + Err(err) => format!("io error: {:?}", err), + Ok(_) => return Ok(()), + }; + + Err(CargoError(err_msg)) } impl WatchThread { @@ -325,7 +353,7 @@ impl WatchThread { // which will break out of the loop, and continue the shutdown let _ = message_send.send(CheckEvent::Begin); - let mut child = run_cargo(&args, Some(&workspace_root), &mut |message| { + let res = run_cargo(&args, Some(&workspace_root), &mut |message| { // Skip certain kinds of messages to only spend time on what's useful match &message { Message::CompilerArtifact(artifact) if artifact.fresh => return true, @@ -334,39 +362,19 @@ impl WatchThread { _ => {} } - match message_send.send(CheckEvent::Msg(message)) { - Ok(()) => {} - Err(_err) => { - // The send channel was closed, so we want to shutdown - return false; - } - }; - - true + // if the send channel was closed, so we want to shutdown + message_send.send(CheckEvent::Msg(message)).is_ok() }); + if let Err(err) = res { + // FIXME: make the `message_send` to be `Sender>` + // to display user-caused misconfiguration errors instead of just logging them here + log::error!("Cargo watcher failed {:?}", err); + } + // We can ignore any error here, as we are already in the progress // of shutting down. let _ = message_send.send(CheckEvent::End); - - // It is okay to ignore the result, as it only errors if the process is already dead - let _ = child.kill(); - - // Again, we are resilient to errors, so we don't try to panic here - match child.wait_with_output() { - Ok(output) => match output.status.code() { - Some(0) | None => {} - Some(exit_code) => { - let output = - std::str::from_utf8(&output.stderr).unwrap_or(""); - - if !output.contains("could not compile") { - log::error!("Cargo failed with exit code {} {}", exit_code, output); - } - } - }, - Err(err) => log::error!("Cargo io error: {:?}", err), - } })) } else { None diff --git a/crates/ra_project_model/src/cargo_workspace.rs b/crates/ra_project_model/src/cargo_workspace.rs index c2857dbfc..c7f9bd873 100644 --- a/crates/ra_project_model/src/cargo_workspace.rs +++ b/crates/ra_project_model/src/cargo_workspace.rs @@ -6,7 +6,7 @@ use std::{ }; use anyhow::{Context, Result}; -use cargo_metadata::{CargoOpt, Message, MetadataCommand, PackageId}; +use cargo_metadata::{BuildScript, CargoOpt, Message, MetadataCommand, PackageId}; use ra_arena::{Arena, Idx}; use ra_cargo_watch::run_cargo; use ra_db::Edition; @@ -254,7 +254,7 @@ pub fn load_out_dirs( "check".to_string(), "--message-format=json".to_string(), "--manifest-path".to_string(), - format!("{}", cargo_toml.display()), + cargo_toml.display().to_string(), ]; if cargo_features.all_features { @@ -263,19 +263,15 @@ pub fn load_out_dirs( // FIXME: `NoDefaultFeatures` is mutual exclusive with `SomeFeatures` // https://github.com/oli-obk/cargo_metadata/issues/79 args.push("--no-default-features".to_string()); - } else if !cargo_features.features.is_empty() { - for feature in &cargo_features.features { - args.push(feature.clone()); - } + } else { + args.extend(cargo_features.features.iter().cloned()); } - let mut res = FxHashMap::default(); - let mut child = run_cargo(&args, cargo_toml.parent(), &mut |message| { + let mut acc = FxHashMap::default(); + let res = run_cargo(&args, cargo_toml.parent(), &mut |message| { match message { - Message::BuildScriptExecuted(message) => { - let package_id = message.package_id; - let out_dir = message.out_dir; - res.insert(package_id, out_dir); + Message::BuildScriptExecuted(BuildScript { package_id, out_dir, .. }) => { + acc.insert(package_id, out_dir); } Message::CompilerArtifact(_) => (), @@ -285,6 +281,9 @@ pub fn load_out_dirs( true }); - let _ = child.wait(); - res + if let Err(err) = res { + log::error!("Failed to load outdirs: {:?}", err); + } + + acc } -- cgit v1.2.3 From 84143426409ac0d62b19d0525ab8900a1ba8692a Mon Sep 17 00:00:00 2001 From: veetaha Date: Sat, 21 Mar 2020 23:36:01 +0200 Subject: fix: typo --- crates/ra_cargo_watch/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index 35b0cb608..eb8ba4da8 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -362,7 +362,7 @@ impl WatchThread { _ => {} } - // if the send channel was closed, so we want to shutdown + // if the send channel was closed, we want to shutdown message_send.send(CheckEvent::Msg(message)).is_ok() }); -- cgit v1.2.3 From 788b29d343a3f4af6658e385621f1d1451d529e2 Mon Sep 17 00:00:00 2001 From: Veetaha Date: Sun, 22 Mar 2020 01:27:03 +0200 Subject: Smol self-nit --- crates/ra_cargo_watch/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index eb8ba4da8..362d32010 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -318,7 +318,7 @@ pub fn run_cargo( Ok(exit_code) if !exit_code.success() && !read_at_least_one_message => { // FIXME: Read the stderr to display the reason, see `read2()` reference in PR comment: // https://github.com/rust-analyzer/rust-analyzer/pull/3632#discussion_r395605298 - format!("the command produced no valid metadata:\n cargo {}", args.join(" ")) + format!("the command produced no valid metadata: cargo {}", args.join(" ")) } Err(err) => format!("io error: {:?}", err), Ok(_) => return Ok(()), -- cgit v1.2.3 From 8be28a2d4f1fa1593bab81e32e465dba35b99448 Mon Sep 17 00:00:00 2001 From: veetaha Date: Sun, 22 Mar 2020 12:01:49 +0200 Subject: ra_cargo_watch: log exit code too --- crates/ra_cargo_watch/src/lib.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index 362d32010..7c525c430 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -318,7 +318,11 @@ pub fn run_cargo( Ok(exit_code) if !exit_code.success() && !read_at_least_one_message => { // FIXME: Read the stderr to display the reason, see `read2()` reference in PR comment: // https://github.com/rust-analyzer/rust-analyzer/pull/3632#discussion_r395605298 - format!("the command produced no valid metadata: cargo {}", args.join(" ")) + format!( + "the command produced no valid metadata (exit code: {:?}): cargo {}", + exit_code, + args.join(" ") + ) } Err(err) => format!("io error: {:?}", err), Ok(_) => return Ok(()), -- cgit v1.2.3