fix: add error message for github PR url in dep #15003
Conversation
Prior to this, using a github PR URL would cause cargo to attempt to
fetch from an incorrect URL several times before failing.
Providing a github pull request url now fails with an error message
that shows how to fix the problem.
E.g.:
```toml
bar = { git = "https://github.com/foo/bar/pull/123" }
```
Now gives the following error message:
```
dependency (bar) specifies a GitHub pull request link. If you were trying to specify a specific github PR, replace the URL with the git URL (e.g. `git = "https://github.com/foo/bar.git"`) and add `rev = "refs/pull/123/head"` in the dependency declaration.
```
Fixes: rust-lang#15001
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
A-manifest
| @@ -2151,6 +2151,37 @@ Caused by: | |||
| .run(); | |||
| } | |||
|
|
|||
| #[cargo_test] | |||
| fn github_pull_request_url() { | |||
There was a problem hiding this comment.
As noted in #14941, this should cover patches as well and we should have a test for it.
| @@ -2144,6 +2144,8 @@ fn to_dependency_source_id<P: ResolveToPath + Clone>( | |||
| .unwrap_or(GitReference::DefaultBranch); | |||
| let loc = git.into_url()?; | |||
|
|
|||
| bail_if_github_pull_request(&name_in_toml, &loc)?; | |||
There was a problem hiding this comment.
I'm a bit uncomfortable proactively erroring if the URL pattern matches to github, rather than providing context if what the user specified fails. I guess the worst that can happen is github allows you to clone PR URLs and we people report it and we have to remove it to allow its use. Not the worst outcome for something that is unclear if it would ever happen.
There was a problem hiding this comment.
The behavior here is the same as for how fragments are handled directly below this (though they are handled as a warning. The difference is that this causes an error later in the processing while the fragment does not. I think it's reasonable to avoid attempting to do cause an error by downloading. If this was wrong, then removing this code or changing it to a warning would be fairly low cost. How about we keep this as-is (in this PR)
tests/testsuite/bad_config.rs
Outdated
| [ERROR] failed to parse manifest at `[ROOT]/foo/Cargo.toml` | ||
|
|
||
| Caused by: | ||
| dependency (bar) specifies a GitHub pull request link. If you were trying to specify a specific github PR, replace the URL with the git URL (e.g. `git = "https://github.com/foo/bar.git"`) and add `rev = "refs/pull/123/head"` in the dependency declaration. |
There was a problem hiding this comment.
As an error message, it should first saw what the problem is. I also feel like this requires people to parse a lot of the message to understand the fix and apply it (e.g. rather than copy/pasting)
What do you think of
`dependencies.{name}.git = "{url}"` is not a repository.
hint: The repository path looks like a pull request, try replacing the repository path with:
`git = "{url}", rev = "{rev}"`
There was a problem hiding this comment.
The first part is for consistency with the other error messages. How about:
dependency (bar) git url https://github.com/foo/bar/pull/123 is not a repository.
The path looks like a pull request.
Try replacing the dependency with: `git = "https://github.com/foo/bar.git" rev = "refs/pull/123/head"` in the dependency declaration.
Prior to this, using a github PR URL would cause cargo to attempt to
fetch from an incorrect URL several times before failing.
Providing a github pull request url now fails with an error message
that shows how to fix the problem.
E.g.:
Now gives the following error message:
Fixes: #15001