Suggest cargo install --git when missing registry package looks like a git* URL #10522

petr-tik · 2022-03-30T19:20:52Z

What does this PR try to resolve?

Create a local Error type that that wraps existing error handling code

Giving the three error variants names improves readability and
makes it easier to share duplicate code

Move all string operations to create errors from 3 separate bail!
calls into anyhow::anyhow! calls that are encapsulated in the
From impl

How should we test and review this PR?

I left 2 comments asking for REVIEW
Please suggest other failure scenarios that SelectPackageError may have missed or too eagerly
bundled in the CouldntFindInRegistry variant.

I thought that adding error handling code is something we might be happy to pay a potential perf
penalty for if we can improve error messages, but please correct me if I am wrong

Additional information

I worked on this by adding a failing test case and implementing until it passed. Then I ran all install tests
cargo test --package cargo --test testsuite -- install --nocapture

rust-highfive · 2022-03-30T19:20:54Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

src/cargo/ops/common_for_install_and_uninstall.rs

epage · 2022-03-30T19:53:07Z

src/cargo/ops/common_for_install_and_uninstall.rs

+    version_req: OptVersionReq,
+}
+
+enum SelectPackageError {


How come SelectPackageError was created rather than just using bail?

Personally, I found the if/else branches using bail hard to read and repetitive.

I performed the refactor to familiarise myself with the codebase.

after I finished it, adding an enum variant converting it to an anyhow::Error was quite easy to make the test pass

Since this error enum and its conversion to anyhow::Error is private to this module, I thought the benefits of defining new readable types would outweigh the PR review costs

Unless I'm missing something, this looks like it overcomplicates the code. I'm not seeing us getting any type-safety, programmatic error case checking, or code reuse benefits. We've replaced simple one-line bail's with 3 types and a From impl.

I thought the benefits of defining new readable types would outweigh the PR review costs

Please keep these things separate. If nothing else, at least separate commits.

Unless I'm missing something, this looks like it overcomplicates the code. I'm not seeing us getting any type-safety, programmatic error case checking, or code reuse benefits. We've replaced simple one-line bail's with 3 types and a From impl.

I can separate the logic that fixes the test into 1 commit and my final refactor into another. Then you can review them separately and if you want only the test fix, I will revert the refactor commit.

Please keep these things separate. If nothing else, at least separate commits.

will do

Then you can review them separately and if you want only the test fix, I will revert the refactor commit.

While I can see value to splitting out the error analysis into a separate function, I feel the more specific error types are boiletplate without benefits to justify it.

Thanks for the input. I will revert the refactoring commit and continue working on the smaller PR surface.

epage · 2022-03-30T19:56:52Z

src/cargo/ops/common_for_install_and_uninstall.rs

+fn is_package_name_a_git_url(package_name: &InternedString) -> bool {
+    if let Ok(url) = Url::parse(package_name) {
+        if let Some(domain) = url.domain() {
+            // REVIEW
+            // Are there any other git services without "git"
+            // in the domain?
+            // bitbucket?
+            // Is it possible to ask the cargo/crates team for
+            // some stats where crates projects are currently hosted on
+            return domain.contains("git");
        }
-        None => {
-            let is_yanked: bool = if dep.version_req().is_exact() {
-                let version: String = dep.version_req().to_string();
-                PackageId::new(dep.package_name(), &version[1..], source.source_id())
-                    .map_or(false, |pkg_id| source.is_yanked(pkg_id).unwrap_or(false))
-            } else {
-                false
-            };
-            if is_yanked {
-                bail!(
-                    "cannot install package `{}`, it has been yanked from {}",
-                    dep.package_name(),
-                    source.source_id()
-                )
+    }
+    false
+}


Not sure where we have this in the contributing guide but we're preferring these design conversations to happen in the issue before getting to the PR stage

Thanks for reviewing so swiftly.

Appreciate this refactor breaches into discuss-first territory and I would have started the conversation earlier, if I had such a clear plan before starting to implement this.

My workflow was:
add a failing test case (very easy thanks to the clear ticket)
find the function select_dep_pkg that throws the error
try to understand the two bails at the bottom and where my addition would fit
refactor into an enum that tries to classify failure scenarios
realise that my failure scenario requires the same info as the SelectPackageError::CouldntFindInRegistry variant
Implement that and get the test to pass

After all the tests passed, I decided to get your thoughts on the implementation. If this refactor is considered too high risk for now, I should now be able to fold my error into an else branch with a bail! macro like the existing handlers.

Let me know

I think the rendering of the diff might have obscured what I was referring to. I was referring to the REVIEW comment, not the refactors. Just a simple question on this in the Issue before getting to the PR stage would be preferred.

Personally, I also start a conversation on these things to help draw attention to an area of conversation to make it easier on reviewers.

Please start a conversation in the issue about detecting cases for suggesting --git

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

Create a local Error type that that wraps existing error handling code Giving the three error variants names improves readability and makes it easier to recognise duplicate code Move all string operations to create errors from 3 separate bail! calls into anyhow::anyhow! calls that are encapsulated in the From<> impl

petr-tik · 2022-04-01T22:08:20Z

Thanks for your review. I have now restructured the PR to consist of 2 commits:
b87ae28
with the regression test and the implementation that makes it pass without refactoring into new errors types.

The original REVIEW comment is still there for anyone to suggest improvement on.

174c80e
Is the refactor commit that adds the local error type and refactors all error handling there.

I hope this makes it easier to review.

I am happy to revert the PR back to the first commit that implements the actionable fixit with minimal refactoring.

Let me know.

Last but not least, congrats on soon becoming the new maintainer of cargo - it's a great tool and I am happy to see such diligent stewardship.

epage · 2022-04-12T14:04:06Z

src/cargo/ops/common_for_install_and_uninstall.rs

-                )
+                if was_git_url_miscategorised_as_a_registry_dep(&dep) {
+                    bail!(
+                        "could not find `{}` in {} with version `{}`. Try adding `--git {}`",


While I don't think I've seen cargo strictly follow rustc's error conventions, I think it'd be helpful for us to consider them with our error messages. See https://rustc-dev-guide.rust-lang.org/diagnostics.html#suggestion-style-guide.

While this isn't quite a question, it still falls under the "be more explicit" part of questions where it says

Sometimes, it's unclear why a particular suggestion is being made

Maybe the "further instruction" section can also provide ideas on how to work this.

The message may contain further instruction such as "to do xyz, use" or "to do xyz, use abc".

Thanks for pointing this out. I will rewrite the error message to follow rustc conventions for the suggestion along the lines of

To install a package from a git repository, use --git {}

I think the original first part of the error message is a) good enough b) familiar to users by now, so will only rewrite the suggestion

epage · 2022-04-12T14:07:34Z

src/cargo/ops/common_for_install_and_uninstall.rs

+    version_req: OptVersionReq,
+}
+
+enum SelectPackageError {


Then you can review them separately and if you want only the test fix, I will revert the refactor commit.

While I can see value to splitting out the error analysis into a separate function, I feel the more specific error types are boiletplate without benefits to justify it.

This reverts commit 174c80e.

…om_git_error_message_alternative_impl

Add a test case for a git@ repo and refactor the method to return Option<String> instead of bool. Reword the suggestion to follow rustc guidelines

Make GitUrl parsing and normalisation defensive. Create a type for InstallSuggestion and pass it to the anyhow error message

bors · 2022-10-17T21:09:42Z

☔ The latest upstream changes (presumably #11243) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss · 2023-08-15T19:36:58Z

Unfortunately I think this PR has fallen through the cracks (over a year ago 😞), and I'm not sure what the status of it is. Does anyone have an update on where it stands?

petr-tik · 2023-08-15T19:57:58Z

Unfortunately I think this PR has fallen through the cracks (over a year ago 😞), and I'm not sure what the status of it is. Does anyone have an update on where it stands?

Let me see if I can resolve the merge conflicts and I would appreciate a review

…om_git_error_message_alternative_impl

petr-tik · 2023-08-20T20:12:45Z

update: missed one of the failing tests. Will work to fix that

Since this PR was originally opened, gitoxide started getting integrated into cargo #11813, but I am not sure if you want new PRs to use gitoxide already.

Using gix_url, I can minimise the risk of adding a completely new dependency for a Quality-of-Life improvement like an actionable error message.
https://docs.rs/gix-url/0.21.1/gix_url/

Please clarify if this is something you would like me to do in this PR or you prefer to keep the current implementation and port it to gix_url when that is more stable

weihanglo · 2023-08-20T21:19:07Z

Using gix_url, I can minimise the risk of adding a completely new dependency for a Quality-of-Life improvement like an actionable error message.

Sounds like a good idea! Not sure if their feature parity is at the same level. For reviewer we have to do the same amount of work for either — auditing the dependency as much as possible. If it is something from @Byron I am more confident to rubber-stamp it (just kidding, we still need to audit it).

Thanks for bringing this up and welcome back!

Byron · 2023-08-21T05:57:00Z

Thanks for chiming me in. I recommend using gix-url for the reason that it's already part of the dependency tree, and it does't use regex and tracing. It's not 100% spec compatible yet but that is in the making. I hope that helps.

Keep the dependency try the same by leveraging already imported gix crate to try parsing a git url from a string

petr-tik · 2023-08-21T22:26:15Z

Thanks for chiming me in. I recommend using gix-url for the reason that it's already part of the dependency tree, and it does't use regex and tracing. It's not 100% spec compatible yet but that is in the making. I hope that helps.

I moved to use gix::url instead of git-url-parse and the diff is now smaller!

The previously passing test passes. The test that was failing is still failing.

I will leave a comment with the root cause of the failing test to share context and would appreciate your input

petr-tik · 2023-08-21T22:39:58Z

src/cargo/ops/common_for_install_and_uninstall.rs

+                Scheme::Ssh | Scheme::Git => {
+                    if let (Some(host), Some(owner)) = (git_url.host(), git_url.user()) {
+                        let https_git_url = format!(
+                            "https://{host}/{owner}/{repo_name}",
+                            // repo_name = git_url.name()
+                            repo_name = "TODO"


the reason this repo_name = "TODO" doesn't cause test_install_from_https_git_generate_suggestion to fail is the same reason why test_install_from_git_generate_suggestion is now failing with

error: the --version provided, bitbucket.org:jcmoyer/rust-tictactoe.git, is not a valid semver version

Since I opened the PR, cargo started supporting crate@version command-line arguments
2806270
and now the test input [email protected]:jcmoyer/rust-tictactoe.git is parsed as a version string (and fails with the error in the CI logs).

the code path I added only deals with strings that weren't erroneously parsed as version strings, so the section I highlighted above won't be traversed, because git@ strings now fail to parse as version strings before they get here.

I propose removing this chunk of code and removing the failing test to focus on supporting actionable errors for https git URLs only.

Please let me know what you think

actioned here
f08e95b

Since I opened the PR, cargo started supporting `crate@version` command-line inputs rust-lang@2806270 and now the test input `[email protected]:jcmoyer/rust-tictactoe.git` is parsed as a version string, which fails the code path I added only deals with strings that weren't erroneously parsed as version strings, so the section I highlighted above won't be traversed, because `git@` strings now fail to parse as version strings before they get here. Delete the failing test

niklaswimmer · 2023-08-23T16:24:33Z

Just wanted to note that since this PR now only cares about http/s URLs, you could also use url::Url::parse directly. The additional logic present in gitoxide does not provide any benefit for this particular case.

gix-url (especially after the rewrite) would be helpful if you want to additionally detect scp like target urls and local file paths.

weihanglo · 2023-08-23T18:56:20Z

@niklaswimmer is mostly right. Thanks for calling out that.

It's my fault not looking at this pull request closer. Yes Cargo doesn't support SCP-like URL for Git dependencies, so url crates seems pretty sufficient for this enhancement at this moment.Sorry for giving a wrong pointer.

We may also want to include ssh:// protocol for this check, as Cargo supports that.

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.

arlosi · 2023-08-29T18:15:33Z

Thank you @petr-tik for your work on making this error message better. I merged another PR (#12575) which implements a similar hint for suggesting --git when a URL is passed.

Apologies for the confusion of having two different PRs fixing the same issue. I'm glad to see this error improved.

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.

rust-highfive assigned ehuss Mar 30, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2022

petr-tik force-pushed the n10485_install_from_git_error_message_alternative_impl branch from 5de32a0 to eb9b6a6 Compare March 30, 2022 19:45

epage reviewed Mar 30, 2022

View reviewed changes

src/cargo/ops/common_for_install_and_uninstall.rs Outdated Show resolved Hide resolved

epage reviewed Mar 30, 2022

View reviewed changes

petr-tik added 2 commits April 1, 2022 22:54

Fix rust-lang#10485

b87ae28

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 force-pushed the n10485_install_from_git_error_message_alternative_impl branch from eb9b6a6 to 174c80e Compare April 1, 2022 22:02

petr-tik requested a review from epage April 12, 2022 11:03

epage reviewed Apr 12, 2022

View reviewed changes

petr-tik mentioned this pull request Apr 12, 2022

Suggest cargo install url using --git #10485

Closed

petr-tik added 4 commits April 12, 2022 18:10

Revert "Refactor select_dep_pkg error handling"

869973f

This reverts commit 174c80e.

Merge remote-tracking branch 'upstream/master' into n10485_install_fr…

e045b4f

…om_git_error_message_alternative_impl

Experiment with GitUrl to generate the suggestion

1c9c53b

Add a test case for a git@ repo and refactor the method to return Option<String> instead of bool. Reword the suggestion to follow rustc guidelines

Fix the tests

ef7ee2c

Make GitUrl parsing and normalisation defensive. Create a type for InstallSuggestion and pass it to the anyhow error message

petr-tik requested a review from epage May 7, 2022 10:36

petr-tik added 2 commits August 20, 2023 20:52

Merge remote-tracking branch 'upstream/master' into n10485_install_fr…

e018509

…om_git_error_message_alternative_impl

Fix the build and update Cargo.lock

283c6e8

Replace git-url-parse with gix::url::parse

ddea258

Keep the dependency try the same by leveraging already imported gix crate to try parsing a git url from a string

petr-tik commented Aug 21, 2023

View reviewed changes

weihanglo assigned weihanglo and unassigned ehuss Aug 22, 2023

cardoso mentioned this pull request Aug 28, 2023

cargo install: suggest --git when package name is url #12575

Merged

arlosi closed this Aug 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest cargo install --git when missing registry package looks like a git* URL #10522

Suggest cargo install --git when missing registry package looks like a git* URL #10522

petr-tik commented Mar 30, 2022 •

edited

Loading

rust-highfive commented Mar 30, 2022

epage Mar 30, 2022

petr-tik Mar 30, 2022

petr-tik Mar 30, 2022

epage Apr 1, 2022

petr-tik Apr 1, 2022

epage Apr 12, 2022

petr-tik Apr 12, 2022

epage Mar 30, 2022

petr-tik Mar 30, 2022

epage Apr 1, 2022

epage Apr 12, 2022

petr-tik commented Apr 1, 2022

epage Apr 12, 2022

petr-tik Apr 12, 2022

epage Apr 12, 2022

bors commented Oct 17, 2022

ehuss commented Aug 15, 2023

petr-tik commented Aug 15, 2023

petr-tik commented Aug 20, 2023 •

edited

Loading

weihanglo commented Aug 20, 2023 •

edited

Loading

Byron commented Aug 21, 2023

petr-tik commented Aug 21, 2023

petr-tik Aug 21, 2023 •

edited

Loading

petr-tik Aug 21, 2023

niklaswimmer commented Aug 23, 2023 •

edited

Loading

weihanglo commented Aug 23, 2023

arlosi commented Aug 29, 2023

Suggest cargo install --git when missing registry package looks like a git* URL #10522

Suggest cargo install --git when missing registry package looks like a git* URL #10522

Conversation

petr-tik commented Mar 30, 2022 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rust-highfive commented Mar 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petr-tik commented Apr 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Oct 17, 2022

ehuss commented Aug 15, 2023

petr-tik commented Aug 15, 2023

petr-tik commented Aug 20, 2023 • edited Loading

weihanglo commented Aug 20, 2023 • edited Loading

Byron commented Aug 21, 2023

petr-tik commented Aug 21, 2023

petr-tik Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklaswimmer commented Aug 23, 2023 • edited Loading

weihanglo commented Aug 23, 2023

arlosi commented Aug 29, 2023

petr-tik commented Mar 30, 2022 •

edited

Loading

petr-tik commented Aug 20, 2023 •

edited

Loading

weihanglo commented Aug 20, 2023 •

edited

Loading

petr-tik Aug 21, 2023 •

edited

Loading

niklaswimmer commented Aug 23, 2023 •

edited

Loading