Add a feature to make which dependency optional #1615

lopopolo · 2019-09-05T22:48:31Z

This PR adds a new feature, enabled by default, called which-rustfmt. When enabled, this feature enables using the which crate to search for the rustfmt binary. which is now an optional dependency. Disabling this feature trims some dependencies in the case that bindings are generated at build time and never checked in.

This PR changes the API of rustfmt_path to return Result<Option<Cow<PathBuf>>>.

Ok(None) is returned in the case where which is disabled and no rustfmt command is
supplied explicitly either via configuration or env variable.

Downstream code checks for the presence of None to directly return the source without
emitting an error.

Fixes GH-1614.

I named the feature which-rustfmt instead of which because the feature is tied to the functionality, not the crate. If this functionality for some reason requires additional dependencies in the future, they can be added to this feature and excluded in a backwards compatible way. This is also the suggested way to name features by the cargo documentation.

@emilio I have rustfmt enabled by default on save in my editor and there was quite a lot of diff to ignore to make this PR minimal. Can we either rustfmt bindgen and add a step to CI to enforce code is formatted or add a rustfmt.toml so the formatting is preserved?

This feature controls whether bindgen will use the which crate to detect the rustfmt binary. This makes which an optional dependency. which-rustfmt is a default feature which makes this change backward compatible.

This commit changes the API of rustfmt_path to return Result<Option<Cow<PathBuf>>>. Ok(None) is returned in the case where which is disabled and no rustfmt command is supplied explicitly either via configuration or env variable. Downstream code checks for the presence of None to directly return the source without emitting an error.

emilio

Thanks, this looks pretty good :)

I think it could be simpler, and it'd be great to tweak .travis.yml to also at least build with this configuration.

Thank you!

emilio · 2019-09-07T12:47:59Z

Cargo.toml

@@ -54,7 +54,7 @@ lazy_static = "1"
 peeking_take_while = "0.1.2"
 quote = { version = "1", default-features = false }
 regex = "1.0"
-which = ">=1.0, <3.0"
+which = { version = ">=1.0, <3.0", optional = true }


What about just leaving this change, then the feature will be automatically called which (you get a feature for each optional dependency).

emilio · 2019-09-07T12:48:13Z

Cargo.toml

@@ -70,9 +70,11 @@ optional = true
 version = "0.4"

 [features]
-default = ["logging", "clap"]
+default = ["logging", "clap", "which-rustfmt"]


So then you replace which-rustfmt by which here...

emilio · 2019-09-07T12:48:20Z

Cargo.toml

 logging = ["env_logger", "log"]
 static = []
+# Dynamically discover a `rustfmt` binary using the `which` crate
+which-rustfmt = ["which"]


And remove this line.

emilio · 2019-09-07T12:48:37Z

src/lib.rs

@@ -32,6 +32,7 @@ extern crate quote;
 extern crate proc_macro2;
 extern crate regex;
 extern crate shlex;
+#[cfg(feature = "which-rustfmt")]


And s/which-rustfmt/which here.

emilio · 2019-09-07T12:49:31Z

src/lib.rs

            Err(e) => Err(io::Error::new(io::ErrorKind::Other, format!("{}", e))),
        }
+        #[cfg(not(feature = "which-rustfmt"))]


I'd just return an error here, like Err(io::Error::new(io::ErrorKind::Other, "which wasn't enabled, and no rustfmt binary specified"))), then the patch becomes much simpler, wdyt?

The error is not fatal so you'd still generate bindings.

lopopolo · 2019-09-15T23:41:35Z

Hi @emilio I'm a bit confused. This PR is approved but you've left feedback. Who is responsible for pushing this forward?

Your feedback on the name of the feature seems to ignore my justification for my decision from the PR description and the recommendation from the cargo docs. I'm happy to make this change but the Cargo.toml also has a logging feature which follows this pattern. Which is the right way in bindgen?

I named the feature which-rustfmt instead of which because the feature is tied to the functionality, not the crate. If this functionality for some reason requires additional dependencies in the future, they can be added to this feature and excluded in a backwards compatible way. This is also the suggested way to name features by the cargo documentation.

