mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

ra_cargo_watch: return Result<> from run_cargo(), and don't read stderr for now

Browse Source 
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
This commit is contained in:
veetaha 2020-03-21 23:30:33 +02:00
parent 59ba386bee
commit ce73c43848
2 changed files with 56 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

78
crates/ra_cargo_watch/src/lib.rs
View File

@ -8,9 +8,10 @@
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 @@ fn new(options: &CheckOptions, workspace_root: &Path) -> 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 @@ fn new(options: &CheckOptions, workspace_root: &Path) -> 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<Result<CheckEvent, CargoError>>`
// 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("<bad utf8 output>");
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

27
crates/ra_project_model/src/cargo_workspace.rs
View File

@ -6,7 +6,7 @@
};
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
}