Adds support for running rustfmt on generated bindings #905

bkchr · 2017-08-11T16:03:49Z

This patch enables bindgen to run rustfmt on generated bindings. Rustfmt is used
from the global PATH. Two new command-line arguments are added:

--format-bindings: Enables running rustfmt
--format-configuration-file: The configuration file for rustfmt (not required).

Fixes: #900

fitzgen

This looks great! Just a couple things before we merge:

The builder has a method command_line_flags that (re)constructs equivalent command line flags to the builder's configuration. This is really useful for debugging with C-Reduce when things go wrong. We need to add support for these new flags to that method. (Also, if we forgot about this for --impl Debug then we should do a new PR for that too).
A few nitpicks below.

Thanks again! :)

fitzgen · 2017-08-11T17:41:11Z

src/lib.rs

+    /// Set the absolute path to the rustfmt configuration file, if None, the standard rustfmt
+    /// options are used.
+    pub fn format_configuration_file(mut self, path: Option<PathBuf>) -> Self {
+        self.options.format_configuration_file = path;


This should probably imply format_bindings is true. That is, calling this method should also set format_bindings to true.

^ This comment does not appear to be addressed.

fitzgen · 2017-08-11T17:45:12Z

src/lib.rs

+            cmd.args(&["--config-path", path]);
+        }
+
+        if let Err(e) = cmd.arg(file).status() {


Note that status() only returns the child processes exit status, it doesn't check that the exit status is success. Additionally, I think it makes sense to propagate any errors, since (unlike if rustfmt is just missing) we don't know if the bindings are still in an OK state if this fails: maybe only half of the formatted bindings got written to the file, and the other half didn't?

let status = cmd.arg(file).status()?; if status.success() { Ok(()) } else { Err(io::Error::new(io::ErrorKind::Other, "rustfmt exited with a failure status: {}", status)) }

As far as I know, rustfmt does not write only the half. The problem with the status code is that if rustfmt can not format (because line too long or whatever), it ends with status != 0. I also can not return an error here, because write_to_file returns a io::Result. Chain_error would be nice here.

he problem with the status code is that if rustfmt can not format (because line too long or whatever), it ends with status != 0.

Yeah. Ideally we would have some way of distinguishing between "real" failures and "warnings". I guess we should just add a comment to this effect here and move on with our lives.

cc @nrc: you may find this ^ useful for informing rustfmt development maybe

I also can not return an error here, because write_to_file returns a io::Result.

io::Error allows for custom errors (like in the above snippet) and the std::process APIs also return io::Results, so I don't understand the problem. I must not be following something. But I guess it doesn't matter since we aren't changing these things.

Ohh... I totally missed that Command returns a io::Result...

fitzgen · 2017-08-11T17:53:00Z

src/lib.rs

@@ -1086,6 +1110,13 @@ pub struct BindgenOptions {

    /// Whether to prepend the enum name to bitfield or constant variants.
    pub prepend_enum_name: bool,
+
+    /// Whether rustfmt should format the generated bindings.
+    pub format_bindings: bool,


Super nitpicky (sorry!), but I'd prefer rustfmt_bindings and --rustfmt-bindings (and similar for the config files).

bkchr · 2017-08-11T19:36:36Z

https://github.com/bkchr/rust-bindgen/blob/35aaee1d21a19cefd37c388f6889c4961dafcb75/src/lib.rs#L457 isn't that your first point? If yes, I also done that for derive-debug :)

fitzgen · 2017-08-11T20:05:56Z

https://github.com/bkchr/rust-bindgen/blob/35aaee1d21a19cefd37c388f6889c4961dafcb75/src/lib.rs#L457 isn't that your first point? If yes, I also done that for derive-debug :)

Ah yes. Somehow I missed that :-P

bkchr · 2017-08-11T21:01:22Z

Addressed all issues :)

fitzgen

👍 thanks!

fitzgen · 2017-08-11T21:24:52Z

@bors-servo r+

bors-servo · 2017-08-11T21:24:52Z

📌 Commit 7fe3f0c has been approved by fitzgen

bors-servo · 2017-08-11T21:24:55Z

⌛ Testing commit 7fe3f0c with merge 99d188f...

Adds support for running rustfmt on generated bindings This patch enables bindgen to run rustfmt on generated bindings. Rustfmt is used from the global PATH. Two new command-line arguments are added: 1. --format-bindings: Enables running rustfmt 2. --format-configuration-file: The configuration file for rustfmt (not required). Fixes: #900

bors-servo · 2017-08-12T01:16:02Z

💔 Test failed - status-travis

bkchr · 2017-08-12T09:41:52Z

I had rustfmt-bindgens enabled by default and then the test failed.. I changed it to disabled by default, is also probably better.

bkchr · 2017-08-12T11:58:33Z

Now all tests are green :)

bors-servo · 2017-08-14T10:05:40Z

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

bkchr · 2017-08-14T11:07:27Z

Rebased.

fitzgen · 2017-08-14T16:26:41Z

I changed it to disabled by default, is also probably better.

Ah, yes! It should be disabled by default, sorry for not catching that in review!

fitzgen · 2017-08-14T16:29:45Z

src/lib.rs

