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

recommended-bin-packages field in Cargo.toml #3383

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

Conversation

not-my-profile
Copy link

@not-my-profile not-my-profile commented Feb 4, 2023

This RFC proposes the addition of an optional recommended-bin-packages field to the [package] section of Cargo.toml, to enable package authors to point out related binary packages in the error message Cargo users get when attempting to cargo install a package without binaries.

Rendered
FCP

(Opening an RFC for this since the idea has gotten positive feedback on the Rust Internals forum).

@steffahn
Copy link
Member

steffahn commented Feb 4, 2023

Clippy could gain a lint to check that the referenced crates actually exist, have not been yanked and are actually binary crates.

I feel like this might be something that cargo publish should could/should check, at least partially, though I'd be fine to still keep it only as a future possibility.

@not-my-profile
Copy link
Author

Right, I did also think about that. The issue with that is that there's somewhat of a chicken-and-egg problem when first publishing crates since the crates referenced via this new field are likely to depend on the crate containing the field.

@steffahn
Copy link
Member

steffahn commented Feb 4, 2023

But if you have not published a foobar-cli crate yet, publishing foobar library crate with a recommendation for foobar-cli is bad, too. Anyone could publish a crate called foobar-cli at that point, before you publish it yourself. It seems highly advisable to me to go about this in a different order, even if doing so would involve pre-publishing a crate in a dummy state to reserve the name. Either that, or have the initial version of foobar not come with a recommended-bin-crates field. And lastly, it’s always possible to disable checks done locally on your machine by cargo publish (e.g. with --no-verify), if you know what you’re doing and you are willing to risk the race-condition and the possibility to make a mistake.

@ehuss ehuss added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Feb 4, 2023
@clarfonthey
Copy link
Contributor

So, I personally feel like this feature is almost too restrictive for binary crates, and would make more sense as a general recommendation mechanism. For example, serde-json is often used with serde, and the serde crate could vouch for that crate being recommended.

Except this almost approaches the point where it's maybe not a good thing to include in the cargo manifest, since that is pinned to versions and for example, installing an old version of serde would give old recommendations.

Just a few thoughts on this; not super strong on these ideas, just kind of my gut reaction to the RFC.

@not-my-profile
Copy link
Author

The proposed field has to be specific to binary crates so that cargo install can point out these crates when an installation fails because the crate doesn't contain any binaries. Improving that error message is very much the primary goal of this RFC.

I don't really see the need for a more general recommendation mechanism, I think documenting related libraries in the README works well enough, for instance the second link of the serde README is Data formats supported by Serde.

