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

Suggest cargo install url using --git #10485

Closed
pickfire opened this issue Mar 18, 2022 · 10 comments · Fixed by #12575
Closed

Suggest cargo install url using --git #10485

pickfire opened this issue Mar 18, 2022 · 10 comments · Fixed by #12575
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-install E-medium Experience: Medium S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@pickfire
Copy link
Contributor

Problem

Currently, someone may try cargo install https://github.com/light4/cargo-info, then I see.

    Updating crates.io index
error: could not find `https://github.com/light4/cargo-info` in registry `crates-io` with version `*`

Proposed Solution

It would be nice to see a suggestion to ask the user to run the same command with --git.

Notes

No response

@pickfire pickfire added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Mar 18, 2022
@weihanglo weihanglo added A-diagnostics Area: Error and warning messages generated by Cargo itself. Command-install labels Mar 19, 2022
petr-tik added a commit to petr-tik/cargo that referenced this issue Apr 1, 2022
Heuristically identify git URL by matching the pattern
"git" in the domain.

Add a special case error with a fixit hint to help users
rerun the correct cargo install command
@petr-tik
Copy link

petr-tik commented Apr 12, 2022

As suggested here
#10522 (comment)

i would like to get some input on a heuristic that I can use to tell if a command line argument is a git-repo in disguise. This is the comment in question.

b87ae28#diff-962037da03b1549f2185f7de90ea6bf84aabfe566f3033a4d983955cb21b6bf5R527-R534

Is matching "git" in the domain enough? What about "bitbucket"? Any other providers? Can this be dynamically populated from cargo config with another repository name?

@pickfire can you please provide some input as the original reporter

@epage
Copy link
Contributor

epage commented Apr 12, 2022

I'm trying to think of what a user might type in, valid or not,

Ideas

  • We can check if the path ends in .git
  • We can check for git@
  • Its probably safe to assume that if its a URL that is more than a path, then its git because the likelihood of a user entering a different URL is low. If we add support for another VCS or URL use, we could expand it at that point.

@pickfire
Copy link
Contributor Author

pickfire commented Apr 13, 2022

Is matching "git" in the domain enough? What about "bitbucket"? Any other providers? Can this be dynamically populated from cargo config with another repository name?

I think string matching probably won't work here given that alias can be done, I see two ways:

  • try git clone without cloning
  • just suggest it when the command fail

We can check if the path ends in .git

Not a good idea as git can work without that.

We can check for git@

That limits the scope greatly given that ssh:// and https:// (and alias) works.

