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

Poor interaction between workspace dependencies and default-features #12162

Open
ParkMyCar opened this issue May 20, 2023 · 11 comments
Open

Poor interaction between workspace dependencies and default-features #12162

ParkMyCar opened this issue May 20, 2023 · 11 comments
Labels
A-edition-next Area: may require a breaking change over an edition A-features Area: features — conditional compilation A-manifest Area: Cargo.toml issues A-workspace-inheritance Area: workspace inheritance RFC 2906 C-bug Category: bug S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request.

Comments

@ParkMyCar
Copy link

ParkMyCar commented May 20, 2023

Problem

Apologies in advance if this is categorized incorrectly, I can see how this could be either a bug or feature request.

Recently introduced to Cargo is the ability to specify dependencies at the workspace level. Crates within that workspace can specify their dependencies as { workspace = true }, which results in that crate inheriting the dependency from their parent workspace.

With respect to features the inheritance works in a purely additive way, which I believe works well in general, but not in the case of default features.

For example say you have a workspace with two crates, foo and bar, both of these crates depend on tracing. You want to make sure foo and bar depend on the same version of tracing, so you define it in your workspace and have crates inherit from it. In the crate bar you want to disable default-features for tracing, but you cannot since when you inherit from a workspace it's in a purely additive way. The only way to disable default-features in bar is to disable it at the workspace level, this has the adverse effect of also disabling default-features for foo. In the crate foo you could specify default-features = true, but this feels like an anti-pattern.

Scaling this example up a bit and you can better see how this is an issue. Say you have a workspace of 50 crates, 10 of which depend on tracing. In 1 of the 10 crates you want to disable default-features, to do that it requires you to specify default-features = true for the other 9, and any crates in the future that depend on tracing.

Steps

  1. Create a new Rust project, make it a workspace, add a workspace.dependency of tracing = 0.1.37 (or any other crate).
  2. Add a new crate foo, add a dependency of tracing = { workspace = true }.
  3. Add a new crate bar, add a dependency of tracing = { workspace = true, default-features = false }.
  4. Observe a compiler warning when building bar, indicating that the default-features flag is ignored.

Possible Solution(s)

Allow inherited dependencies within a workspace to disable default features.

Notes

No response

Version

$ cargo version --verbose                                                                                                                                                 
cargo 1.69.0 (6e9a83356 2023-04-12)
release: 1.69.0
commit-hash: 6e9a83356b70586d4b77613a6b33f9ea067b9cdf
commit-date: 2023-04-12
host: aarch64-apple-darwin
libgit2: 1.5.0 (sys:0.16.0 vendored)
libcurl: 7.86.0 (sys:0.4.59+curl-7.86.0 system ssl:(SecureTransport) LibreSSL/3.3.6)
os: Mac OS 13.2.0 [64-bit]
@ParkMyCar ParkMyCar added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels May 20, 2023
@ParkMyCar
Copy link
Author

Note: I tried a workaround of specifying default-features = true at the workspace level, and then default-features = false within bar at the crate level. That did not produce a compiler warning but bar still depended on default features, specifying false at the crate level had no impact.

@weihanglo
Copy link
Member

Thanks for the detailed report!

Totally understand the inconvenience. I am sorry for telling you that this is an intentional behavior for following the additive rule of Cargo features. Please see this table from this comment #11409 (comment). Changing the behavior could lead to a breakage.

Besides the intentional design, do you feel anything can be documented better in this chapter in Cargo Guide?

@ParkMyCar
Copy link
Author

Thanks for linking the comment! I had searched through the various PRs and RFCs to see if this was covered, I missed that one though 🙂

Could I propose changing the behavior then? What would be the right process for this, submitting an RFC maybe?

I totally understand that features within Cargo need to be additive, without this property any projects with a decent number of dependencies could fail to compile because the same crate could be depended on twice, but with conflicting feature sets, making the project impossible to compile. Because this additive property exists, having a crate opt out of a feature that the workspace enables should be entirely possible? In other words, it shouldn't cause any compilation issues? I don't believe the ability to generically opt out of a feature is needed, but without the ability to disable default features at the crate level, workspace inheritance is much less useful of a feature.

Recently I worked on migrating a decently large codebase to use workspace inheritance, MaterializeInc/materialize#19375, but ran into this issue with default features. Without the ability to disable default-features at a crate level, I don't think we'll go through with this switch. Which is disappointing because having a single location to define all of our dependency versions would be super useful!

Thanks for chatting about this 🙂

@weihanglo
Copy link
Member

FWIW, #3126 has more discussions about the future of default-features and some syntax to subtract a feature. Maybe this would help?

For proposing a change, I currently have no idea which route can fix this issue, as changing the behavior leads to another breakage. However, I think you can draft your idea here and if you can think of any transition to alleviate the potential breakage, please include them!