@@ -1183,6 +1214,8 @@ impl Default for BindgenOptions {
            objc_extern_crate: false,
            enable_mangling: true,
            prepend_enum_name: true,
+            format_bindings: true,


And this is defaulting to true, but you just said you changed it to default to false (which it should) -- did you forget to push some local changes?

fitzgen · 2017-08-14T16:32:09Z

@bkchr looks like a couple pieces of feedback got missed, let me know when you push fixes, thanks!

This patch enables bindgen to run rustfmt on generated bindings. Rustfmt is used from the global PATH. Two new command-line arguments are added: 1. --format-bindings: Enables running rustfmt 2. --format-configuration-file: The configuration file for rustfmt (not required).

…08-10)

The --rustfmt-configuration-file command-line argument automatically activates --rustfmt-bindings.

bkchr · 2017-08-14T16:36:21Z

@fitzgen ohh sorry... I worked across different pc's... And I did not pull before rebasing.. Now it should be as intended^^

fitzgen · 2017-08-14T16:49:22Z

@bkchr no problem, thanks for the PR :)

@bors-servo r+

bors-servo · 2017-08-14T16:49:22Z

📌 Commit 27dd628 has been approved by fitzgen

bors-servo · 2017-08-14T16:49:25Z

⌛ Testing commit 27dd628 with merge 5ca0569...

Adds support for running rustfmt on generated bindings This patch enables bindgen to run rustfmt on generated bindings. Rustfmt is used from the global PATH. Two new command-line arguments are added: 1. --format-bindings: Enables running rustfmt 2. --format-configuration-file: The configuration file for rustfmt (not required). Fixes: #900

bors-servo · 2017-08-14T17:03:40Z

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing 5ca0569 to master...

fitzgen reviewed Aug 11, 2017

View reviewed changes

fitzgen mentioned this pull request Aug 11, 2017

Tools news rust-dev-tools/dev-tools-team#22

Closed

fitzgen approved these changes Aug 11, 2017

View reviewed changes

bkchr force-pushed the rustfmt branch from 07345cb to feb3ded Compare August 14, 2017 11:07

fitzgen reviewed Aug 14, 2017

View reviewed changes

bkchr added 5 commits August 14, 2017 18:34

Fixes compilation warnings with rustc 1.21.0-nightly (13d94d5fa 2017-…

31f6ced

…08-10)

Renames format-* command-line arguments to rustfmt-*

35627a8

The --rustfmt-configuration-file command-line argument automatically activates --rustfmt-bindings.

Improves error handling for rustfmt

d134eb1

Disables --rustfmt-bindgens by default

27dd628

bkchr force-pushed the rustfmt branch from feb3ded to 27dd628 Compare August 14, 2017 16:35

bors-servo merged commit 27dd628 into rust-lang:master Aug 14, 2017

bkchr deleted the rustfmt branch August 14, 2017 17:48

rukai mentioned this pull request Apr 17, 2024

Remove which dependency #2809

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for running rustfmt on generated bindings #905

Adds support for running rustfmt on generated bindings #905

bkchr commented Aug 11, 2017

fitzgen left a comment

fitzgen Aug 11, 2017 •

edited

fitzgen Aug 14, 2017

fitzgen Aug 11, 2017

bkchr Aug 11, 2017

fitzgen Aug 11, 2017

bkchr Aug 11, 2017

fitzgen Aug 11, 2017

bkchr commented Aug 11, 2017

fitzgen commented Aug 11, 2017

bkchr commented Aug 11, 2017

fitzgen left a comment

fitzgen commented Aug 11, 2017

bors-servo commented Aug 11, 2017

bors-servo commented Aug 11, 2017

bors-servo commented Aug 12, 2017

bkchr commented Aug 12, 2017

bkchr commented Aug 12, 2017

bors-servo commented Aug 14, 2017

bkchr commented Aug 14, 2017

fitzgen commented Aug 14, 2017

fitzgen Aug 14, 2017

fitzgen commented Aug 14, 2017

bkchr commented Aug 14, 2017 •

edited

fitzgen commented Aug 14, 2017

bors-servo commented Aug 14, 2017

bors-servo commented Aug 14, 2017

bors-servo commented Aug 14, 2017

Adds support for running rustfmt on generated bindings #905

Adds support for running rustfmt on generated bindings #905

Conversation

bkchr commented Aug 11, 2017

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen Aug 11, 2017 • edited

Choose a reason for hiding this comment

fitzgen Aug 14, 2017

Choose a reason for hiding this comment

fitzgen Aug 11, 2017

Choose a reason for hiding this comment

bkchr Aug 11, 2017

Choose a reason for hiding this comment

fitzgen Aug 11, 2017

Choose a reason for hiding this comment

bkchr Aug 11, 2017

Choose a reason for hiding this comment

fitzgen Aug 11, 2017

Choose a reason for hiding this comment

bkchr commented Aug 11, 2017

fitzgen commented Aug 11, 2017

bkchr commented Aug 11, 2017

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen commented Aug 11, 2017

bors-servo commented Aug 11, 2017

bors-servo commented Aug 11, 2017

bors-servo commented Aug 12, 2017

bkchr commented Aug 12, 2017

bkchr commented Aug 12, 2017

bors-servo commented Aug 14, 2017

bkchr commented Aug 14, 2017

fitzgen commented Aug 14, 2017

fitzgen Aug 14, 2017

Choose a reason for hiding this comment

fitzgen commented Aug 14, 2017

bkchr commented Aug 14, 2017 • edited

fitzgen commented Aug 14, 2017

bors-servo commented Aug 14, 2017

bors-servo commented Aug 14, 2017

bors-servo commented Aug 14, 2017

fitzgen Aug 11, 2017 •

edited

bkchr commented Aug 14, 2017 •

edited