-
Notifications
You must be signed in to change notification settings - Fork 12
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
Buffrs library pulls in a lot of dependencies that are not needed. #143
Comments
@xfbs we already make use of cargo features to only inline the bare minimum of features for each target. Maybe we can improve them, but i dont think we need to split crates here? |
I'm with @xfbs on this. I'm trying to use buffrs, and I'm finding it's pulling in (in my cargo build):
Each of these adds a few seconds to my compile, are duplicated due to buffrs being a build and normal dependency, and worse, the requirements for rebuilding are highly frequent on environment changes. Essentially, running 'cargo clippy' from a vscode shell and then running it from iterm is enough to cause these deps to require a full rebuild (20 seconds overall) whereas without them, the build would only require compiling my crates. My understanding is that it's not easy to have a dependency be default included with a binary target, but optional if depended on as a lib. At present, I think I can work around this by depending on buffrs as:
or similar. But it feels unfortunate that using buffrs as documented needlessly slows down your development, and my experience is that crate boundaries are the correct way to ensure appropriate dependency choice. In particular, I think the thing that is being traded off here is:
It's unsurprising to me that buffrs is accidentally slowing down cargo builds given that most folks developing on it are devving on the cli. |
@j-baker, I think I had identified the similar issues as you. Even thought we already do some feature-gating as @mara-schulke suggested, we might have a fundamental issue right now. Problem definitionUnder the same
In other words: the default feature (and thus dependency) sets of these two use-cases are different. Ideally, you want to be able to add this to your project's dependencies and be sure to get [dependencies]
buffrs = "1.2.3" We can actually kind of split the library use-case into twoWe can actually even split the library use-case into two separate use-cases:
So, a usage might actually look like this: [dependencies]
# literally only the `include` macro?
buffrs = "1.2.3"
[build-dependencies]
# enable protobuf dependencies?
buffrs = { version = "1.2.3", features = ["build"] } At the same time, we want to have a CLI which is fairly comprehensive. It might depend on a lot of nice crates, such as With the current setup, we cannot easily achieve this. Currently, even using As far as I understand, we have two approaches to make this work, I will list both with their respective implications: Solution spaceFeature-gating the CLIThe idea here:
This is how we would tell Cargo: [features]
# ...
cli = ["clap", "git2", "..."]
[[bin]]
name = "buffrs"
required-features = ["cli"] Now, when a project uses The downside of this approach is that it makes installing the CLI counter-intuitive: instead of being simply able to
This might be a bit annoying, because when you forget to add the And we probably (?) also don't want to add Splitting cratesThe other option is to have a separate CLI crate.
So we basically add a folder called [package]
name = "buffrs-cli"
# ...
[[main]]
name = "buffrs" # override name of binary
path = "src/main.rs" This package will then still export a This would mean that installing the CLI would look like this:
This is a small change, but I feel that this might be the easiest and "future-proof". ConclusionLet's maybe get @mara-schulke's input on this, to see if any of the ideas here make sense. This is not urgent, but I think if we put a little thought into this we can definitely achieve some rather large improvements. |
To add my two cents, I have the same observations. My own preference goes strongly to splitting Buffrs into separate crates, especially since there is already a separate crate for the registry. My reasoning is that crates are much easier to discover than features within a crate. They also enable (more) parallel compilation. |
buffrs
is two things at oncebuffrs::include!()
The unfortunate side effect of that is that buffrs comes with a lot of dependencies, some of which are not needed when using it as a library.
A common pattern here is to do one of two things:
buffrs
into multiple crates. For example, there could be abuffrs
orbuffrs_core
crate which implements the library side of things, and abuffrs-cli
crate which implements the CLI. We can still name the binary it producesbuffrs
if the did call this onebuffrs-cli
.cli
feature to buffrs, mark thebinary
as being conditional on the presence of this feature, and only enable certain things (such asmiette
'sfancy
feature) when this feature is present.Doing this would also help us in some way by having an explicit error boundary, where we could use structured errors on the library side and unstructured ones on the binary side, as currently the two are a bit mixed.
This is not really high-priority, but something to think about.
The text was updated successfully, but these errors were encountered: