lib: rustfmt output to stdout #1042

manaskarekar · 2017-09-26T12:02:47Z

Taking a crack at issue #968.

I have yet to write tests, but would appreciate some comments on what I have so far.

Any help appreciated from r? @fitzgen or others.

Thank you.

highfive · 2017-09-26T12:02:51Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @fitzgen (or someone else) soon.

fitzgen

This is a good start!

We should add a smoke test to tests/tests.rs asserting that these result in the same output:

cargo run <some simple test header>
rustfmt(cargo run --no-rustfmt-bindings <some simple test header>)

Additionally, we should remove write_to_stdout and remove rustfmt_generated_file. Instead, we should call rustfmt_generated_string from write before writing the source to the given writer. This lets us share more code and avoid copies of things doing basically the same job.

Thanks @manaskarekar ! Let me know if you have any questions!

fitzgen · 2017-09-26T16:54:17Z

src/lib.rs

+            cmd.args(&["--config-path", path]);
+        }
+
+        let mut child = cmd.spawn().expect("Failed to spawn rustfmt.");


This should propagate the error rather than unwrapping it with expect.

fitzgen · 2017-09-26T17:00:38Z

src/lib.rs

+
+        let child_stderr = stderr_handle.join()
+          .expect("stderr reader thread should not have panicked")
+          .expect("should have read child rustfmt's stderr OK");


All these expects in this function should be propagating errors instead of unwrapping them.

fitzgen · 2017-09-26T17:09:51Z

src/lib.rs

+    }
+
+    /// Checks if rustfmt_bindings is set and runs rustfmt on the string
+    fn rustfmt_generated_string(&self, source: String) -> io::Result<Option<(String, String)>> {


I think we can just let the child's stderr inherit our stderr, and that will let us simplify this function and its signature a bit.

We should end up with:

fn rustfmt_generated_string(&self, source: String) -> io::Result<String> { ... }

Hey @fitzgen, if we inherit stderr, how can we match on the status code of rustfmt to perform the respective actions? Just let the stderr display rustfmt's errors?

We inherit stderr by simply not calling .stderr(..) on the command that is being built. It doesn't affect how the status code is returned; its orthogonal.

Does that make sense?

fitzgen · 2017-09-26T17:13:00Z

src/lib.rs

+        let _t = self.context.timer("rustfmt_generated_string");
+
+        if !self.context.options().rustfmt_bindings {
+            return Ok(None);


This should just return the source string unmodified, or else we won't print anything to stdout at all.

fitzgen · 2017-09-26T17:17:34Z

src/lib.rs

+            return Err(io::Error::new(
+                io::ErrorKind::Other,
+                "Rustfmt activated, but it could not be found in global path.",
+            ));


This should just return Ok(source) as well. We don't want to avoid emitting bindings just because there is no rustfmt (rustfmting is configured on by default).

We can eprintln!("warning: could not find usable rustfmt to pretty print bindings") to give users an idea why the bindings aren't pretty printed.

fitzgen · 2017-09-26T17:22:32Z

src/lib.rs

+            ));
+        };
+
+        let mut cmd = Command::new(rustfmt);


This is going to fail with rustfmt-nightly which is where rustfmt development is happening these days:

$ rustfmt --version rustfmt: error while loading shared libraries: libsyntax-ff7df0d2d58ad322.so: cannot open shared object file: No such file or directory $ rustup run nightly rustfmt -- --version 0.2.6-nightly ( )

We should do something like this:

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) }; // ... continue configuring `cmd` ...

fitzgen · 2017-09-26T17:44:07Z

src/lib.rs

+
+        // Ignore actual rustfmt status because it is often non-zero for trivial
+        // things.
+        let _ = child.wait().expect("should wait on rustfmt child OK");


This was fine for tests, but now that this is for a wider audience, lets do what rustfmt_generated_file is doing with the status code:

match output.status.code() { Some(0) => Ok(()), 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(()) } _ => Err(io::Error::new( io::ErrorKind::Other, format!("Internal rustfmt error:\n{}", stderr), )), }

You can see more information about rustfmt's exit codes with rustfmt --help:

$ rustup run nightly rustfmt -- --help <snip> Exit Codes: 0 = No errors 1 = Encountered operational errors e.g. an IO error 2 = Failed to reformat code because of parsing errors 3 = Code is valid, but it is impossible to format it properly 4 = Formatted code differs from existing code (write-mode diff only)

manaskarekar · 2017-09-26T17:56:49Z

Thank you for taking the time to give me thorough feedback @fitzgen.

I will make the relevant changes ~~this evening.~~ as soon as I get a chance.

As an aside, this continues to be a great exercise for a crash course through rust! Thank you.

fitzgen · 2017-10-02T17:05:40Z

Looks like there's a bunch of compilation errors:

error: method is never used: `rustfmt_generated_file`
    --> src/lib.rs:1606:5
     |
