Skip to content

Commit a27f5e3

Browse files
bors[bot]Veetaha
andauthored
Merge #3632
3632: ra_cargo_watch: log errors r=matklad a=Veetaha Until this moment we totally ignored all the errors from cargo process. Though this is still true, but we now try to log ones that are critical (i.e. misconfiguration errors and ignore compile errors). This fixes #3631, and gives us a better error message to more gracefully handle the #3265 ![image](https://user-images.githubusercontent.com/36276403/76958683-d7e1f080-6920-11ea-83d8-04561c11ccc4.png) Though I think that outputting this only to `Output` channel is not enough. We should somehow warn the user that he passed wrong arguments to `cargo-watch.args`. I didn't bother looking for how to do this now, but this PR at least gives us something. *cc* @kiljacken @matklad Co-authored-by: veetaha <veetaha2@gmail.com> Co-authored-by: Veetaha <veetaha2@gmail.com>
2 parents c7a2052 + 8be28a2 commit a27f5e3

File tree

2 files changed

+59
-35
lines changed

2 files changed

+59
-35
lines changed

crates/ra_cargo_watch/src/lib.rs

Lines changed: 46 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,10 @@ use lsp_types::{
88
WorkDoneProgressEnd, WorkDoneProgressReport,
99
};
1010
use std::{
11+
error, fmt,
1112
io::{BufRead, BufReader},
1213
path::{Path, PathBuf},
13-
process::{Child, Command, Stdio},
14+
process::{Command, Stdio},
1415
thread::JoinHandle,
1516
time::Instant,
1617
};
@@ -70,10 +71,10 @@ impl std::ops::Drop for CheckWatcher {
7071
fn drop(&mut self) {
7172
if let Some(handle) = self.handle.take() {
7273
// Take the sender out of the option
73-
let recv = self.cmd_send.take();
74+
let cmd_send = self.cmd_send.take();
7475

7576
// Dropping the sender finishes the thread loop
76-
drop(recv);
77+
drop(cmd_send);
7778

7879
// Join the thread, it should finish shortly. We don't really care
7980
// whether it panicked, so it is safe to ignore the result
@@ -246,11 +247,21 @@ enum CheckEvent {
246247
End,
247248
}
248249

250+
#[derive(Debug)]
251+
pub struct CargoError(String);
252+
253+
impl fmt::Display for CargoError {
254+
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
255+
write!(f, "Cargo failed: {}", self.0)
256+
}
257+
}
258+
impl error::Error for CargoError {}
259+
249260
pub fn run_cargo(
250261
args: &[String],
251262
current_dir: Option<&Path>,
252263
on_message: &mut dyn FnMut(cargo_metadata::Message) -> bool,
253-
) -> Child {
264+
) -> Result<(), CargoError> {
254265
let mut command = Command::new("cargo");
255266
if let Some(current_dir) = current_dir {
256267
command.current_dir(current_dir);
@@ -273,6 +284,8 @@ pub fn run_cargo(
273284
// simply skip a line if it doesn't parse, which just ignores any
274285
// erroneus output.
275286
let stdout = BufReader::new(child.stdout.take().unwrap());
287+
let mut read_at_least_one_message = false;
288+
276289
for line in stdout.lines() {
277290
let line = match line {
278291
Ok(line) => line,
@@ -291,12 +304,31 @@ pub fn run_cargo(
291304
}
292305
};
293306

307+
read_at_least_one_message = true;
308+
294309
if !on_message(message) {
295310
break;
296311
}
297312
}
298313

299-
child
314+
// It is okay to ignore the result, as it only errors if the process is already dead
315+
let _ = child.kill();
316+
317+
let err_msg = match child.wait() {
318+
Ok(exit_code) if !exit_code.success() && !read_at_least_one_message => {
319+
// FIXME: Read the stderr to display the reason, see `read2()` reference in PR comment:
320+
// https://github.com/rust-analyzer/rust-analyzer/pull/3632#discussion_r395605298
321+
format!(
322+
"the command produced no valid metadata (exit code: {:?}): cargo {}",
323+
exit_code,
324+
args.join(" ")
325+
)
326+
}
327+
Err(err) => format!("io error: {:?}", err),
328+
Ok(_) => return Ok(()),
329+
};
330+
331+
Err(CargoError(err_msg))
300332
}
301333

302334
impl WatchThread {
@@ -325,7 +357,7 @@ impl WatchThread {
325357
// which will break out of the loop, and continue the shutdown
326358
let _ = message_send.send(CheckEvent::Begin);
327359

328-
let mut child = run_cargo(&args, Some(&workspace_root), &mut |message| {
360+
let res = run_cargo(&args, Some(&workspace_root), &mut |message| {
329361
// Skip certain kinds of messages to only spend time on what's useful
330362
match &message {
331363
Message::CompilerArtifact(artifact) if artifact.fresh => return true,
@@ -334,26 +366,19 @@ impl WatchThread {
334366
_ => {}
335367
}
336368

337-
match message_send.send(CheckEvent::Msg(message)) {
338-
Ok(()) => {}
339-
Err(_err) => {
340-
// The send channel was closed, so we want to shutdown
341-
return false;
342-
}
343-
};
344-
345-
true
369+
// if the send channel was closed, we want to shutdown
370+
message_send.send(CheckEvent::Msg(message)).is_ok()
346371
});
347372

373+
if let Err(err) = res {
374+
// FIXME: make the `message_send` to be `Sender<Result<CheckEvent, CargoError>>`
375+
// to display user-caused misconfiguration errors instead of just logging them here
376+
log::error!("Cargo watcher failed {:?}", err);
377+
}
378+
348379
// We can ignore any error here, as we are already in the progress
349380
// of shutting down.
350381
let _ = message_send.send(CheckEvent::End);
351-
352-
// It is okay to ignore the result, as it only errors if the process is already dead
353-
let _ = child.kill();
354-
355-
// Again, we don't care about the exit status so just ignore the result
356-
let _ = child.wait();
357382
}))
358383
} else {
359384
None

crates/ra_project_model/src/cargo_workspace.rs

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use std::{
66
};
77

88
use anyhow::{Context, Result};
9-
use cargo_metadata::{CargoOpt, Message, MetadataCommand, PackageId};
9+
use cargo_metadata::{BuildScript, CargoOpt, Message, MetadataCommand, PackageId};
1010
use ra_arena::{Arena, Idx};
1111
use ra_cargo_watch::run_cargo;
1212
use ra_db::Edition;
@@ -254,7 +254,7 @@ pub fn load_out_dirs(
254254
"check".to_string(),
255255
"--message-format=json".to_string(),
256256
"--manifest-path".to_string(),
257-
format!("{}", cargo_toml.display()),
257+
cargo_toml.display().to_string(),
258258
];
259259

260260
if cargo_features.all_features {
@@ -263,19 +263,15 @@ pub fn load_out_dirs(
263263
// FIXME: `NoDefaultFeatures` is mutual exclusive with `SomeFeatures`
264264
// https://github.com/oli-obk/cargo_metadata/issues/79
265265
args.push("--no-default-features".to_string());
266-
} else if !cargo_features.features.is_empty() {
267-
for feature in &cargo_features.features {
268-
args.push(feature.clone());
269-
}
266+
} else {
267+
args.extend(cargo_features.features.iter().cloned());
270268
}
271269

272-
let mut res = FxHashMap::default();
273-
let mut child = run_cargo(&args, cargo_toml.parent(), &mut |message| {
270+
let mut acc = FxHashMap::default();
271+
let res = run_cargo(&args, cargo_toml.parent(), &mut |message| {
274272
match message {
275-
Message::BuildScriptExecuted(message) => {
276-
let package_id = message.package_id;
277-
let out_dir = message.out_dir;
278-
res.insert(package_id, out_dir);
273+
Message::BuildScriptExecuted(BuildScript { package_id, out_dir, .. }) => {
274+
acc.insert(package_id, out_dir);
279275
}
280276

281277
Message::CompilerArtifact(_) => (),
@@ -285,6 +281,9 @@ pub fn load_out_dirs(
285281
true
286282
});
287283

288-
let _ = child.wait();
289-
res
284+
if let Err(err) = res {
285+
log::error!("Failed to load outdirs: {:?}", err);
286+
}
287+
288+
acc
290289
}

0 commit comments

Comments
 (0)