What I think is when a path is invalid, maybe we can first probe git to try and clone it (given that you can use alias in git like gh: to https://github.com/), so I think the best way is to try clone (or search from a valid alias in github) without really cloning on failure to find it then suggest user based on that?

But most importantly we only suggest this when the input is invalid, so it's fine even though the suggestion may be incorrect, so we don't really have to git clone to probe it, can just suggest it when the crate name didn't work out.

@epage
Copy link
Contributor

epage commented Apr 13, 2022

We can check if the path ends in .git

Not a good idea as git can work without that.

We can check for git@

That limits the scope greatly given that ssh:// and https:// (and alias) works.

Note that my ideas for checking .git or git@ were not meant to be mutually exclusive with anything else but clues to help give the best hint. These ideas are also predicated on the fact that most people are unlikely to hand-write a git url but copy from something.

What I think is when a path is invalid, maybe we can first probe git to try and clone it (given that you can use alias in git like gh: to https://github.com/), so I think the best way is to try clone (or search from a valid alias in github) without really cloning on failure to find it then suggest user based on that?

We probably should not make any network connections to determine what hint we should give.

But most importantly we only suggest this when the input is invalid, so it's fine even though the suggestion may be incorrect, so we don't really have to git clone to probe it, can just suggest it when the crate name didn't work out.

As I mentioned, there are other cases we should differentiate with like for paths. That is why I was suggesting the URL approach so we can differentiate between an identifier, a path, and a url and suggest --git on urls.

@petr-tik
Copy link

Invalid: git url, including

* https://github.com/rust-lang/cargo.git

* [[email protected]](mailto:[email protected]):rust-lang/cargo.git

* https://github.com/rust-lang/cargo

* https://bitbucket.org/jcmoyer/rust-tictactoe/src/master/

* https://bitbucket.org/jcmoyer/rust-tictactoe.git

* [[email protected]](mailto:[email protected]):jcmoyer/rust-tictactoe.git

What about using this crate
https://crates.io/crates/git-url-parse
and specifically the parse method to verify if the invalid argument to install is actually a git url
https://docs.rs/git-url-parse/0.4.0/git_url_parse/struct.GitUrl.html#method.parse

then our hint can use the GitUrl struct to generate the appropriate suggestion?

@petr-tik
Copy link

GitUrl manages to successfully parse all 5 URLs that you gave, but installing from git@ urls fails.

cargo install --git [email protected]:rust-lang/cargo.git
error: invalid url `[email protected]:rust-lang/cargo.git`: relative URL without a base

Replacing Url with GitUrl in cargo feels like a bigger task than this ticket and current bandwidth allows. The question is - would you be amenable to using GitUrl only to verify that the cmdline argument is indeed a valid git URL and generate a suggestion that uses https for cargo install?

@petr-tik
Copy link

What do you think about this prototype? It uses GitUrl and then normalises git@ arguments into http-based git URLs that cargo currently supports.

https://github.com/rust-lang/cargo/pull/10522/files/e045b4f94dbf6ef25529d0dbfa974cebc65bc1b1..ef7ee2c7ee9e1373ec66f04460dca1db1da7f828

@pickfire
Copy link
Contributor Author

What do you think about this prototype? It uses GitUrl and then normalises git@ arguments into http-based git URLs that cargo currently supports.

Not sure if we want to normalize it but I think it's better to show the suggestion with the url that user was given rather than the normalized url, then the user might be surprised where did that url come from, if it's from their input then it should not be surprising.

@petr-tik
Copy link

Not sure if we want to normalize it but I think it's better to show the suggestion with the url that user was given rather than the normalized url, then the user might be surprised where did that url come from, if it's from their input then it should not be surprising.

if we leave the URL supplied by the user, the hint will suggest another failing command

cargo install [email protected]:jcmoyer/rust-tictactoe.git
    Updating crates.io index
error: could not find `[email protected]:jcmoyer/rust-tictactoe.git` in registry `crates-io` with version `*` # imagine this suggested add `install --git [email protected]:jcmoyer/rust-tictactoe.git

cargo install --git [email protected]:jcmoyer/rust-tictactoe.git
error: invalid url `[email protected]:jcmoyer/rust-tictactoe.git`: relative URL without a base

you are right about the principle of least surprise and I want to follow it, but I don't want to make a suggestion that also errors.

what if I restrict the hint suggestion to only return something for http(s) git URLs?

@pickfire
Copy link
Contributor Author

what if I restrict the hint suggestion to only return something for http(s) git URLs?

That works too since most git url behave like that, then we can just ignore those without for now.

@weihanglo weihanglo added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. E-medium Experience: Medium labels May 24, 2023
bors added a commit that referenced this issue Aug 29, 2023
cargo install: suggest --git when package name is url

### What does this PR try to resolve?

Improve the error message when specifying a URL for a package name in `cargo install`.

Fixes #10485

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

Just cargo test and trying a common case like `cargo install https://github.com/rust-lang/cargo`

### Additional information

I found this PR after finishing this one: #10522
But it seems have a larger scope to refactor some of the related code.

Perhaps this one would be easier to merge and that one could focus on the refactor, otherwise sorry for the noise and feel free to close.
@bors bors closed this as completed in 333ca23 Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-install E-medium Experience: Medium S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
4 participants