From bedb5241a6acb27051dc5d23e45c9b6f4479d06c Mon Sep 17 00:00:00 2001 From: bors Date: Sun, 26 Feb 2023 02:55:02 +0000 Subject: [PATCH 1/2] Auto merge of #11756 - arlosi:sparse-uninstall, r=ehuss Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml The URL associated with a `SourceId` for a sparse registry includes the `sparse+` prefix in the URL to differentiate from git (non-sparse) registries. `SourceId::from_url` was not including the `sparse+` prefix of sparse registry URLs on construction, which caused roundtrips of `as_url` and `from_url` to fail by losing the prefix. Fixes #11751 by: * Including the prefix in the URL * Adding a test that verifies round-trip behavior for sparse `SourceId`s * Modifying `CanonicalUrl` so it no longer considers `sparse+` and non-`sparse+` URLs to be equivalent --- src/cargo/core/source/source_id.rs | 21 +++++++++- src/cargo/util/canonical_url.rs | 13 +----- tests/testsuite/install.rs | 65 ++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 13 deletions(-) diff --git a/src/cargo/core/source/source_id.rs b/src/cargo/core/source/source_id.rs index ce8f685f8be..5faa95d3f8b 100644 --- a/src/cargo/core/source/source_id.rs +++ b/src/cargo/core/source/source_id.rs @@ -81,6 +81,14 @@ impl SourceId { /// /// The canonical url will be calculated, but the precise field will not fn new(kind: SourceKind, url: Url, name: Option<&str>) -> CargoResult { + if kind == SourceKind::SparseRegistry { + // Sparse URLs are different because they store the kind prefix (sparse+) + // in the URL. This is because the prefix is necessary to differentiate + // from regular registries (git-based). The sparse+ prefix is included + // everywhere, including user-facing locations such as the `config.toml` + // file that defines the registry, or whenever Cargo displays it to the user. + assert!(url.as_str().starts_with("sparse+")); + } let source_id = SourceId::wrap(SourceIdInner { kind, canonical_url: CanonicalUrl::new(&url)?, @@ -152,7 +160,7 @@ impl SourceId { .with_precise(Some("locked".to_string()))) } "sparse" => { - let url = url.into_url()?; + let url = string.into_url()?; Ok(SourceId::new(SourceKind::SparseRegistry, url, None)? .with_precise(Some("locked".to_string()))) } @@ -721,6 +729,7 @@ impl<'a> fmt::Display for SourceIdAsUrl<'a> { ref url, .. } => { + // Sparse registry URL already includes the `sparse+` prefix write!(f, "{url}") } SourceIdInner { @@ -864,4 +873,14 @@ mod tests { assert_eq!(gen_hash(source_id), 17459999773908528552); assert_eq!(crate::util::hex::short_hash(&source_id), "6568fe2c2fab5bfe"); } + + #[test] + fn serde_roundtrip() { + let url = "sparse+https://my-crates.io/".into_url().unwrap(); + let source_id = SourceId::for_registry(&url).unwrap(); + let formatted = format!("{}", source_id.as_url()); + let deserialized = SourceId::from_url(&formatted).unwrap(); + assert_eq!(formatted, "sparse+https://my-crates.io/"); + assert_eq!(source_id, deserialized); + } } diff --git a/src/cargo/util/canonical_url.rs b/src/cargo/util/canonical_url.rs index 74a7152296a..7516e035691 100644 --- a/src/cargo/util/canonical_url.rs +++ b/src/cargo/util/canonical_url.rs @@ -1,4 +1,4 @@ -use crate::util::{errors::CargoResult, IntoUrl}; +use crate::util::errors::CargoResult; use std::hash::{self, Hash}; use url::Url; @@ -56,17 +56,6 @@ impl CanonicalUrl { url.path_segments_mut().unwrap().pop().push(&last); } - // Ignore the protocol specifier (if any). - if url.scheme().starts_with("sparse+") { - // NOTE: it is illegal to use set_scheme to change sparse+http(s) to http(s). - url = url - .to_string() - .strip_prefix("sparse+") - .expect("we just found that prefix") - .into_url() - .expect("a valid url without a protocol specifier should still be valid"); - } - Ok(CanonicalUrl(url)) } diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index c585b2f047a..3a1c04a9ecb 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -3,6 +3,7 @@ use std::fs::{self, OpenOptions}; use std::io::prelude::*; +use cargo_test_support::compare; use cargo_test_support::cross_compile; use cargo_test_support::git; use cargo_test_support::registry::{self, registry_path, Package}; @@ -2105,3 +2106,67 @@ fn no_auto_fix_note() { .run(); assert_has_not_installed_exe(cargo_home(), "auto_fix"); } + +#[cargo_test] +fn sparse_install() { + // Checks for an issue where uninstalling something corrupted + // the SourceIds of sparse registries. + // See https://github.com/rust-lang/cargo/issues/11751 + let _registry = registry::RegistryBuilder::new().http_index().build(); + + pkg("foo", "0.0.1"); + pkg("bar", "0.0.1"); + + cargo_process("install foo --registry dummy-registry") + .with_stderr( + "\ +[UPDATING] `dummy-registry` index +[DOWNLOADING] crates ... +[DOWNLOADED] foo v0.0.1 (registry `dummy-registry`) +[INSTALLING] foo v0.0.1 (registry `dummy-registry`) +[UPDATING] `dummy-registry` index +[COMPILING] foo v0.0.1 (registry `dummy-registry`) +[FINISHED] release [optimized] target(s) in [..] +[INSTALLING] [ROOT]/home/.cargo/bin/foo[EXE] +[INSTALLED] package `foo v0.0.1 (registry `dummy-registry`)` (executable `foo[EXE]`) +[WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries +", + ) + .run(); + assert_has_installed_exe(cargo_home(), "foo"); + let assert_v1 = |expected| { + let v1 = fs::read_to_string(paths::home().join(".cargo/.crates.toml")).unwrap(); + compare::assert_match_exact(expected, &v1); + }; + assert_v1( + r#"[v1] +"foo 0.0.1 (sparse+http://127.0.0.1:[..]/index/)" = ["foo[EXE]"] +"#, + ); + cargo_process("install bar").run(); + assert_has_installed_exe(cargo_home(), "bar"); + assert_v1( + r#"[v1] +"bar 0.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = ["bar[EXE]"] +"foo 0.0.1 (sparse+http://127.0.0.1:[..]/index/)" = ["foo[EXE]"] +"#, + ); + + cargo_process("uninstall bar") + .with_stderr("[REMOVING] [CWD]/home/.cargo/bin/bar[EXE]") + .run(); + assert_has_not_installed_exe(cargo_home(), "bar"); + assert_v1( + r#"[v1] +"foo 0.0.1 (sparse+http://127.0.0.1:[..]/index/)" = ["foo[EXE]"] +"#, + ); + cargo_process("uninstall foo") + .with_stderr("[REMOVING] [CWD]/home/.cargo/bin/foo[EXE]") + .run(); + assert_has_not_installed_exe(cargo_home(), "foo"); + assert_v1( + r#"[v1] +"#, + ); +} From 451b3997446d4cb8f56bb0a0f5a5f54192302ec4 Mon Sep 17 00:00:00 2001 From: bors Date: Sat, 25 Feb 2023 23:49:51 +0000 Subject: [PATCH 2/2] Auto merge of #11771 - ehuss:fix-temp-unused, r=Eh2406 Fix warning with tempfile A recent release of the tempfile crate added a `#[must_use]` attribute to the `into_path` method. This triggers a warning when building. This also adds a test to verify that the intermediate artifacts persist in the temp directory. --- src/cargo/ops/cargo_install.rs | 2 +- tests/testsuite/install.rs | 38 ++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 21de0997bf5..ef07c0263f9 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -307,7 +307,7 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> { let compile = ops::compile_ws(&self.ws, &self.opts, &exec).with_context(|| { if let Some(td) = td_opt.take() { // preserve the temporary directory, so the user can inspect it - td.into_path(); + drop(td.into_path()); } format!( diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index 3a1c04a9ecb..35efd8c0b72 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -2,6 +2,7 @@ use std::fs::{self, OpenOptions}; use std::io::prelude::*; +use std::path::Path; use cargo_test_support::compare; use cargo_test_support::cross_compile; @@ -10,6 +11,7 @@ use cargo_test_support::registry::{self, registry_path, Package}; use cargo_test_support::{ basic_manifest, cargo_process, no_such_file_err_msg, project, project_in, symlink_supported, t, }; +use cargo_util::ProcessError; use cargo_test_support::install::{ assert_has_installed_exe, assert_has_not_installed_exe, cargo_home, @@ -2107,6 +2109,42 @@ fn no_auto_fix_note() { assert_has_not_installed_exe(cargo_home(), "auto_fix"); } +#[cargo_test] +fn failed_install_retains_temp_directory() { + // Verifies that the temporary directory persists after a build failure. + Package::new("foo", "0.0.1") + .file("src/main.rs", "x") + .publish(); + let err = cargo_process("install foo").exec_with_output().unwrap_err(); + let err = err.downcast::().unwrap(); + let stderr = String::from_utf8(err.stderr.unwrap()).unwrap(); + compare::match_contains( + "\ +[UPDATING] `dummy-registry` index +[DOWNLOADING] crates ... +[DOWNLOADED] foo v0.0.1 (registry `dummy-registry`) +[INSTALLING] foo v0.0.1 +[COMPILING] foo v0.0.1 +", + &stderr, + None, + ) + .unwrap(); + compare::match_contains( + "error: failed to compile `foo v0.0.1`, intermediate artifacts can be found at `[..]`", + &stderr, + None, + ) + .unwrap(); + + // Find the path in the output. + let start = stderr.find("found at `").unwrap() + 10; + let end = stderr[start..].find('\n').unwrap() - 1; + let path = Path::new(&stderr[start..(end + start)]); + assert!(path.exists()); + assert!(path.join("release/deps").exists()); +} + #[cargo_test] fn sparse_install() { // Checks for an issue where uninstalling something corrupted