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.

Sign up for GitHub

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

Jump to bottom

Display cargo install crate-name rather than cargo add crate-name for binary crates #5882

Closed
Aloso opened this issue Jan 7, 2023 · 14 comments
Closed

Display cargo install crate-name rather than cargo add crate-name for binary crates #5882

Aloso opened this issue Jan 7, 2023 · 14 comments
Labels
C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works

Comments

@Aloso
Copy link

Aloso commented Jan 7, 2023

Current Behavior

crates.io has an "Install" sections with copy-paste snippets to install the crate. It includes these two snippets:

cargo add crate-name

crate-name = "version"

For binary crates (e.g. ripgrep) this is not useful.

Expected Behavior

For crates that have a binary target, but not a library target, crates.io should show instead:

cargo install crate-name

For crates that have both a binary target and a library target, crates.io should all three:

cargo add crate-name

crate-name = "version"

cargo install crate-name

Steps To Reproduce

  1. Go to https://crates.io/crates/ripgrep
  2. Look at the page

Environment

  • Browser: any
  • OS: irrelevant

Anything else?

No response
@epage
Copy link

epage commented Jan 7, 2023

At the moment, crates.io has no knowledge of whether a crate has a [[bin]] or a [lib] component. You can see what cargo publish tells crates.io here.

Adjusting crates.io's backend to take that information and cargo to report it would be the first step. We'd then need to wait a period of time for publishes to happen to get the new data. In theory, we could tear apart the crates and update the metadata but I suspect this is a more more complex of a case (e.g. different lib and main auto-detection rules) that we would just wait for new publishes. Then the webpage could start sporting the new information.

I feel like there were an issue or two in cargo's backlog where having this information would be helpful but I can't find them at the moment.

@Turbo87 Turbo87 added the C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works label Jan 7, 2023
@epage
Copy link

epage commented Jan 7, 2023

I remembered what cargo's case is.

If we are already putting whether a crate has a [[bin]] and/or a [lib] in the index, it could be useful for us to enumerate what [[bin]]s are present.

In cargo today, if you type cargo foo without the cargo-foo bin being installed, we could check if a cargo-foo crate exists and suggest cargo install cargo-foo. The more advanced solution is if we can walk the entire index, checking for what crate includes the cargo-foo bin, we can then suggest that crate name. For example, the cargo upgrade bin is provided by cargo-edit crate.

See rust-lang/cargo#4682 for more details.

@jonasbb
Copy link

jonasbb commented Jan 7, 2023

[...] it could be useful for us to enumerate what [[bin]]s are present.

That could also be a first step in helping cargo avoid needless reinstalls of crates, since cargo currently gets confused when bin targets are only available conditionally, e.g., rust-lang/cargo#8513 and rust-lang/cargo#8703.

@epage epage mentioned this issue Mar 6, 2023
Open
@epage epage mentioned this issue Apr 6, 2023
Merged
@ccqpein
Copy link

ccqpein commented Feb 13, 2024

I found the same thing on crates.io days ago and I found this issue. I checked the ref issues/PRs linked with this issue and source code. If I understand correct, we need to update db by adding the enum crates types, index it, then change the source code to parsing the [lib] [[bin]] etc., right?

Another rookie question, I code Rust for years but never has chance to contribute to community, so what's the workflow if we want to implement this? Like we give purpose first or RFC? Do we already has this issue's RFC? Thanks.

@epage
Copy link

epage commented Feb 13, 2024

crates.io is using a library to parse Cargo.toml that might or might not support target discovery. Implementing it can be a bit messy because you are operating on a .crate so you need to implement a FS trait. However, they should be switching to cargo-util-schema as that is maintained by the cargo team and used in cargo. That doesn't do target discovery. However, I have been wanting to change cargo to "normalize" the manifests to include all targets to be discovered within the manifest when its published. This gets a bit messy so not sure how good it is for a first issue but willing to help out if you want to go that route. I'm available on zulip and have office hours.

If crates.io team wants to go with the FS trait until then, I leave that to them.

Once its in the manifest, it becomes easier. In the code, you would check the manifest for what it contains and then set a field in the database that you'd add (maybe a "has bin" and "bin names"?). You'd then have the frontend change things based on that. I can't speak to all of the best practices and desires of the crates.io team towards the specifics of this.

@ccqpein
Copy link

ccqpein commented Feb 13, 2024

@epage Thanks!

I have been wanting to change cargo to "normalize" the manifests to include all targets

Is this one? rust-lang/rfcs#3371

@Turbo87
Copy link
Member

Turbo87 commented Feb 13, 2024

the normalized manifests would certainly make things easier in the future. unfortunately they won't help if we want to backfill the information for existing crate files though.

crates.io is using a library to parse Cargo.toml that might or might not support target discovery.

https://github.com/LukeMathWalker/cargo-manifest/blob/v0.12.1/src/lib.rs#L239-L248 is what Ed was talking about. the library that we're currently using supports it, but we're not using that part of the library yet since it would slow down the publish step even more. we could however introduce another background worker job, that downloads the newly published crate file again and does a slightly deeper analysis including the library-or-application check.

