From 1a7720c712a03a6c2efda0c7318c6abf680040c0 Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Wed, 1 Jan 2025 21:44:24 -0800 Subject: [PATCH 1/4] fix: add error message for github PR url in dep 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: #15001 --- src/cargo/util/toml/mod.rs | 22 ++++++++++++++++++++++ tests/testsuite/bad_config.rs | 31 +++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index bc80097b705..1f0b7816379 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -2144,6 +2144,8 @@ fn to_dependency_source_id( .unwrap_or(GitReference::DefaultBranch); let loc = git.into_url()?; + bail_if_github_pull_request(&name_in_toml, &loc)?; + if let Some(fragment) = loc.fragment() { let msg = format!( "URL fragment `#{fragment}` in git URL is ignored for dependency ({name_in_toml}). \ @@ -2182,6 +2184,26 @@ fn to_dependency_source_id( } } +/// Checks if the URL is a GitHub pull request URL. +/// +/// If the URL is a GitHub pull request URL, an error is returned with a message that explains +/// how to specify a specific git revision. +fn bail_if_github_pull_request(name_in_toml: &str, url: &Url) -> CargoResult<()> { + if url.host_str() != Some("github.com") { + return Ok(()); + } + let path_components = url.path().split('/').collect::>(); + if let ["", owner, repo, "pull", pr_number, ..] = path_components[..] { + bail!( + "dependency ({name_in_toml}) 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/{owner}/{repo}.git\"`) \ + and add `rev = \"refs/pull/{pr_number}/head\"` in the dependency declaration.", + ); + } + Ok(()) +} + pub(crate) fn lookup_path_base<'a>( base: &PathBaseName, gctx: &GlobalContext, diff --git a/tests/testsuite/bad_config.rs b/tests/testsuite/bad_config.rs index 0fb00f8e8fa..5759e826ada 100644 --- a/tests/testsuite/bad_config.rs +++ b/tests/testsuite/bad_config.rs @@ -2151,6 +2151,37 @@ Caused by: .run(); } +#[cargo_test] +fn github_pull_request_url() { + 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#" +[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. + +"#]]) + .run(); +} + #[cargo_test] fn fragment_in_git_url() { let p = project() From d743987ca49149bfd8c48b48eabea641ddef34ea Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Thu, 2 Jan 2025 16:45:35 -0800 Subject: [PATCH 2/4] fix: tweak message and add test for patch --- src/cargo/util/toml/mod.rs | 10 ++++---- tests/testsuite/bad_config.rs | 2 +- tests/testsuite/patch.rs | 44 +++++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 5 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 1f0b7816379..ea6a088cdf4 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -4,6 +4,7 @@ use std::ffi::OsStr; use std::path::{Path, PathBuf}; use std::rc::Rc; use std::str::{self, FromStr}; +use tracing_subscriber::fmt::format; use crate::core::summary::MissingDependencyError; use crate::AlreadyPrintedError; @@ -2194,11 +2195,12 @@ fn bail_if_github_pull_request(name_in_toml: &str, url: &Url) -> CargoResult<()> } let path_components = url.path().split('/').collect::>(); 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"); bail!( - "dependency ({name_in_toml}) 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/{owner}/{repo}.git\"`) \ - and add `rev = \"refs/pull/{pr_number}/head\"` in the dependency declaration.", + "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.", ); } Ok(()) diff --git a/tests/testsuite/bad_config.rs b/tests/testsuite/bad_config.rs index 5759e826ada..80f69e543a9 100644 --- a/tests/testsuite/bad_config.rs +++ b/tests/testsuite/bad_config.rs @@ -2176,7 +2176,7 @@ fn github_pull_request_url() { [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. + 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. "#]]) .run(); diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs index ff46b324091..99488e1f38c 100644 --- a/tests/testsuite/patch.rs +++ b/tests/testsuite/patch.rs @@ -357,6 +357,50 @@ fn patch_to_git() { .run(); } +#[cargo_test] +fn patch_to_git_pull_request() { + let bar = git::repo(&paths::root().join("override")) + .file("Cargo.toml", &basic_manifest("bar", "0.1.0")) + .file("src/lib.rs", "pub fn bar() {}") + .build(); + + 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#" +[ERROR] failed to parse manifest at `[ROOT]/foo/Cargo.toml` + +Caused by: + 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. + +"#]]) + .run(); +} + #[cargo_test] fn unused() { Package::new("bar", "0.1.0").publish(); From 85c93879f61b467e8edb51fd0801df915a19746e Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Wed, 8 Jan 2025 21:57:13 -0800 Subject: [PATCH 3/4] Replace error with warning --- src/cargo/util/toml/mod.rs | 18 ++++++++++-------- tests/testsuite/bad_config.rs | 23 +++++++++++++++++++++-- tests/testsuite/patch.rs | 25 ++++++++++++++++++------- 3 files changed, 49 insertions(+), 17 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index ea6a088cdf4..5768825e938 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -4,7 +4,6 @@ use std::ffi::OsStr; use std::path::{Path, PathBuf}; use std::rc::Rc; use std::str::{self, FromStr}; -use tracing_subscriber::fmt::format; use crate::core::summary::MissingDependencyError; use crate::AlreadyPrintedError; @@ -2145,7 +2144,7 @@ fn to_dependency_source_id( .unwrap_or(GitReference::DefaultBranch); let loc = git.into_url()?; - bail_if_github_pull_request(&name_in_toml, &loc)?; + warn_if_github_pull_request(&name_in_toml, &loc, manifest_ctx.warnings); if let Some(fragment) = loc.fragment() { let msg = format!( @@ -2187,23 +2186,26 @@ fn to_dependency_source_id( /// Checks if the URL is a GitHub pull request URL. /// -/// If the URL is a GitHub pull request URL, an error is returned with a message that explains -/// how to specify a specific git revision. -fn bail_if_github_pull_request(name_in_toml: &str, url: &Url) -> CargoResult<()> { +/// 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 . +fn warn_if_github_pull_request(name_in_toml: &str, url: &Url, warnings: &mut Vec) { if url.host_str() != Some("github.com") { - return Ok(()); + return; } let path_components = url.path().split('/').collect::>(); 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"); - bail!( + 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); } - Ok(()) } pub(crate) fn lookup_path_base<'a>( diff --git a/tests/testsuite/bad_config.rs b/tests/testsuite/bad_config.rs index 80f69e543a9..7b482b90779 100644 --- a/tests/testsuite/bad_config.rs +++ b/tests/testsuite/bad_config.rs @@ -2173,10 +2173,29 @@ fn github_pull_request_url() { p.cargo("check -v") .with_status(101) .with_stderr_data(str![[r#" -[ERROR] failed to parse manifest at `[ROOT]/foo/Cargo.toml` +[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: - 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. + unexpected http status code: 404; class=Http (34) "#]]) .run(); diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs index 99488e1f38c..0bc833f8b2d 100644 --- a/tests/testsuite/patch.rs +++ b/tests/testsuite/patch.rs @@ -359,11 +359,6 @@ fn patch_to_git() { #[cargo_test] fn patch_to_git_pull_request() { - let bar = git::repo(&paths::root().join("override")) - .file("Cargo.toml", &basic_manifest("bar", "0.1.0")) - .file("src/lib.rs", "pub fn bar() {}") - .build(); - Package::new("bar", "0.1.0").publish(); let p = project() @@ -392,10 +387,26 @@ fn patch_to_git_pull_request() { p.cargo("check -v") .with_status(101) .with_stderr_data(str![[r#" -[ERROR] failed to parse manifest at `[ROOT]/foo/Cargo.toml` +[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: - 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. + unexpected http status code: 404; class=Http (34) "#]]) .run(); From a27647d981077efb5ce26e407207fe19c2bd62a8 Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Thu, 9 Jan 2025 15:25:38 -0800 Subject: [PATCH 4/4] make test agnostic about exact network failure reason. Co-authored-by: Weihang Lo --- tests/testsuite/bad_config.rs | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/tests/testsuite/bad_config.rs b/tests/testsuite/bad_config.rs index 7b482b90779..dca1d77afa4 100644 --- a/tests/testsuite/bad_config.rs +++ b/tests/testsuite/bad_config.rs @@ -2175,9 +2175,7 @@ fn github_pull_request_url() { .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: @@ -2185,17 +2183,7 @@ Caused by: 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();