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

Cargo structured syntax for feature dependencies on crates #3663

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Jun 21, 2024

This RFC extends Cargo's structured table form of features to include a deps
key, to unambiguously specify packages the feature depends on without having to
use the dep:foo microformat.

[features]
myfeature = { deps = ["some-crate", "another-crate"] }
# This is equivalent to `myfeature = ["dep:some-crate", "dep:another-crate"]`
# This can also be written in shorthand form:
myfeature.deps = ["some-crate", "another-crate"]

This incremental RFC was inspired by the incremental, easier-to-review RFCs
currently being used for other individual pieces of the structured table format
for features.

Rendered

This RFC extends Cargo's structured table form of features to include a `deps`
key, to unambiguously specify packages the feature depends on without having to
use the `dep:foo` microformat.

```toml
[features]
myfeature = { deps = ["some-crate", "another-crate"] }
# This is equivalent to `myfeature = ["dep:some-crate", "dep:another-crate"]`
# This can also be written in shorthand form:
myfeature.deps = ["some-crate", "another-crate"]
```
@joshtriplett joshtriplett added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Jun 21, 2024

In the future, we could also provide an alternative to the
`crate-name?/feature-name` microformat, or alternatively we could migrate
towards making `crate-name/feature-name` mean that in a future edition. This
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My -1 is mainly because of putting pkg?/feat and pkg/feat into "future possibilities" 😕. Without "structurizing" DepFeature, the introduction of deps key puts the transition in an awkward incomplete state as you still need to use the microformat to enable features of a dep. Better just postpone if it's not ready.

I don't think actually defining DepFeature here makes the RFC harder to review, this RFC is already short enough.

Copy link
Member

@weihanglo weihanglo Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you still need to use the microformat to enable features of a dep

Agree. Not considering DepFeature as a whole may end up having three ways to define that. It's better to one or the other, not a combination of them.

# old
[features]
tracing = ["valuable", "log/std"]

# this
[features]
tracing = { enables = ["valuable", "log/std"], deps = ["log"]  }

# future maybe?
[features.tracing]
enables = ["valuable"]
deps.log = ["std"]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree from at least a specific perspective. For me, the thing that has bothered me is the inconsistent way of referring to dependencies:

feature_name
dep:dep_name
dep_name/feature_name

I feel like this should be

feature_name
dep:dep_name
dep:dep_name/feature_name

(this is worse with implicit features because deps and features get muddled but 2024 fixes that)


However, if a crate uses the `deps` key, it should consistently use the `deps`
key for all feature dependencies on crates, rather than using a mix of `deps`
and `"dep:"`. (Cargo may choose to warn about mixing the two.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a brand-new syntax, believing that it could be an error.
A future possibility is that we ship this as a hard error, and later become configurable via cargo-lints.


In the future, we could also provide an alternative to the
`crate-name?/feature-name` microformat, or alternatively we could migrate
towards making `crate-name/feature-name` mean that in a future edition. This
Copy link
Member

@weihanglo weihanglo Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you still need to use the microformat to enable features of a dep

Agree. Not considering DepFeature as a whole may end up having three ways to define that. It's better to one or the other, not a combination of them.

# old
[features]
tracing = ["valuable", "log/std"]

# this
[features]
tracing = { enables = ["valuable", "log/std"], deps = ["log"]  }

# future maybe?
[features.tracing]
enables = ["valuable"]
deps.log = ["std"]

Comment on lines +82 to +88
# Drawbacks
[drawbacks]: #drawbacks

- This adds minor incremental complexity to parsing the manifest, both for
Cargo itelf, and for third-party tools that parse the manifest without using
Cargo's parser.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're providing two ways for the user to do something which can cause extra confusion.

Comment on lines +82 to +88
# Drawbacks
[drawbacks]: #drawbacks

- This adds minor incremental complexity to parsing the manifest, both for
Cargo itelf, and for third-party tools that parse the manifest without using
Cargo's parser.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we are saying to always use the "detailed" (table) format, the user has to translate between two syntaxes as their use case grows.

Comment on lines +36 to +45

This has created somewhat of a microformat within the dependencies list, and
this microformat adds complexity compared to other parts of the Cargo manifest,
which distinguish different kinds of things via different keys or sections. (By
way of example, we have `dev-dependencies` and `build-dependencies`, rather
than a microformat like `dependencies."build:xyz"`.)

Now that [RFC 3416](https://github.com/rust-lang/rfcs/pull/3416) has defined a
structured table form for defining features, this RFC takes a first step
towards providing structured equivalents in place of the microformat.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have the cfg microformat.

Overall, I feel like judicious use of microformats extends what a standardized grammar allows and can enhance the format. For me, this motivation feels very weak and the change possibly a net negative while encouraging a wide ecosystem churn that I'm not seeing the pay off for.

@chrysn
Copy link

chrysn commented Jun 27, 2024

I like this because of previously unmentioned future possibilities: It paves the way to have documentation on features in a more parsable format than what is currently used for the document-features crate, and it could provide machine readable descriptions for the impact on stability of a given feature (when features are used in a way that alters semver guarantees, or increase the MSRV).

[edit: Comment should have been to 3416, sorry for the noise, thanks for the clarification in #3663 (comment)]

@epage
Copy link
Contributor

epage commented Jun 27, 2024

Note that the table syntax is independent of this and its RFC has been merged. There is a description RFC up.

feat1 = { enables = ["feat2", "feat3"], deps = ["crate1"] }
feat2 = { deps = ["crate2"] }
feat3.deps = ["crate3"]
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a typo?

Suggested change
"""

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-cargo Relevant to the Cargo team, which will review and decide on the RFC.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

6 participants