Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: rustfmt output to stdout #1042

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 80 additions & 45 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1626,18 +1626,15 @@ impl Bindings {

/// Write these bindings as source text to a file.
pub fn write_to_file<P: AsRef<Path>>(&self, path: P) -> io::Result<()> {
{
let file = try!(
OpenOptions::new()
.write(true)
.truncate(true)
.create(true)
.open(path.as_ref())
);
self.write(Box::new(file))?;
}

self.rustfmt_generated_file(path.as_ref())
let file = try!(
OpenOptions::new()
.write(true)
.truncate(true)
.create(true)
.open(path.as_ref())
);
self.write(Box::new(file))?;
Ok(())
}

/// Write these bindings as source text to the given `Write`able.
Expand All @@ -1650,31 +1647,50 @@ impl Bindings {
writer.write(line.as_bytes())?;
writer.write("\n".as_bytes())?;
}

if !self.options.raw_lines.is_empty() {
writer.write("\n".as_bytes())?;
}

writer.write(self.module.as_str().as_bytes())?;
let bindings = self.module.as_str().to_string();

match self.rustfmt_generated_string(bindings) {
Ok(rustfmt_bindings) => {
writer.write(rustfmt_bindings.as_str().as_bytes())?;
},
Err(err) => eprintln!("{:?}", err),
}
Ok(())
}

/// Checks if rustfmt_bindings is set and runs rustfmt on the file
fn rustfmt_generated_file(&self, file: &Path) -> io::Result<()> {
let _t = time::Timer::new("rustfmt_generated_file")
/// Checks if rustfmt_bindings is set and runs rustfmt on the string
fn rustfmt_generated_string(&self, source: String) -> io::Result<String> {
let _t = time::Timer::new("rustfmt_generated_string")
.with_output(self.options.time_phases);

if !self.options.rustfmt_bindings {
return Ok(());
return Ok(source);
}

let rustfmt = if let Ok(rustfmt) = which::which("rustfmt") {
rustfmt
} else {
warn!("Not running rustfmt because it does not exist in PATH");
return Ok(());
eprintln!("warning: could not find usable rustfmt to pretty print bindings");
return Ok(source);
};

let mut cmd = if let Ok(rustup) = which::which("rustup") {
let mut cmd = Command::new(rustup);
cmd.args(&["run", "nightly", "rustfmt", "--"]);
cmd
} else {
Command::new(rustfmt)
};

let mut cmd = Command::new(rustfmt);
cmd
.args(&["--write-mode=display"])
.stdin(Stdio::piped())
.stdout(Stdio::piped());

if let Some(path) = self.options
.rustfmt_configuration_file
Expand All @@ -1684,34 +1700,53 @@ impl Bindings {
cmd.args(&["--config-path", path]);
}

if let Ok(output) = cmd.arg(file).output() {
if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
match output.status.code() {
Some(2) => Err(io::Error::new(
io::ErrorKind::Other,
format!("Rustfmt parsing errors:\n{}", stderr),
)),
Some(3) => {
warn!(
"Rustfmt could not format some lines:\n{}",
stderr
);
Ok(())
if let Ok(mut child) = cmd.spawn() {

let mut child_stdin = child.stdin.take().unwrap();
let mut child_stdout = child.stdout.take().unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fitzgen Is there a better way than to unwrap here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, unwrap is fine because we know it is Some becuse we asked it to be Stdio::piped()


// Write to stdin in a new thread, so that we can read from stdout on this
// thread. This keeps the child from blocking on writing to its stdout which
// might block us from writing to its stdin.
let stdin_handle = ::std::thread::spawn(move || {
let _ = child_stdin.write_all(source.as_bytes());
Copy link
Contributor Author

@manaskarekar manaskarekar Oct 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fitzgen What would be a better way to handle this "let _"?

Perhaps we can do something like:

match child_stdin.write_all(source.as_bytes()) {
  Ok(_) => {},
  Err(e) => {
      eprintln!("Failed to write to rustfmt's stdin with error {}", e);
      return source;
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks fine by me.


source
});

let mut output = vec![];
io::copy(&mut child_stdout, &mut output)?;

let status = child.wait()?;

let source = stdin_handle.join().unwrap();
Copy link
Contributor Author

@manaskarekar manaskarekar Oct 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fitzgen How can (and should) we avoid the unwrap here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to, we can

.map_err(|_| io::Error::new(io::ErrorKind::Other, "panicked writing to `rustfmt` stdin".into())

and then use the ? operator.

But I don't see any way that thread should be able to panic, so unwrap is probably fine.


match String::from_utf8(output) {
Ok(bindings) => {
if !status.success() {
match status.code() {
Some(2) => Err(io::Error::new(
io::ErrorKind::Other,
format!("Rustfmt parsing errors."),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"...".into() is better than using format!

)),
Some(3) => {
warn!("Rustfmt could not format some lines.");
Ok(bindings)
}
_ => Err(io::Error::new(
io::ErrorKind::Other,
format!("Internal rustfmt error"),
)),
}
} else {
Ok(bindings)
}
_ => Err(io::Error::new(
io::ErrorKind::Other,
format!("Internal rustfmt error:\n{}", stderr),
)),
}
} else {
Ok(())
},
_ => Ok(source)
}
} else {
Err(io::Error::new(
io::ErrorKind::Other,
"Error executing rustfmt!",
))
eprintln!("Error executing rustfmt!");
Ok(source)
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ The latest `rustfmt` is required to run the `bindgen` test suite. Install
"nightly",
"rustfmt",
"--config-path",
concat!(env!("CARGO_MANIFEST_DIR"), "/tests/rustfmt.toml")
concat!(env!("CARGO_MANIFEST_DIR"), "/rustfmt.toml")
])
.stdin(process::Stdio::piped())
.stdout(process::Stdio::piped())
Expand Down