1606 | /     fn rustfmt_generated_file(&self, file: &Path) -> io::Result<()> {
1607 | |         let _t = self.context.timer("rustfmt_generated_file");
1608 | |
1609 | |         if !self.context.options().rustfmt_bindings {
...    |
1659 | |         }
1660 | |     }
     | |_____^
     |
note: lint level defined here
    --> src/lib.rs:8:9
     |
8    | #![deny(warnings)]
     |         ^^^^^^^^
     = note: #[deny(dead_code)] implied by #[deny(warnings)]
error: unused `std::result::Result` which must be used
    --> src/lib.rs:1728:9
     |
1728 |         stdin_handle.join();
     |         ^^^^^^^^^^^^^^^^^^^^
     |
note: lint level defined here
    --> src/lib.rs:8:9
     |
8    | #![deny(warnings)]
     |         ^^^^^^^^
     = note: #[deny(unused_must_use)] implied by #[deny(warnings)]
error: unused variable: `child_stderr`
    --> src/lib.rs:1730:13
     |
1730 |         let child_stderr = stderr_handle.join();
     |             ^^^^^^^^^^^^
     |
note: lint level defined here
    --> src/lib.rs:8:9
     |
8    | #![deny(warnings)]
     |         ^^^^^^^^
     = note: #[deny(unused_variables)] implied by #[deny(warnings)]

Let me know when you have all the tests passing and this is ready for me to look at again.

Also, let's squash this PR into a single commit to hide the WIP commits from the git history which improves bisecting: https://github.com/servo/servo/wiki/Beginner's-guide-to-rebasing-and-squashing

Thanks @manaskarekar !

manaskarekar · 2017-10-02T18:03:29Z

Sorry about that, Nick, I'll make the changes accordingly. I had a few questions that I'll ask on chat.

Thanks for your help in reviewing it!

bors-servo · 2017-10-12T21:22:47Z

☔ The latest upstream changes (presumably #1079) made this pull request unmergeable. Please resolve the merge conflicts.

manaskarekar · 2017-10-22T16:58:45Z

src/lib.rs

+            // 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());


@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; } }

That looks fine by me.

manaskarekar · 2017-10-22T16:59:06Z

src/lib.rs