recommended-bin-crates = ["foobar-cli"]
```

Specifying this field for a library-only crate, enables Cargo to print
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when this field is specified in a package with bins?

Copy link
Author

Choose a reason for hiding this comment

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

Good question. I'd say the field is specifically meant for crates without binaries. Specifying it for a crate with binaries wouldn't have any effect unless websites for crates would decide to display the field regardless.

I'll clarify the Guide-level explanation accordingly.

# Future possibilities
[future-possibilities]: #future-possibilities

* crates.io and/or lib.rs could additionally link crates referenced via
Copy link
Contributor

Choose a reason for hiding this comment

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

aside: #3243 could offer another way to link these crates together in the UI

Comment on lines 156 to 157
* Clippy could gain a lint to check that the referenced crates actually
exist, have not been yanked and are actually binary crates.
Copy link
Contributor

Choose a reason for hiding this comment

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

At least as of today, clippy does not lint Cargo.toml. We are cautious of adding warnings to cargo itself as we don't (yet) have lint controls.

Another problem is whatever does the linting would need to update and check the index.

Also, is the "actually exist" check just to move the cargo publish check earlier?

Copy link
Author

Choose a reason for hiding this comment

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

I think you are mistaken about clippy currently not linting Cargo.toml. Clippy has the clippy::cargo category which currently contains 5 lints (you can filter for the Cargo lint group on the Clippy Lints website) ... it's just that the category isn't enabled by default.

You are right about the indexing part ... I can add a respective note to the RFC. Since this is the Future possibilities section, I think it's fine that some things would still need to be figured out.

My reasoning for suggesting such a clippy lint is that the referenced packages may change after the referencing package has been published. E.g. if we publish a lib-only crate with a valid reference and the referenced crate gets yanked after, that wouldn't be picked up until we do a new cargo publish ... so the lint would be particularly helpful for lib-only crates that don't have regular releases.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are some clippy lints but the team seems hesitant about each new cargo clippy lint.

Copy link
Contributor

Choose a reason for hiding this comment

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

As an update, cargo's linting system is moving forward, see rust-lang/cargo#12235

The main question at this time is always-run lints (like rustc) vs on-demand lints (like clippy)

@not-my-profile not-my-profile changed the title recommended-bin-crates field in Cargo.toml recommended-bin-packages field in Cargo.toml Feb 15, 2023
@linyihai
Copy link

linyihai commented Nov 8, 2023

I saw other people using cargo install this way,

cargo install foo-cli
alias dora='foo-cli'
foo --help

so I thought I could add a feature: if a user wants to install a foo binary, cargo can install foo-cli directly and the user can run it as foo

@Nemo157
Copy link
Member

Nemo157 commented Nov 8, 2023

if a user wants to install a foo binary, cargo can install foo-cli directly and the user can run it as foo

The latter is already possible, the binary name does not have to match the package name (a package may also include multiple binaries), either by putting the main at src/bin/foo/main.rs and relying on auto-detection, or manually configuring the name separately:

[package]
name = "foo-cli"

[[bin]]
name = "foo"
path = "src/main.rs"

@epage
Copy link
Contributor

epage commented May 21, 2024

A random way of re-framing this is for Cargo to have an equivalent of the diagnostic namespace.

@epage
Copy link
Contributor

epage commented May 28, 2024

We discussed this in today's Cargo team meeting as part of our RFC triage. We had previously discussed the trade offs between putting everything into a package vs using a workspace. We are overall in favor of improving the workflow for people.

The main concern is over what schema to use (which different designs can lead to minor tweaks to the semantics). We discussed the possibility of a [diagnostics] table (inspired by the recently stabilized diagnostics attribute) and lean towards it after exploring several potential use cases for it. We'd recommend exploring what the experience would be like to create a [diagnostics] table, what kind of fields make sense within that and what lessons we should learn from the diagnostic attribute (CC @estebank for any insight you can provide).

@rfcbot merge

@rfcbot concern schema

@rfcbot
Copy link
Collaborator

rfcbot commented May 28, 2024

Team member @epage 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 May 28, 2024
@epage epage added the S-waiting-on-author Status: This is awaiting some action from the author. label May 28, 2024
@estebank
Copy link
Contributor

@epage (off the cuff) one thing that came up with #[diagnostic] is the need for it to be very permissive without causing errors: whenever you add a feature you would never be able to remove it without pain, and if you add a new feature to one version and people want to use it, you're forcing crate authors to bump their MSRV sorely for better errors. You'll also need to see how you handle these for transitive dependencies, whether crate authors can customize errors not just for people who have added them as direct dependencies, or for everyone that has them as a dependency in their tree.

@epage
Copy link
Contributor

epage commented May 29, 2024

@estebank any lessons on the reporting side? We're wondering whether to expose just a string or go all out like diagnostics or have special built fields so we have semantic information.

@joshtriplett
Copy link
Member

joshtriplett commented Sep 16, 2024

Rather than treating this purely as diagnostic, what if we treated it as directly actionable metadata? If someone tries to install a crate, and it isn't installable because it's a library crate, and it has this field, directly install the mentioned crate instead.

We can decide that exact behavior later, but if we might want that behavior, then it doesn't seem like this belongs under a diagnostic namespace.

@epage
Copy link
Contributor

epage commented Sep 16, 2024

@joshtriplett I'd have to dig through our notes to fully remember why we were leaning towards diagnostics but one concern with this RFC as a whole is that we need to match the behavior with expectations in a way that avoids the expectations that would lead to creating a circular dependency between the library and the bin.

@joshtriplett
Copy link
Member

joshtriplett commented Sep 16, 2024

@epage Not following how this would go circular. It would only lead cargo from attempting to install the lib to actually installing the bin, and installing the bin would likely depend on the lib, but that's not circular. What's the scenario of concern?

(Not looking to retread a discussion that has already taken place, if this already has. I don't recall this aspect of the topic arising, though.)

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. S-waiting-on-author Status: This is awaiting some action from the author. 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.