-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Allow specifying dependencies for individual artifacts #2887
Conversation
@ehuss I've been reviewing @dmarcuse's efforts on this for a while now (she's a new contributor), and would like to say it's in a great state to get some official feedback. I believe you're the appropriate person here, so if you could have a look and give any feedback, that would be much appreciated! |
It would be great to have this fixed! I'm a bit concerned about the proposed solution direction of adding separate per-target dependency tables -- as the RFC mentions, it would make it harder to get a good understanding of the different dependencies involved. Instead I would propose a new per-target (I believe the second unresolved question would be solved with the new resolver that @ehuss is working on, starting from rust-lang/cargo#7820.) |
One of the stickler points in the past about this feature has been what to do about the crates.io index. The index is where Cargo learns dependency information for crates.io-sourced packages, and the dependencies (even optional/target-specific ones) are currently in the index. A relatively large change that this would naively require would be to update the index with all this new information. In any case I think it's important for the RFC to address the situation about the index. I have an idea, however, which means that the index doesn't have to get updated (which is always a tricky thing to do). The main "target" that the crates.io index cares about is the library itself, not any binary, example, or test suite. The binaries and examples only come into play with To fix Put another way, I think we can get away with not updating the index at all here and continue to function normally. In discussing this with other Cargo folks the main thing that we'd lose is a high-fidelity view into reverse dependencies because the index/crates.io would not be informed of bin-specific dependencies. We could try to fix that with extra JSON information on publish which doesn't go into the index, only crates.io's database, but it's altogether not the worst downside! |
I personally don't like the indirectness of this solution. It would be nice to allow binaries to enabled features as well, however. Like how downstream crates can. |
@alexcrichton Sounds like a nice approach that doesn't lose too much in honesty. I guess it all depends how painful it would end up being to update the index, which I really don't know... but your solution as I understand is not what I'd called "hacky", at least! :-) |
I'm not familiar with how the crates.io index works, but that sounds like a good workaround to me. Would it negatively impact build times when using |
One concern about not being in the index is that future changes like rust-lang/cargo#4316 would be more difficult (not a blocker, just something to note). It might be worth adding Swift to the "Prior Art" section, and taking a look at how they approached it. Just a few days ago they released a new version that included essentially the same thing this RFC proposes. You can read more about it at https://github.com/apple/swift-evolution/blob/master/proposals/0226-package-manager-target-based-dep-resolution.md. One interesting thing about their approach is that each target lists the dependencies it uses by name, but keeps the actual dependency declarations in one place. I think that would be more similar to the idea of having a set of default features for a target that could enable optional dependencies. I wouldn't completely dismiss the idea of using default target features instead. I think that would be substantially easier to implement, easier to integrate with tooling, would avoid the reverse-dependency problem, and be easier to document/explain. It would also make some scenarios easier to express. For example, if you have 3 binaries that depend on clap, you would only need to list |
I agree that there's overlap with
|
I don't think anyone is arguing that there's overlap with While the In the case of adding dependencies to a single build target, it is arguably a bit more work (but note that dependencies marked as It seems to me that features are already defined crate-wide, so the situation of features being defined for a In any case, the features approach should be clearly explained in the alternatives section, with the attendant advantages and disadvantages. |
To be clear, I wasn't advocating doing anything with
I think it will be more work if you have more than one target that wants to use the same dependency. Although for the single-use situation it is less work, the difference is pretty small. Compare: [dependencies]
clap = {version="2", optional=true}
[[bin]]
name = "foo"
features = ["clap"] to: [[bin]]
name = "foo"
[bin.dependencies]
clap = {version="2"} Mostly a difference of one line. I agree that conceptually it is not as straightforward in this simple case, but once you have just one more binary, it becomes worse. And it becomes conceptually more complex for any tool that works with Cargo's dependencies. They will all need to be updated to handle target-specific dependencies, whereas with features most of them should work as-is. It's also not clear to me what happens if multiple targets define dependencies on the same thing. Do they have to be exactly the same? If there are differences, how are those handled?
How would it be inefficient? I believe it would be essentially zero cost. |
From my perspective, having bin targets enabling features would be something of an abuse of the purpose of features. As far as I know (and intuitively), they're meant to be externally-facing boolean parameters that change the feature set (and only as a by-product the dependencies) of a particulate crate that is getting used. Sometimes a bin target would conceptually want to enable a feature for the crate lib, because that's the nature of the binary, but sometimes it just needs a few extra dependencies. That would entail creating a dummy feature, which would itself be usable by downstream crates (not intended), and would require a dummy name which is referenced in As for the "cost", I don't think there's anything more than the most minute cost for the more indirect feature-based approach, but I would hazard a guess that @dmarcuse meant "inefficient" in terms of characters in the |
Yep, @alexreg is exactly right about what I was trying to say. |
I find it annoying when I run So I'd like to see a more lenient handling of |
Cargo is somewhat undecided about what should be in scope of a single crate, and what should be a workspace. For example, Cargo is supposed to discourage users from having both lib and bin targets in a single crate: rust-lang/api-guidelines#167 And this feature goes in the other direction: helps having more targets in a single crate. So maybe this feature should not exist?
|
@kornelski you bring up some good points, but I disagree with your perspective that separate crates in a workspace should be used here. Using separate crates does allow you to fully separate the dependency trees in a way that's already supported by cargo, but it introduces its own complexities:
|
I got confused because I tried to run the player with: cargo run --examples demo_player However this did not work because demo_player is structured as a subcrate in a workspace, not as an example. Cargo examples do not have a src directory nor their own Cargo.toml: https://doc.rust-lang.org/cargo/guide/project-layout.html Unfortunately there is no way to specify dependencies only for an example. There is an open RFC for that: rust-lang/rfcs#2887 So for now, I think it is best to leave the demo applications as subcrates in a workspace, but rename the "examples" directory to "demos" so people don't think they will work with cargo run --examples
Doesn't changing the enabled features trigger a full rebuild of the lib target? |
Overall, this is a very good RFC addressing a problem since 2015. I'm alongside @dmarcuse advocating a direct per-crate dependency list, avoiding using features for this kind of dependency specification. The reasons are pretty much as explained by @dmarcuse and others: Features are indirect, require more efforts, and don't play nicely with editors. (Cargo) features are meant to provide (program) features. Specifying dependencies is often not a feature. The fact that my bin crate uses Issues have been filed repeatedly within these years, rust-lang/rust#95513 and rust-lang/cargo#12854 and some others. I don't think there's a nice solution without per-crate dependencies. Playing with features on this is going astray. The fundamental problem is cargo compiles each crate on its own thus unifying dependencies of different crates simply doesn't make sense. While there could be a common dependency list for all crates, it shall not remove the ability to specify dependencies for each crate. However, one problem I see in this RFC itself is its terminology. For example, I guess when it says:
It actually means:
This RFC universally substituted crate for package and artifact for crate, which I think is a mistake. |
I don't see it on the RFC so I thought i will ask but does [bin.dependencies.myproject_cli]
clap = "*" and [bin.dependencies.myproject_cli.clap]
version = "*" would work ? Or maybe this syntax is induced by that of the RFC (I am not a toml expert) |
Why not use the optional syntax for this concern ? |
Also, I wanted to share another use case for this i haven't saw. It would be to use only some portion of code of a package, without including his dependencies. I don't think you could to that without this rfc. |
@rfcbot close #3374 was created as an alternative to this but has since been closed. A major problem that had that is also shared with this is that it takes an existing problem for users (feature unification) and applies it not just to the package level but to the individual build target (see #3374 (comment)). Adding new dependencies tables would also cause a significant ecosystem churn that we need to be mindful of. Note also that this isn't the only problem in dealing with wanting a package to scale to complex build targets needs. We discussed this as a team. For myself, I think we should focus more on improving the experience with workflows before we re-evaluate how much more we should do for improving complex package needs. |
Team member @epage has proposed to close this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
I'd have liked this RFC or something like it such that, if I have a binary and a library in the same crate, where the binary requires
to build just the library without pulling
to build the binary with The current With this RFC being closed, is there any path towards that behavior being possible that's being pursued? |
@JarredAllen did you already read the discussion referenced in #2887 (comment)? |
The final comment period, with a disposition to close, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This is now closed. |
This RFC proposes adding a way to specify dependencies (in a cargo manifest) that are only used for a single artifact (lib/bin/etc). This is intended to allow crate authors publishing multiple artifacts in a single crate to avoid pulling in dependencies unnecessarily, improving compile times and binary size.
Previous discussion
Rendered