@weihanglo weihanglo added A-features Area: features — conditional compilation S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. A-workspace-inheritance Area: workspace inheritance RFC 2906 and removed S-triage Status: This issue is waiting on initial triage. labels Jun 1, 2023
@epage
Copy link
Contributor

epage commented Mar 20, 2024

I've had a growing feeling that being able to inherit things besides dependency sources was a mistake. Rarely do implementation requirements mean every package needs the same set of features, optional, public, etc. Instead, they diverge. The one exception is workspace hacks but workspace.dependencies alone is insufficient to support that.

If I felt confident enough in this stance, we could move to only allow inheriting source information in the next Edition (the wording is so that the package's Edition is in control since workspaces don't have an Edition). However, I am not confident enough at this time to move in that direction.

People can do it themselves (or with the help of a clippy lint) except for this issue.

I wonder if what we could do on an Edition boundary is to make it so that the lack of default-features in workspace.dependencies doesn't apply the default when inheriting but is ignored.

@Muscraft thoughts?

@epage
Copy link
Contributor

epage commented Mar 26, 2024

We talked about this in today's cargo team meeting.

Its a bit unclear if we'd want to go the full way and deprecate features, default-features in workspace.dependencies, removing them in an Edition.

  • We gave users a tool and might be hard to claw it back, even if its "correct"
  • We theorize most uses are for implementation focused and not requirements focused but its hard to say for sure

If we want to consider that, likely the best route is a short-term solution to this issue with a lint to discourage default-features / features. If the lint were in an opinionated linter like clippy, it could even be warn by default one day (ie be promoted from pedantic to style). Whether its allow or warn by default, we could search github to see how much it is used (while recognizing the bias that open source might be doing things differently than corporate).

For a short-term solution, it would likely be to make default-features in a workspace.dependencies be a Option<bool> where the None case means "unset" (compared to today where its "true")

One concern with this short-term route is that is isn't as consistent in behavior across cargo. If we deprecated default-features / features, then we would become consistent again, but instead of being consistent with dependencies, it would be patch. However, sometimes user expectations don't favor consistency and the more intuitive route is inconsistent. Maybe this is one of those cases.

We do need to recognize the cost of ecosystem churn with any of this. This would be a subtle behavior change that people would need to migrate through, both in code and in education. We'd also need to make the docs more complicated to cover the cases for each Edition.

@epage
Copy link
Contributor

epage commented Mar 26, 2024

To summarize the proposal, quoting from @Muscraft who built this on top of the work of @ehuss in #11409

Workspace Member 1.64 behavior 1.69 behavior 2024 edition behavior proposed 202X Edition behavior Reasoning
nothing df=false Enabled Enabled, warning that it is ignored Error Disabled Basically means let the package control default features when not specified
df=true df=false Enabled Enabled, warning Error Error Don't want the field ignored in the package
nothing df=true Enabled Enabled Enabled Enabled There is no conflict about default being enabled.
df=true df=true Enabled Enabled Enabled Enabled ^Same
nothing nothing Enabled Enabled Enabled Enabled No ambiguity.
df=true nothing Enabled Enabled Enabled Enabled Enabled
df=false df=false Disabled Disabled Disabled Disabled No ambiguity.
df=false df=true Disabled Enabled Enabled Enabled This allows members to decide whether or not they want default features. The workaround of features=["default"] seems unnecessary.
df=false df=nothing Disabled Disabled Disabled Disabled Natural way to write "give me whatever was written in workspace".

Note that #13629 could #13839 did turn those warnings into errors in the next Edition.

@LukeMathWalker
Copy link

The proposal looks good to me @epage.

I'd prefer an error in scenario no. 2, since we have the freedom to do that at an edition boundary.

@ParkMyCar
Copy link
Author

Hey @epage, thanks for reconsidering this! The behavior table you listed above makes sense to me and fixes the original concerns I had.

FWIW I also like the idea that workspace dependencies should only be able to inherit source information, but I think the correct long term solution is a lint guiding users away from it, instead of disallowing it entirely. An example of where it would be nice to inherit features from the workspace is when it's a performance related feature, e.g. smallvec's union feature. You might want to enable union for all uses of smallvec in your workspace, and workspace dependencies seems like the right tool for that. All of this to say, if a lint is created it would be nice if users could exclude specific crates.

@epage
Copy link
Contributor

epage commented Apr 1, 2024

I'd prefer an error in scenario no. 2, since we have the freedom to do that at an edition boundary.

I left that out because its being talked about as part of #13629

@epage
Copy link
Contributor

epage commented May 2, 2024

A way to reframe this: instead of merging a package dependency into a workspace dependency, keeping the workspace dependency's defaults, we could switch to merging a workspace dependency into a package dependency, keeping the package dependency's defaults.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-next Area: may require a breaking change over an edition A-features Area: features — conditional compilation A-manifest Area: Cargo.toml issues A-workspace-inheritance Area: workspace inheritance RFC 2906 C-bug Category: bug S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request.
Projects
Development

No branches or pull requests

4 participants