From 1772eb0f1a5c714c91f8ae45cc67cbae6b7ff348 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Tue, 20 Apr 2021 16:06:20 +0300 Subject: fix: no longer get stuck on windows reading both stdout & stderr is a common gotcha, you need to drain them concurrently to avoid deadlocks. Not sure why I didn't do the right thing from the start. Seems like I assumed the stderr is short? That's not the case when cargo spams `compiling xyz` messages --- crates/project_model/src/build_data.rs | 134 +++++++++++++++++++-------------- 1 file changed, 76 insertions(+), 58 deletions(-) (limited to 'crates/project_model') diff --git a/crates/project_model/src/build_data.rs b/crates/project_model/src/build_data.rs index ab5cc8c49..faca336de 100644 --- a/crates/project_model/src/build_data.rs +++ b/crates/project_model/src/build_data.rs @@ -1,7 +1,6 @@ //! Handles build script specific information use std::{ - io::BufReader, path::PathBuf, process::{Command, Stdio}, sync::Arc, @@ -13,7 +12,8 @@ use cargo_metadata::{BuildScript, Message}; use itertools::Itertools; use paths::{AbsPath, AbsPathBuf}; use rustc_hash::FxHashMap; -use stdx::{format_to, JodChild}; +use serde::Deserialize; +use stdx::format_to; use crate::{cfg_flag::CfgFlag, CargoConfig}; @@ -171,67 +171,86 @@ impl WorkspaceBuildData { cmd.stdout(Stdio::piped()).stderr(Stdio::piped()).stdin(Stdio::null()); - let mut child = cmd.spawn().map(JodChild)?; - let child_stdout = child.stdout.take().unwrap(); - let stdout = BufReader::new(child_stdout); - let mut res = WorkspaceBuildData::default(); - for message in cargo_metadata::Message::parse_stream(stdout).flatten() { - match message { - Message::BuildScriptExecuted(BuildScript { - package_id, - out_dir, - cfgs, - env, - .. - }) => { - let cfgs = { - let mut acc = Vec::new(); - for cfg in cfgs { - match cfg.parse::() { - Ok(it) => acc.push(it), - Err(err) => { - anyhow::bail!("invalid cfg from cargo-metadata: {}", err) - } - }; - } - acc - }; - let package_build_data = - res.per_package.entry(package_id.repr.clone()).or_default(); - // cargo_metadata crate returns default (empty) path for - // older cargos, which is not absolute, so work around that. - if !out_dir.as_str().is_empty() { - let out_dir = AbsPathBuf::assert(PathBuf::from(out_dir.into_os_string())); - package_build_data.out_dir = Some(out_dir); - package_build_data.cfgs = cfgs; - } - package_build_data.envs = env; + let mut callback_err = None; + let output = stdx::process::streaming_output( + cmd, + &mut |line| { + if callback_err.is_some() { + return; } - Message::CompilerArtifact(message) => { - progress(format!("metadata {}", message.target.name)); - - if message.target.kind.contains(&"proc-macro".to_string()) { - let package_id = message.package_id; - // Skip rmeta file - if let Some(filename) = message.filenames.iter().find(|name| is_dylib(name)) - { - let filename = AbsPathBuf::assert(PathBuf::from(&filename)); - let package_build_data = - res.per_package.entry(package_id.repr.clone()).or_default(); - package_build_data.proc_macro_dylib_path = Some(filename); + + // Copy-pasted from existing cargo_metadata. It seems like we + // should be using sered_stacker here? + let mut deserializer = serde_json::Deserializer::from_str(&line); + deserializer.disable_recursion_limit(); + let message = Message::deserialize(&mut deserializer) + .unwrap_or(Message::TextLine(line.to_string())); + + match message { + Message::BuildScriptExecuted(BuildScript { + package_id, + out_dir, + cfgs, + env, + .. + }) => { + let cfgs = { + let mut acc = Vec::new(); + for cfg in cfgs { + match cfg.parse::() { + Ok(it) => acc.push(it), + Err(err) => { + callback_err = Some(anyhow::format_err!( + "invalid cfg from cargo-metadata: {}", + err + )); + return; + } + }; + } + acc + }; + let package_build_data = + res.per_package.entry(package_id.repr.clone()).or_default(); + // cargo_metadata crate returns default (empty) path for + // older cargos, which is not absolute, so work around that. + if !out_dir.as_str().is_empty() { + let out_dir = + AbsPathBuf::assert(PathBuf::from(out_dir.into_os_string())); + package_build_data.out_dir = Some(out_dir); + package_build_data.cfgs = cfgs; } + + package_build_data.envs = env; } + Message::CompilerArtifact(message) => { + progress(format!("metadata {}", message.target.name)); + + if message.target.kind.contains(&"proc-macro".to_string()) { + let package_id = message.package_id; + // Skip rmeta file + if let Some(filename) = + message.filenames.iter().find(|name| is_dylib(name)) + { + let filename = AbsPathBuf::assert(PathBuf::from(&filename)); + let package_build_data = + res.per_package.entry(package_id.repr.clone()).or_default(); + package_build_data.proc_macro_dylib_path = Some(filename); + } + } + } + Message::CompilerMessage(message) => { + progress(message.target.name.clone()); + } + Message::BuildFinished(_) => {} + Message::TextLine(_) => {} + _ => {} } - Message::CompilerMessage(message) => { - progress(message.target.name.clone()); - } - Message::BuildFinished(_) => {} - Message::TextLine(_) => {} - _ => {} - } - } + }, + &mut |_| (), + )?; for package in packages { let package_build_data = res.per_package.entry(package.id.repr.clone()).or_default(); @@ -244,7 +263,6 @@ impl WorkspaceBuildData { } } - let output = child.into_inner().wait_with_output()?; if !output.status.success() { let mut stderr = String::from_utf8(output.stderr).unwrap_or_default(); if stderr.is_empty() { -- cgit v1.2.3