one open question about this is how we deal with crate files that have both an application and a library in them. but that can still be decided and changed on the frontend, once we have all the necessary data in the database and exposed on the API.

@epage
Copy link

epage commented Feb 13, 2024

Is this one? rust-lang/rfcs#3371

That is just about target/. What I'm talking about doesn't need an RFC but it does touch a lot of places in cargo (I have a branch where I started on it).

@epage epage mentioned this issue Apr 10, 2024
Merged
bors added a commit to rust-lang/cargo that referenced this issue Apr 29, 2024
@bors
Auto merge of #13713 - epage:no-auto, r=weihanglo
ba6fb40 
fix(toml): Warn, rather than fail publish, if a target is excluded

### What does this PR try to resolve?

We have a couple of problems with publishing
- Inconsistent errors: if a target that `package` doesn't verify is missing `path`, it will error, while one with `path` won't, see #13456
- Users may want to exclude targets and their choices are
  - Go ahead and include them.  I originally excluded my examples before doc-scraping was a think.  The problem was if I had to set `required-features`, I then could no longer exclude them
  - Muck with `Cargo.toml` during publish and pass `--allow-dirty`

This fixes both by auto-stripping targets on publish.  We will warn the user that we did so.

This is a mostly-one-way door on behavior because we are turning an error case into a warning.
For the most part, I think this is the right thing to do.
My biggest regret is that the warning is only during `package`/`publish` as it will be too late to act on it and people who want to know will want to know when the problem is introduced.
The error is also very late in the process but at least its before a non-reversible action has been taken.
Dry-run and `yank` help.

Fixes #13456
Fixes #5806

### How should we test and review this PR?

Tests are added in the first commit and you can then follow the commits to see how the test output evolved.

The biggest risk factors for this change are
- If the target-stripping logic mis-identifies a path as excluded because of innocuous path differences (e.g. case)
- Setting a minimum MSRV for published packages: `auto*` were added in 1.27 (#5335) but were insta-stable.  `autobins = false` did nothing until 1.32 (#6329).  I have not checked to see how this behaves pre-1.32  or pre-1.27.  Since my memory of that error is vague, I believe it will either do a redundant discovery *or* it will implicitly skip discovery

Resolved risks
- #13729 ensured our generated target paths don't have `\` in them
- #13729 ensures the paths are normalize so the list of packaged paths

For case-insensitive filesystems, I added tests to show the original behavior (works locally but will fail when depended on from a case-sensitive filesystem) and tracked how that changed with this PR (on publish warn that those targets are stripped).  We could try to normalize the case but it will also follow symlinks and is likely indicative of larger casing problems that the user had.  Weighing how broken things are now , it didn't seem changing behavior on this would be too big of a deal.

We should do a Call for Testing when this hits nightly to have people to `cargo package` and look for targets exclusion warnings that don't make sense.

### Additional information

This builds on #13701 and the work before it.

By enumerating all targets in `Cargo.toml`, it makes it so rust-lang/crates.io#5882 and rust-lang/crates.io#814 can be implemented without any other filesystem interactions.

A follow up PR is need to make much of a difference in performance because we unconditionally walk the file system just in case `autodiscover != Some(false)` or a target is missing a `path`.

We cannot turn off auto-discovery of libs, so that will always be done for bin-only packages.
This was referenced Jun 10, 2024
Closed
Merged
Merged
@finnbear
Copy link

finnbear commented Jul 12, 2024 •
edited
Loading

I have a library crate with a few binaries for my own use during development. They are not for end users of the crate. Is there a way to disable the cargo install instructions? (or, preferably, a way to prevent cargo install from doing anything?)

image

@epage
Copy link

epage commented Jul 12, 2024

Could you exclude them from being published? Or move them to another package in the workspace?

@finnbear
Copy link

Could you exclude them from being published?

Thanks for the suggestion, but I thought exclude was for excluding files not disabling binaries. The binaries are mentioned in my Cargo.toml file, and I don't know how cargo or crates.io will react if binary files are simply missing.

Or move them to another package in the workspace?

Yes, as a last resort :)

@epage
Copy link

epage commented Jul 12, 2024

Thanks for the suggestion, but I thought exclude was for excluding files not disabling binaries. The binaries are mentioned in my Cargo.toml file, and I don't know how cargo or crates.io will react if binary files are simply missing.

If you wait a couple of weeks, Cargo 1.80 will strip [[bin]] entries that are excluded, see rust-lang/cargo#13713

@finnbear
Copy link

Thanks, that solves my problem! ❤️

@Turbo87
Copy link
Member

Turbo87 commented Jul 17, 2024 •
edited
Loading

the has_lib and bin_names columns in the database have been backfilled today (see https://rust-lang.zulipchat.com/#narrow/stream/318791-t-crates-io/topic/lib.2Fbin.20detection). https://crates.io/crates/ripgrep finally shows cargo install now, so I guess we can finally close this issue as resolved 🎉

@Turbo87 Turbo87 closed this as completed Jul 17, 2024
@ia0 ia0 mentioned this issue Aug 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@epage @Turbo87 @jonasbb @ccqpein @Aloso @finnbear