I found the codebase more difficult to contribute to than it should be because my reasonable editor configuration makes massive diffs when formatting on save. I've filed GH-1618 to address this.

emilio · 2019-09-16T00:14:27Z

Hi @emilio I'm a bit confused. This PR is approved but you've left feedback. Who is responsible for pushing this forward?

I expected you to address the comments before I merge it, though if you don't have the cycles I'm happy to fix it.

I found the codebase more difficult to contribute to than it should be because my reasonable editor configuration makes massive diffs when formatting on save. I've filed GH-1618 to address this.

That's fair, sorry about it. I'd be happy to do that, but probably worth waiting until this is merged so you don't have to rebase conflicts.

emilio · 2019-09-16T00:15:47Z

I named the feature which-rustfmt instead of which because the feature is tied to the functionality, not the crate. If this functionality for some reason requires additional dependencies in the future, they can be added to this feature and excluded in a backwards compatible way. This is also the suggested way to name features by the cargo documentation.

Ah, sorry, had missed this... Let's add build this on .travis.yml.

…sabled

lopopolo · 2019-09-16T01:37:40Z

@emilio (pending Travis passing) I think this is ready to be looked at again

emilio

Looks great, thanks!

lopopolo · 2019-09-17T06:47:06Z

Thanks for merging @emilio 😄

lopopolo added 2 commits September 5, 2019 15:28

Add which-rustfmt feature

f64287d

This feature controls whether bindgen will use the which crate to detect the rustfmt binary. This makes which an optional dependency. which-rustfmt is a default feature which makes this change backward compatible.

highfive added the S-awaiting-review label Sep 5, 2019

emilio approved these changes Sep 7, 2019

View reviewed changes

lopopolo mentioned this pull request Sep 15, 2019

rustfmt the codebase #1618

Closed

lopopolo added 3 commits September 15, 2019 18:23

Return Error if no rustfmt path given and which-rustfmt feature is di…

af4eebd

…sabled

Test bindgen with no default features

258940a

lint ci/script.sh

ebd8d4d

lopopolo added 2 commits September 15, 2019 19:19

Set RUSTFMT for nofeatures tests

e627649

Add rustfmt to nightly toolchain

c22fcf3

emilio approved these changes Sep 17, 2019

View reviewed changes

emilio merged commit 3062841 into rust-lang:master Sep 17, 2019

prokopyl mentioned this pull request Sep 22, 2019

Upgrade 'which' to remove indirect dependency on 'failure'. #1625

Closed

heidezomp mentioned this pull request Sep 24, 2019

Add .with_rustfmt("rustfmt") if the which-rustfmt feature is disabled? #1628

Closed

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

Add a feature to make which dependency optional #1615

Add a feature to make which dependency optional #1615

lopopolo commented Sep 5, 2019

emilio left a comment

emilio Sep 7, 2019

emilio Sep 7, 2019

emilio Sep 7, 2019

emilio Sep 7, 2019

emilio Sep 7, 2019

lopopolo commented Sep 15, 2019

emilio commented Sep 16, 2019

emilio commented Sep 16, 2019

lopopolo commented Sep 16, 2019

emilio left a comment

lopopolo commented Sep 17, 2019

Add a feature to make which dependency optional #1615

Add a feature to make which dependency optional #1615

Conversation

lopopolo commented Sep 5, 2019

emilio left a comment

Choose a reason for hiding this comment

emilio Sep 7, 2019

Choose a reason for hiding this comment

emilio Sep 7, 2019

Choose a reason for hiding this comment

emilio Sep 7, 2019

Choose a reason for hiding this comment

emilio Sep 7, 2019

Choose a reason for hiding this comment

emilio Sep 7, 2019

Choose a reason for hiding this comment

lopopolo commented Sep 15, 2019

emilio commented Sep 16, 2019

emilio commented Sep 16, 2019

lopopolo commented Sep 16, 2019

emilio left a comment

Choose a reason for hiding this comment

lopopolo commented Sep 17, 2019