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

Added 'binary' feature to reduce lib dependencies #26

Merged
merged 1 commit into from
Feb 3, 2023

Conversation

blumbra63
Copy link

@blumbra63 blumbra63 commented Feb 3, 2023

I have an application which depends on rust-industrial-io and I'm looking to improve its build time. I noticed that this crate pulls in clap, but it is only needed for iio_info_rs binary, and I only depend on the libraries. On my machine this reduces the lib-only build time from ~6.3 to ~5 seconds and removes several dependencies.

I'm not sure if this is the best way to separate dependencies by binary vs. library builds. It looks like there is an RFC rust-lang/rfcs#3374 to provide a better way to do this, but I think this is the best that exists right now.

@fpagliughi
Copy link
Owner

Hey, thanks for the PR.

Yeah, I'd be happy to add this sort of thing; I've actually started doing this more and more in my other libraries.

But I think we should use a better name for the feature. I've been using the term "utilities" for this. Would that work? (I'm also assuming more will be added in the future, so make it plural and not just "utility").

Then it's a question of whether it's on by default to keep backward compatibility? I'm on the fence about this one.

@fpagliughi
Copy link
Owner

fpagliughi commented Feb 3, 2023

Oh, and my preferred way to do this at the moment is entirely in Cargo.toml - so no #![cfg(...)] needed in the source files. Change/add this to the bottom of Cargo.toml:

[features]
default = ["utilities"]     # Maybe?
utilities = ["clap"]

# ----- Utilities -----

[[bin]]
name = "iio_info_rs"
required-features = ["utilities"]

[[bin]]
name = "riio_stop_all"
required-features = ["utilities"]

# ... repeat for any other binaries we add in the future ...

@blumbra63
Copy link
Author

blumbra63 commented Feb 3, 2023

Thanks for the quick feedback. "utilities" sounds like a fine name. I also much prefer your cargo-only way of handling the feature. Thanks for the suggestion.

Regarding if the "utilities" feature is on by default, I wasn't sure either. I slightly lean yes, but don't have a strong opinion. Getting only the necessary dependencies by default would be nice, but there's something beautiful about the simplicity of a working cargo build. Users will get the same behavior as today, by default, and can opt in to receive the optimization.

If/when the RFC above is added to cargo, I can add enable-features = ["utilities"] to the bin targets.

@fpagliughi
Copy link
Owner

Cool. Yeah, let's keep the feature on by default, at least for now, especially if there's not a good reason to break backward compatibility. That way I can push this out quickly in a minor update without any breaking changes. We can always change our minds in the next major version update.

I'll merge this into the repo and have a look at it all over the weekend. There are a few minor updates that have been sitting in the develop branch for literally a year now. I should push it all to a new v0.5.2, or something like that.

Soon, I'll start on some other, bigger updates. I think.

@fpagliughi fpagliughi merged commit 23d9422 into fpagliughi:develop Feb 3, 2023
@fpagliughi
Copy link
Owner

Nothing that was pending in the develop branch (this included) seemed to break backward compatibility. So I just published it to crates.io as v0.5.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants