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

RFC: Cargo feature descriptions #3485

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

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Sep 9, 2023

Rendered

RFC for feature-documentation
RFC goals: add a way to write feature descriptions in Cargo.toml

This was split from #3416

[features]
# current configuration
foo = []
# Add a description to the feature
bar = { enables = ["foo"], doc = "simple docstring here"}

# Features can also be full tables if descriptions are longer
[features.qux]
enables = ["bar", "baz"]
doc = """
# qux

This could be a longer description of this feature
"""

This would resolve rust-lang/cargo#4956

FCP

@tgross35
Copy link
Contributor Author

tgross35 commented Sep 9, 2023

@rustbot label +t-cargo

@rustbot
Copy link
Collaborator

rustbot commented Sep 9, 2023

Error: Label t-cargo can only be set by Rust team members

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@compiler-errors compiler-errors added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Sep 9, 2023
I directly applied my more minor feedback that did not change the
characteristic of the RFC.
@epage
Copy link
Contributor

epage commented Sep 10, 2023

@tgross35 thanks for the access. Definitely makes it handy for the more trivial feedback.

Comment on lines +159 to +160
- Rather than being consistent with `rustdoc` and accepting markdown, should the
`doc` key be consistent with `package.description` and only support plain
Copy link
Contributor

Choose a reason for hiding this comment

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

Wanted to highlight this for discussion. My main interest is in being able to show summaries in cargo add. Might be good to reach out to @kornelski for what they have seen of how features are documented through the ecosystem as that might help show potential requirements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Package descriptions tend to use markdown `code` and *emphasis*. Rust devs really like using ` everywhere. Even rustc uses ` in terminal error messages.

Markdown's goal is to look fine even when displayed as plain text.

You could define it as the first line being for CLI help, and the rest for docs. Analogous to how rustdoc handles doc comments.

Copy link
Member

Choose a reason for hiding this comment

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

@kornelski 👍 for using markdown. And I think it makes sense to treat the first logical line (anything before the first double-newline) as a "short description", cutting off subsequent paragraphs in places where full documentation doesn't fit.


This RFC describes a new key to under `features` in `Cargo.toml` for
documentation. This will allow Cargo to display this information to the user and
provide a way for `rustdoc` to eventually render this data (how this is rendered
Copy link
Member

Choose a reason for hiding this comment

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

One of the issues is how rustdoc consumes the data. rust doc generally knows nothing about Cargo.toml. I would suggest taking #3123 as a reference to start a discussion on cargo-rustdoc integration of this. It doesn't need to be perfect but at least two teams should have some consensus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did bring it up when I initially proposed this feature, https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Descriptions.20for.20feature.20flags and then opened a draft RFC suggesting that rustdoc accept JSON configuration, which Cargo could pass it #3421. That didn't get too much traction, though. I will start that discussion back up

@joshtriplett
Copy link
Member

Since RFC 3416 is in FCP, that unblocks this feature.

I'm going to go ahead and propose FCP here, and see how close we are to consensus.

Personally, I do think we should use markdown here. Markdown already looks reasonable in plain text, by design, and it gives crates.io and docs.rs and rustdoc something nicer to work with.

@joshtriplett
Copy link
Member

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jun 21, 2024

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

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.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Jun 21, 2024
@weihanglo
Copy link
Member

weihanglo commented Jun 22, 2024

@rfcbot reviewed

Though I still want to call out that intra-doc links are important #3416 (review). CC @GuillaumeGomez if they have any opinion on this.

@GuillaumeGomez
Copy link
Member

This is a very good point. Thanks for the ping!

# Tables are preferred for longer descriptions
[features.corge]
enables = ["bar", "baz"]
doc = """
Copy link
Member

Choose a reason for hiding this comment

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

Another good question raised was "should we also support intra-doc links in this documentation?". I personally think we should and make the context the same as the crate top-level. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see you mentioned it below, my bad.

Copy link
Member

@weihanglo weihanglo Jun 22, 2024

Choose a reason for hiding this comment

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

Thanks for the response!

The follow-up question from me would be: Is there any compatibility issues if we hadn't implemented this RFC and rustdoc change all together? If not then this RFC can safely go first.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure. From cargo perspective, whether there are intra-doc links or not in this documentation doesn't matter. But it'll definitely need to be mentioned when support will be discussed in rustdoc.

For the current case, I think cargo should mention that intra-doc links may be supported when rustdoc support this option and that's it. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me.

@tgross35 could you add something like this? (or whichever way you'd like to rephase this)

- Rustdoc can build on this to show feature documentation.
+ Rustdoc can build on this to show feature documentation.
+ If this RFC gets stabilized before any corresponding change in rustdoc,
+ its documentation should highlight that rustdoc may parse the description and support intra-doc links in the feature.
+ Users need to be aware of this potential incompatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, thank you!


[guide-level-explanation]: #guide-level-explanation

A new `doc` key will be allowed within a feature's table. This key provides a
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming (not finding the past discussion on it)

  • doc mirrors #[doc(...)]
  • description mirrors package.description
  • documentation mirrors package.documentation (but that is a URL)

We also tend to not use abbreviations as much. For example, for public-private dependencies, we discussed using pub vs public and went with public

Copy link
Member

Choose a reason for hiding this comment

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

@epage In my opinion, I think this should be doc.

I think we should encourage using this not just for a "description" of a feature but for "documentation" for a feature, and that's supported by the idea of it showing up in rustdoc and allowing markdown.

And I think there's a big difference in length between doc and documentation, compared to the difference between pub and public. (I personally would have gone with pub for that too, based on widespread usage of pub within Rust, but I also think public was less obtrusive because it's fewer characters.)

It's hard enough to get people to write documentation; let's not have any extra friction.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I personally would have gone with pub for that too, based on widespread usage of pub within Rust, but I also think public was less obtrusive because it's fewer characters.)

Personally, I'd prefer we be consistent and not just focusing on character count.

It's hard enough to get people to write documentation; let's not have any extra friction.

We've shot well past that by requiring a "long form" just to write documentation, hurting both discoverability, character count, and ease of not typing some of those characters on international keyboards.

@epage
Copy link
Contributor

epage commented Jul 9, 2024

@rfcbot concern naming

See my comment

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 15, 2024

This technically avoids promising work from the rustdoc and docs.rs teams, but that seems like a technicality. The obvious follow-up is for those teams to do something with this information. I have been in adequately following the conversation here. Have they been consulted? Given that none of the technical details have been decided, I don't think their checkboxes should be required, but I also don't want this to be a surprise to them.

@rfcbot concern docs-included-in-design

(Anyone should feel free to resolve this concern if members of those teams have been consulted.)

@GuillaumeGomez
Copy link
Member

In the case of docs.rs, it has access to more information so it's not really an issue. The big unknown here is how this information could be given to rustdoc. I suppose through a command-line argument. My only worry about this solution is about the command-line argument size limitation. However, it can be completely nullified by using an @ argument. So I don't see any blocker from rustdoc side either (on how to provide this information).

The big discussion will actually be: how do we display this information in the generated documentation?

@syphar
Copy link
Member

syphar commented Jul 15, 2024

This technically avoids promising work from the rustdoc and docs.rs teams, but that seems like a technicality. The obvious follow-up is for those teams to do something with this information. I have been in adequately following the conversation here. Have they been consulted? Given that none of the technical details have been decided, I don't think their checkboxes should be required, but I also don't want this to be a surprise to them.

I do follow this PR from the sidelines, though not all the discussion on here.

( also, @GuillaumeGomez already said what I wanted to say)

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 15, 2024

@rfcbot resolve docs-included-in-design

@obi1kenobi
Copy link
Member

The big discussion will actually be: how do we display this information in the generated documentation?

Tangentially: I hope this information might also become available in rustdoc JSON one day, for the benefit of tools like cargo-semver-checks. It's possible to cause semver breakage by changing crate features, and right now, detecting such breakage requires parsing the Cargo.toml file which can be trickier than it may seem at first.

@GuillaumeGomez
Copy link
Member

You can ping me directly if it's not there but it's present in the HTML doc. I'll add it as fast as I can if I forgot. ;)

@syphar
Copy link
Member

syphar commented Jul 15, 2024

The big discussion will actually be: how do we display this information in the generated documentation?

and following from there, what happens to the docs.rs view, especially since we also have rust-lang/docs.rs#2530 and rust-lang/docs.rs#2531

@tgross35
Copy link
Contributor Author

tgross35 commented Jul 15, 2024

In the case of docs.rs, it has access to more information so it's not really an issue. The big unknown here is how this information could be given to rustdoc. I suppose through a command-line argument. My only worry about this solution is about the command-line argument size limitation. However, it can be completely nullified by using an @ argument. So I don't see any blocker from rustdoc side either (on how to provide this information).

Does rustdoc already have access to the output of cargo metadata? If so, I think this should eventually wind up there even though I didn't mention it in this RFC (@epage does it seem reasonable to add it?).

I had a draft RFC for a schema to give rustdoc this information if some non-Cargo tool wants to make use of the features display, but would probably need to rework it a ton at this point #3421

The big discussion will actually be: how do we display this information in the generated documentation?

I preemptively opened a tracking issue for rustdoc support and will put some ideas there (I am sure you have some already too :) ) rust-lang/rust#127771. Also noted JSON support as something to consider from @obi1kenobi's comment.

The big discussion will actually be: how do we display this information in the generated documentation?

and following from there, what happens to the docs.rs view, especially since we also have rust-lang/docs.rs#2530 and rust-lang/docs.rs#2531

I don't know what would happen to the docs.rs view; there doesn't seem to be much harm in keeping it but maybe also not much sense in augmenting it much further if rustdoc will begin to cover it? Those issues could likely be addressed in whatever rustdoc winds up generating here.

@GuillaumeGomez
Copy link
Member

In the case of docs.rs, it has access to more information so it's not really an issue. The big unknown here is how this information could be given to rustdoc. I suppose through a command-line argument. My only worry about this solution is about the command-line argument size limitation. However, it can be completely nullified by using an @ argument. So I don't see any blocker from rustdoc side either (on how to provide this information).

Does rustdoc already have access to the output of cargo metadata? If so, I think this should eventually wind up there even though I didn't mention it in this RFC (@epage does it seem reasonable to add it?).

Easy answer: same as rustc (so no as far as I know).

The big discussion will actually be: how do we display this information in the generated documentation?

I preemptively opened a tracking issue for rustdoc support and will put some ideas there (I am sure you have some already too :) ) rust-lang/rust#127771. Also noted JSON support as something to consider from @obi1kenobi's comment.

👍

The big discussion will actually be: how do we display this information in the generated documentation?

and following from there, what happens to the docs.rs view, especially since we also have rust-lang/docs.rs#2530 and rust-lang/docs.rs#2531

I don't know what would happen to the docs.rs view; there doesn't seem to be much harm in keeping it but maybe also not much sense in augmenting it much further if rustdoc will begin to cover it? Those issues could likely be addressed in whatever rustdoc winds up generating here.

docs.rs and rustdoc are tangent. They can both provide an information in different ways.

@epage
Copy link
Contributor

epage commented Jul 15, 2024

I think this should eventually wind up [in cargo metadata] even though I didn't mention it in this RFC (@epage does it seem reasonable to add it?).

So while this isn't relevant for #3421, this is a good point more generally and one we overlooked in #3416. We need to decide if we want to include this in cargo metadata and what that schema would look like. Would we need a parallel structure? Update the format version?

@rfcbot concern cargo-metadata

@tgross35
Copy link
Contributor Author

tgross35 commented Jul 15, 2024

I think this should eventually wind up [in cargo metadata] even though I didn't mention it in this RFC (@epage does it seem reasonable to add it?).

So while this isn't relevant for #3421, this is a good point more generally and one we overlooked in #3416. We need to decide if we want to include this in cargo metadata and what that schema would look like. Would we need a parallel structure? Update the format version?

@rfcbot concern cargo-metadata

The most elegant schema would be to mirror these tables directly into metadata. That is, update:

"features": {
    "foo": ["bar"],
    "bar": [],
    "baz": []
}

To be something like:

"features": {
    "foo": { "enables": ["bar"], "doc": "Enable wxyz" },
                               // ^ bikeshed still being built
    "bar": { "enables": [], "doc": "Make use of ..." },
    "baz": { "enables": [] } // < only having one serialization seems easier than allowing
                             // `"bar": []`, but doesn't matter much
}

However; being that Cargo needs to support all possible schema versions forever (as I understand it), it seems like the barrier for a breaking schema change is quite high. So maybe we should add a new features_metadata object next to features that adds this information in a redundant but backward-compatible way. And then just put replacing features with features_metadata on a "wanted for v2" list for whenever there is enough to justify the new version.

@amab8901
Copy link

why is this PR still open if the parent is closed?

@tgross35
Copy link
Contributor Author

tgross35 commented Aug 17, 2024

why is this PR still open if the parent is closed?

The RFC you linked only establishes a format for adding more metadata to features, it doesn't actually confirm that we want to add anything specific. This RFC is about adding documentation as part of metadata, but needs resolution to the two listed concerns.


@epage does #3485 (comment) sound reasonable? If so, I will update the RFC.

If accepted here we may want to consider updating #3416 so that the feature schema isn't provided only in the documentation RFC - obviously up to you to weigh that vs. updating an accepted RFC.

@epage
Copy link
Contributor

epage commented Aug 19, 2024

@tgross35

The most elegant schema would be to mirror these tables directly into metadata.
That is, update:

<simple form>

To be something like:

<detailed form>

However; being that Cargo needs to support all possible schema versions forever (as I understand it), it seems like the barrier for a breaking schema change is quite high.

As these details seem to be implicit, you are saying this would be a new --format-version <VERSION>?

Yes, 'wed need to support generating data for all possible schemas for ever.

Pulled in your ideas and added some more. Mind adding this to the RFC alternatives and documenting which you think we should go with and why? We can then discuss it in a cargo team meeting.

A new message format version

We either deprecate the current version, making new features all-or-nothing, or need to add features to both versions

As the format-version is defaulted (with a warning), we'd need to consider what the migration path would be to v2.

A parallel table for just this

This is your features_metadata.

We need to do this for every new field. While #3663 was closed, doing something like that would put coupled data in parallel tables.

A new features2 or features-detailed

Automatically gets new data.

Seems inelegant but that isn't necessarily a blocker.

Use simple form where possible

Technically, this gets us into the territory of "forwards compatibility" rather than "backwards compatibility", ie things break when new features are used with old tools rather than breaking existing features on new tools.

We'd need to make sure we render in the "simple" format whenever we can.

This can still be disruptive to the ecosystem. serde handles adding of new fields well (ignore) but not changing of data types (error). Most people go through cargo_metadata so we'd likely only need to implement things once and then people just need to update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. T-cargo Relevant to the Cargo team, which will review and decide on the RFC.
Projects
Status: FCP blocked
Status: Unreviewed
Development

Successfully merging this pull request may close these issues.

Allow adding a description for features