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

fix: add error message for github PR url in dep #15003

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2144,6 +2144,8 @@ fn to_dependency_source_id<P: ResolveToPath + Clone>(
.unwrap_or(GitReference::DefaultBranch);
let loc = git.into_url()?;

warn_if_github_pull_request(&name_in_toml, &loc, manifest_ctx.warnings);

if let Some(fragment) = loc.fragment() {
let msg = format!(
"URL fragment `#{fragment}` in git URL is ignored for dependency ({name_in_toml}). \
Expand Down Expand Up @@ -2182,6 +2184,30 @@ fn to_dependency_source_id<P: ResolveToPath + Clone>(
}
}

/// Checks if the URL is a GitHub pull request URL.
///
/// If the URL is a GitHub pull request URL, an warning is emitted with a message that explains how
/// to specify a specific git revision.
///
/// At some point in the future it might be worth considering making this a hard error, but for now
/// it's just a warning. See <https://github.com/rust-lang/cargo/pull/15003#discussion_r1908005924>.
fn warn_if_github_pull_request(name_in_toml: &str, url: &Url, warnings: &mut Vec<String>) {
if url.host_str() != Some("github.com") {
return;
}
let path_components = url.path().split('/').collect::<Vec<_>>();
if let ["", owner, repo, "pull", pr_number, ..] = path_components[..] {
let repo_url = format!("https://github.com/{owner}/{repo}.git");
let rev = format!("refs/pull/{pr_number}/head");
let warning = format!(
"dependency ({name_in_toml}) git url {url} is not a repository. \
The path looks like a pull request. Try replacing the dependency with: \
`git = \"{repo_url}\" rev = \"{rev}\"` in the dependency declaration.",
);
warnings.push(warning);
}
}

pub(crate) fn lookup_path_base<'a>(
base: &PathBaseName,
gctx: &GlobalContext,
Expand Down
50 changes: 50 additions & 0 deletions tests/testsuite/bad_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2151,6 +2151,56 @@ Caused by:
.run();
}

#[cargo_test]
fn github_pull_request_url() {
joshka marked this conversation as resolved.
Show resolved Hide resolved
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.0"
edition = "2015"
authors = []

[dependencies.bar]
git = "https://github.com/foo/bar/pull/123"
"#,
)
.file("src/lib.rs", "")
.build();

p.cargo("check -v")
.with_status(101)
.with_stderr_data(str![[r#"
[WARNING] 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.
[UPDATING] git repository `https://github.com/foo/bar/pull/123`
[WARNING] spurious network error (3 tries remaining): unexpected http status code: 404; class=Http (34)
[WARNING] spurious network error (2 tries remaining): unexpected http status code: 404; class=Http (34)
[WARNING] spurious network error (1 tries remaining): unexpected http status code: 404; class=Http (34)
[ERROR] failed to get `bar` as a dependency of package `foo v0.0.0 ([ROOT]/foo)`

Caused by:
failed to load source for dependency `bar`

Caused by:
Unable to update https://github.com/foo/bar/pull/123

Caused by:
failed to clone into: [ROOT]/home/.cargo/git/db/123-[HASH]

Caused by:
network failure seems to have happened
if a proxy or similar is necessary `net.git-fetch-with-cli` may help here
https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli

Caused by:
unexpected http status code: 404; class=Http (34)
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. I thought it was network flakiness. Anyway, you can use ... to omit the unimportant part.

Suggested change
[WARNING] spurious network error (3 tries remaining): unexpected http status code: 404; class=Http (34)
[WARNING] spurious network error (2 tries remaining): unexpected http status code: 404; class=Http (34)
[WARNING] spurious network error (1 tries remaining): unexpected http status code: 404; class=Http (34)
[ERROR] failed to get `bar` as a dependency of package `foo v0.0.0 ([ROOT]/foo)`
Caused by:
failed to load source for dependency `bar`
Caused by:
Unable to update https://github.com/foo/bar/pull/123
Caused by:
failed to clone into: [ROOT]/home/.cargo/git/db/123-[HASH]
Caused by:
network failure seems to have happened
if a proxy or similar is necessary `net.git-fetch-with-cli` may help here
https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli
Caused by:
unexpected http status code: 404; class=Http (34)
...
[ERROR] failed to get `bar` as a dependency of package `foo v0.0.0 ([ROOT]/foo)`
Caused by:
failed to load source for dependency `bar`
Caused by:
Unable to update https://github.com/foo/bar/pull/123
...

Copy link
Author

Choose a reason for hiding this comment

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

This is the problem with not making this change cause an error. If this is a warning, then after emitting the warning, cargo will attempt to load the github PR url, which is not a git repo, and so that attempt will 404 or some other error (e.g. not a git repo). I think that ... change would make sense though, so committing it as is.

PS. feel free to squash this commit (I know Ed prefers atomic commits in the shape that will end up in the repo, but this is really a single change - not sure how that applies to this project).

Copy link
Member

Choose a reason for hiding this comment

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

[patch] test also needs the same trick :)


"#]])
.run();
}

#[cargo_test]
fn fragment_in_git_url() {
let p = project()
Expand Down
55 changes: 55 additions & 0 deletions tests/testsuite/patch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,61 @@ fn patch_to_git() {
.run();
}

#[cargo_test]
fn patch_to_git_pull_request() {
Package::new("bar", "0.1.0").publish();

let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
edition = "2015"
authors = []

[dependencies]
bar = "0.1"

[patch.crates-io]
bar = { git = 'https://github.com/foo/bar/pull/123' }
"#,
)
.file(
"src/lib.rs",
"extern crate bar; pub fn foo() { bar::bar(); }",
)
.build();

p.cargo("check -v")
.with_status(101)
.with_stderr_data(str![[r#"
[WARNING] 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.
[UPDATING] git repository `https://github.com/foo/bar/pull/123`
[WARNING] spurious network error (3 tries remaining): unexpected http status code: 404; class=Http (34)
[WARNING] spurious network error (2 tries remaining): unexpected http status code: 404; class=Http (34)
[WARNING] spurious network error (1 tries remaining): unexpected http status code: 404; class=Http (34)
[ERROR] failed to load source for dependency `bar`

Caused by:
Unable to update https://github.com/foo/bar/pull/123

Caused by:
failed to clone into: [ROOT]/home/.cargo/git/db/123-[HASH]

Caused by:
network failure seems to have happened
if a proxy or similar is necessary `net.git-fetch-with-cli` may help here
https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli

Caused by:
unexpected http status code: 404; class=Http (34)

"#]])
.run();
}

#[cargo_test]
fn unused() {
Package::new("bar", "0.1.0").publish();
Expand Down
Loading