Skip to content

Commit d906b7a

Browse files
matkladmatklad
committed
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
1 parent 7570212 commit d906b7a

File tree

5 files changed

+330
-65
lines changed

5 files changed

+330
-65
lines changed

‎Cargo.lock‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎crates/project_model/src/build_data.rs‎

Lines changed: 76 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
//! Handles build script specific information
22
33
use std::{
4-
io::BufReader,
54
path::PathBuf,
65
process::{Command, Stdio},
76
sync::Arc,
@@ -13,7 +12,8 @@ use cargo_metadata::{BuildScript, Message};
1312
use itertools::Itertools;
1413
use paths::{AbsPath, AbsPathBuf};
1514
use rustc_hash::FxHashMap;
16-
use stdx::{format_to, JodChild};
15+
use serde::Deserialize;
16+
use stdx::format_to;
1717

1818
use crate::{cfg_flag::CfgFlag, CargoConfig};
1919

@@ -171,67 +171,86 @@ impl WorkspaceBuildData {
171171

172172
cmd.stdout(Stdio::piped()).stderr(Stdio::piped()).stdin(Stdio::null());
173173

174-
let mut child = cmd.spawn().map(JodChild)?;
175-
let child_stdout = child.stdout.take().unwrap();
176-
let stdout = BufReader::new(child_stdout);
177-
178174
let mut res = WorkspaceBuildData::default();
179-
for message in cargo_metadata::Message::parse_stream(stdout).flatten() {
180-
match message {
181-
Message::BuildScriptExecuted(BuildScript {
182-
package_id,
183-
out_dir,
184-
cfgs,
185-
env,
186-
..
187-
}) => {
188-
let cfgs = {
189-
let mut acc = Vec::new();
190-
for cfg in cfgs {
191-
match cfg.parse::<CfgFlag>() {
192-
Ok(it) => acc.push(it),
193-
Err(err) => {
194-
anyhow::bail!("invalid cfg from cargo-metadata: {}", err)
195-
}
196-
};
197-
}
198-
acc
199-
};
200-
let package_build_data =
201-
res.per_package.entry(package_id.repr.clone()).or_default();
202-
// cargo_metadata crate returns default (empty) path for
203-
// older cargos, which is not absolute, so work around that.
204-
if !out_dir.as_str().is_empty() {
205-
let out_dir = AbsPathBuf::assert(PathBuf::from(out_dir.into_os_string()));
206-
package_build_data.out_dir = Some(out_dir);
207-
package_build_data.cfgs = cfgs;
208-
}
209175

210-
package_build_data.envs = env;
176+
let mut callback_err = None;
177+
let output = stdx::process::streaming_output(
178+
cmd,
179+
&mut |line| {
180+
if callback_err.is_some() {
181+
return;
211182
}
212-
Message::CompilerArtifact(message) => {
213-
progress(format!("metadata {}", message.target.name));
214-
215-
if message.target.kind.contains(&"proc-macro".to_string()) {
216-
let package_id = message.package_id;
217-
// Skip rmeta file
218-
if let Some(filename) = message.filenames.iter().find(|name| is_dylib(name))
219-
{
220-
let filename = AbsPathBuf::assert(PathBuf::from(&filename));
221-
let package_build_data =
222-
res.per_package.entry(package_id.repr.clone()).or_default();
223-
package_build_data.proc_macro_dylib_path = Some(filename);
183+
184+
// Copy-pasted from existing cargo_metadata. It seems like we
185+
// should be using sered_stacker here?
186+
let mut deserializer = serde_json::Deserializer::from_str(&line);
187+
deserializer.disable_recursion_limit();
188+
let message = Message::deserialize(&mut deserializer)
189+
.unwrap_or(Message::TextLine(line.to_string()));
190+
191+
match message {
192+
Message::BuildScriptExecuted(BuildScript {
193+
package_id,
194+
out_dir,
195+
cfgs,
196+
env,
197+
..
198+
}) => {
199+
let cfgs = {
200+
let mut acc = Vec::new();
201+
for cfg in cfgs {
202+
match cfg.parse::<CfgFlag>() {
203+
Ok(it) => acc.push(it),
204+
Err(err) => {
205+
callback_err = Some(anyhow::format_err!(
206+
"invalid cfg from cargo-metadata: {}",
207+
err
208+
));
209+
return;
210+
}
211+
};
212+
}
213+
acc
214+
};
215+
let package_build_data =
216+
res.per_package.entry(package_id.repr.clone()).or_default();
217+
// cargo_metadata crate returns default (empty) path for
218+
// older cargos, which is not absolute, so work around that.
219+
if !out_dir.as_str().is_empty() {
220+
let out_dir =
221+
AbsPathBuf::assert(PathBuf::from(out_dir.into_os_string()));
222+
package_build_data.out_dir = Some(out_dir);
223+
package_build_data.cfgs = cfgs;
224224
}
225+
226+
package_build_data.envs = env;
225227
}
228+
Message::CompilerArtifact(message) => {
229+
progress(format!("metadata {}", message.target.name));
230+
231+
if message.target.kind.contains(&"proc-macro".to_string()) {
232+
let package_id = message.package_id;
233+
// Skip rmeta file
234+
if let Some(filename) =
235+
message.filenames.iter().find(|name| is_dylib(name))
236+
{
237+
let filename = AbsPathBuf::assert(PathBuf::from(&filename));
238+
let package_build_data =
239+
res.per_package.entry(package_id.repr.clone()).or_default();
240+
package_build_data.proc_macro_dylib_path = Some(filename);
241+
}
242+
}
243+
}
244+
Message::CompilerMessage(message) => {
245+
progress(message.target.name.clone());
246+
}
247+
Message::BuildFinished(_) => {}
248+
Message::TextLine(_) => {}
249+
_ => {}
226250
}
227-
Message::CompilerMessage(message) => {
228-
progress(message.target.name.clone());
229-
}
230-
Message::BuildFinished(_) => {}
231-
Message::TextLine(_) => {}
232-
_ => {}
233-
}
234-
}
251+
},
252+
&mut |_| (),
253+
)?;
235254

236255
for package in packages {
237256
let package_build_data = res.per_package.entry(package.id.repr.clone()).or_default();
@@ -244,7 +263,6 @@ impl WorkspaceBuildData {
244263
}
245264
}
246265

247-
let output = child.into_inner().wait_with_output()?;
248266
if !output.status.success() {
249267
let mut stderr = String::from_utf8(output.stderr).unwrap_or_default();
250268
if stderr.is_empty() {

‎crates/stdx/Cargo.toml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,15 @@ edition = "2018"
1010
doctest = false
1111

1212
[dependencies]
13+
libc = "0.2.93"
1314
backtrace = { version = "0.3.44", optional = true }
1415
always-assert = { version = "0.1.2", features = ["log"] }
1516
# Think twice before adding anything here
1617

18+
[target.'cfg(windows)'.dependencies]
19+
miow = "0.3.6"
20+
winapi = "0.3.9"
21+
1722
[features]
1823
# Uncomment to enable for the whole crate graph
1924
# default = [ "backtrace" ]

‎crates/stdx/src/lib.rs‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
//! Missing batteries for standard libraries.
2-
use std::{cmp::Ordering, ops, process, time::Instant};
2+
use std::{cmp::Ordering, ops, time::Instant};
33

44
mod macros;
5+
pub mod process;
56
pub mod panic_context;
67

78
pub use always_assert::{always, never};
@@ -179,17 +180,17 @@ where
179180
}
180181

181182
#[repr(transparent)]
182-
pub struct JodChild(pub process::Child);
183+
pub struct JodChild(pub std::process::Child);
183184

184185
impl ops::Deref for JodChild {
185-
type Target = process::Child;
186-
fn deref(&self) -> &process::Child {
186+
type Target = std::process::Child;
187+
fn deref(&self) -> &std::process::Child {
187188
&self.0
188189
}
189190
}
190191

191192
impl ops::DerefMut for JodChild {
192-
fn deref_mut(&mut self) -> &mut process::Child {
193+
fn deref_mut(&mut self) -> &mut std::process::Child {
193194
&mut self.0
194195
}
195196
}
@@ -202,9 +203,9 @@ impl Drop for JodChild {
202203
}
203204

204205
impl JodChild {
205-
pub fn into_inner(self) -> process::Child {
206+
pub fn into_inner(self) -> std::process::Child {
206207
// SAFETY: repr transparent
207-
unsafe { std::mem::transmute::<JodChild, process::Child>(self) }
208+
unsafe { std::mem::transmute::<JodChild, std::process::Child>(self) }
208209
}
209210
}
210211

0 commit comments

Comments
 (0)