+        if let Ok(mut child) = cmd.spawn() {
+
+            let mut child_stdin = child.stdin.take().unwrap();
+            let mut child_stdout = child.stdout.take().unwrap();


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

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

manaskarekar · 2017-10-22T17:01:52Z

src/lib.rs

+
+            let status = child.wait()?;
+
+            let source = stdin_handle.join().unwrap();


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

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.

fitzgen · 2017-10-23T18:04:12Z

src/lib.rs

+        if let Ok(mut child) = cmd.spawn() {
+
+            let mut child_stdin = child.stdin.take().unwrap();
+            let mut child_stdout = child.stdout.take().unwrap();


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

fitzgen · 2017-10-23T18:08:12Z

src/lib.rs

+            // 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());


That looks fine by me.

fitzgen · 2017-10-23T18:11:38Z

src/lib.rs

+
+            let status = child.wait()?;
+
+            let source = stdin_handle.join().unwrap();


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.

fitzgen · 2017-10-23T18:13:50Z

src/lib.rs

+                        match status.code() {
+                            Some(2) => Err(io::Error::new(
+                                io::ErrorKind::Other,
+                                format!("Rustfmt parsing errors."),


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

fitzgen · 2017-10-23T18:28:25Z

I believe that the test failures on CI are due to not using --config-path tests/rustfmt.toml with rustfmt on the tests anymore -- can you try making the tests configure the bindgen invocation to use the appropriate rustfmt config?

Are the tests passing for you locally?

manaskarekar · 2017-10-25T12:56:38Z

EDIT: The tests pass locally only after the first run, due to the generated expectations/tests being updated in the first run.

manaskarekar · 2017-10-26T12:20:44Z

I changed the rustfmt to point the correct one and some tests seem to be failing due to formatting.

I'm not sure if these changes are due to rustfmt updates.

The generated expectations/tests are showing some weird formatting with extra spaces etc. I wonder if the rustfmt.toml needs a tweak.

fitzgen · 2017-10-26T16:38:08Z

Are you sure you have the most up-to-date rustfmt?

$ rustup update nightly
$ cargo +nightly install -f rustfmt-nightly

Because after doing that locally, this doesn't produce any diffs for me:

$ rustup run nightly rustfmt --config-path tests/rustfmt.toml tests/expectations/tests/*.rs

If you can push any local changes you have, then I can try and pull this branch down and poke at it.

Simplify the rustfmt and write mechanism. Use rustfmt generated string to allow writing to stdout or to rustfmt a file.

Change path to rustfmt.toml. Some tests fail due to formatting differences in expectations and generated bindings.

fitzgen · 2017-10-27T18:10:44Z

I think the issue was

Changing the rustfmt.toml that was being used by the test runner, and/or
Not passing --no-rustfmt-bindings in the test runner, since it does its own formatting.

All the tests are passing now, and bidnings are properly rustfmted to stdout as well.

Moving to #1109 instead of this PR. Thanks for the persistence @manaskarekar !

lib: rustfmt output to stdout *(Recreated + slightly touched up version of #1042 + a version bump)* Simplify the rustfmt and write mechanism. Use rustfmt generated string to allow writing to stdout or to rustfmt a file.

manaskarekar · 2017-10-27T18:12:08Z

I was just looking into this. Thanks!

highfive assigned fitzgen Sep 26, 2017

highfive added the S-awaiting-review label Sep 26, 2017

fitzgen reviewed Sep 26, 2017

View reviewed changes

This was referenced Sep 27, 2017

rustfmt errors when std:: enabled ctaggart/octh#27

Closed

can't compile bindings after rustfmt #1045

Closed

fitzgen mentioned this pull request Oct 6, 2017

Both _WIN32 and _WIN64 are defined when targeting 32-bit windows from 64-bit windows #1062

Closed

highfive added the S-needs-rebase label Oct 12, 2017

manaskarekar force-pushed the master branch 4 times, most recently from ad4fe98 to 066ea49 Compare October 22, 2017 16:40

manaskarekar commented Oct 22, 2017

View reviewed changes

fitzgen reviewed Oct 23, 2017

View reviewed changes

manaskarekar added 2 commits October 26, 2017 21:49

lib: rustfmt output to stdout

5660db7

Simplify the rustfmt and write mechanism. Use rustfmt generated string to allow writing to stdout or to rustfmt a file.

CI Tests Failing:

0095e99

Change path to rustfmt.toml. Some tests fail due to formatting differences in expectations and generated bindings.

manaskarekar force-pushed the master branch from 81028e7 to 0095e99 Compare October 27, 2017 01:49

fitzgen mentioned this pull request Oct 27, 2017

lib: rustfmt output to stdout #1109

Merged

fitzgen closed this Oct 27, 2017

fitzgen mentioned this pull request Oct 31, 2017

--rustfmt_bindings doesn't work with printing bindings to stdout #968

Closed

Kobata mentioned this pull request Dec 9, 2017

Rustfmt is not run if rustup exists, but nightly toonchain/rustfmt-nightly is not installed #1184

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: rustfmt output to stdout #1042

lib: rustfmt output to stdout #1042

manaskarekar commented Sep 26, 2017

highfive commented Sep 26, 2017

fitzgen left a comment

fitzgen Sep 26, 2017

fitzgen Sep 26, 2017

fitzgen Sep 26, 2017

manaskarekar Oct 13, 2017

fitzgen Oct 13, 2017

fitzgen Sep 26, 2017

fitzgen Sep 26, 2017

fitzgen Sep 26, 2017

fitzgen Sep 26, 2017

manaskarekar commented Sep 26, 2017 •

edited

Loading

fitzgen commented Oct 2, 2017

manaskarekar commented Oct 2, 2017 •

edited

Loading

bors-servo commented Oct 12, 2017

manaskarekar Oct 22, 2017 •

edited

Loading

fitzgen Oct 23, 2017

manaskarekar Oct 22, 2017

fitzgen Oct 23, 2017

manaskarekar Oct 22, 2017 •

edited

Loading

fitzgen Oct 23, 2017

fitzgen Oct 23, 2017

fitzgen Oct 23, 2017

fitzgen Oct 23, 2017

fitzgen Oct 23, 2017

fitzgen commented Oct 23, 2017

manaskarekar commented Oct 25, 2017 •

edited

Loading

manaskarekar commented Oct 26, 2017 •

edited

Loading

fitzgen commented Oct 26, 2017

fitzgen commented Oct 27, 2017

manaskarekar commented Oct 27, 2017


		let status = child.wait()?;

		let source = stdin_handle.join().unwrap();

lib: rustfmt output to stdout #1042

lib: rustfmt output to stdout #1042

Conversation

manaskarekar commented Sep 26, 2017

highfive commented Sep 26, 2017

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manaskarekar commented Sep 26, 2017 • edited Loading

fitzgen commented Oct 2, 2017

manaskarekar commented Oct 2, 2017 • edited Loading

bors-servo commented Oct 12, 2017

manaskarekar Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manaskarekar Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen commented Oct 23, 2017

manaskarekar commented Oct 25, 2017 • edited Loading

manaskarekar commented Oct 26, 2017 • edited Loading

fitzgen commented Oct 26, 2017

fitzgen commented Oct 27, 2017

manaskarekar commented Oct 27, 2017

manaskarekar commented Sep 26, 2017 •

edited

Loading

manaskarekar commented Oct 2, 2017 •

edited

Loading

manaskarekar Oct 22, 2017 •

edited

Loading

manaskarekar Oct 22, 2017 •

edited

Loading

manaskarekar commented Oct 25, 2017 •

edited

Loading

manaskarekar commented Oct 26, 2017 •

edited

Loading