From 98c862f0227e2bc51bf7cc98b3d8680a12ce1893 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 26 Jul 2023 11:02:47 -0500 Subject: [PATCH] fix(package): Avoid multiple package list entries To keep this simple, we are leveraging `unicase` for case-insensitive comparisons which I've been using for years on other projects. This also opens the door for us to add cross-platform compatibility hazard warnings about multiple paths that would write to the same location on a case insensitive file system. I held off on that because I assume we would want #12235 first. This does mean we can't test the "no manifest" case anymore because the one case (no pun intended) I knew of for hitting it is now gone. --- Cargo.lock | 1 + Cargo.toml | 2 + src/cargo/ops/cargo_package.rs | 101 +++++++++++++++++++-------------- tests/testsuite/package.rs | 62 +------------------- 4 files changed, 62 insertions(+), 104 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b92088f6d1da..322cebc0fdc6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -335,6 +335,7 @@ dependencies = [ "time", "toml", "toml_edit", + "unicase", "unicode-width", "unicode-xid", "url", diff --git a/Cargo.toml b/Cargo.toml index 1ee07b17e37d..48b6e1bef157 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -94,6 +94,7 @@ thiserror = "1.0.40" time = { version = "0.3", features = ["parsing", "formatting", "serde"] } toml = "0.7.0" toml_edit = "0.19.0" +unicase = "2.6.0" unicode-width = "0.1.5" unicode-xid = "0.2.0" url = "2.2.2" @@ -177,6 +178,7 @@ termcolor.workspace = true time.workspace = true toml.workspace = true toml_edit.workspace = true +unicase.workspace = true unicode-width.workspace = true unicode-xid.workspace = true url.workspace = true diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 1493f48479df..e371bd43523c 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -25,6 +25,7 @@ use flate2::{Compression, GzBuilder}; use log::debug; use serde::Serialize; use tar::{Archive, Builder, EntryType, Header, HeaderMode}; +use unicase::UniCase; pub struct PackageOpts<'cfg> { pub config: &'cfg Config, @@ -227,50 +228,54 @@ fn build_ar_list( src_files: Vec, vcs_info: Option, ) -> CargoResult> { - let mut result = Vec::new(); + let mut result = HashMap::new(); let root = pkg.root(); - let mut manifest_found = false; - for src_file in src_files { - let rel_path = src_file.strip_prefix(&root)?.to_path_buf(); - check_filename(&rel_path, &mut ws.config().shell())?; - let rel_str = rel_path - .to_str() - .ok_or_else(|| { - anyhow::format_err!("non-utf8 path in source directory: {}", rel_path.display()) - })? - .to_string(); + for src_file in &src_files { + let rel_path = src_file.strip_prefix(&root)?; + check_filename(rel_path, &mut ws.config().shell())?; + let rel_str = rel_path.to_str().ok_or_else(|| { + anyhow::format_err!("non-utf8 path in source directory: {}", rel_path.display()) + })?; match rel_str.as_ref() { - "Cargo.toml" | - // normalize for case insensitive filesystems (like on Windows) - "cargo.toml" => { - manifest_found = true; - result.push(ArchiveFile { - rel_path: PathBuf::from(ORIGINAL_MANIFEST_FILE), - rel_str: ORIGINAL_MANIFEST_FILE.to_string(), - contents: FileContents::OnDisk(src_file), - }); - result.push(ArchiveFile { - rel_path: PathBuf::from("Cargo.toml"), - rel_str: "Cargo.toml".to_string(), - contents: FileContents::Generated(GeneratedFile::Manifest), - }); - } "Cargo.lock" => continue, VCS_INFO_FILE | ORIGINAL_MANIFEST_FILE => anyhow::bail!( "invalid inclusion of reserved file name {} in package source", rel_str ), _ => { - result.push(ArchiveFile { - rel_path, - rel_str, - contents: FileContents::OnDisk(src_file), - }); + result + .entry(UniCase::new(rel_str)) + .or_insert_with(Vec::new) + .push(ArchiveFile { + rel_path: rel_path.to_owned(), + rel_str: rel_str.to_owned(), + contents: FileContents::OnDisk(src_file.clone()), + }); } } } - if !manifest_found { + + // Ensure we normalize for case insensitive filesystems (like on Windows) by removing the + // existing entry, regardless of case, and adding in with the correct case + if result.remove(&UniCase::new("Cargo.toml")).is_some() { + result + .entry(UniCase::new(ORIGINAL_MANIFEST_FILE)) + .or_insert_with(Vec::new) + .push(ArchiveFile { + rel_path: PathBuf::from(ORIGINAL_MANIFEST_FILE), + rel_str: ORIGINAL_MANIFEST_FILE.to_string(), + contents: FileContents::OnDisk(pkg.manifest_path().to_owned()), + }); + result + .entry(UniCase::new("Cargo.toml")) + .or_insert_with(Vec::new) + .push(ArchiveFile { + rel_path: PathBuf::from("Cargo.toml"), + rel_str: "Cargo.toml".to_string(), + contents: FileContents::Generated(GeneratedFile::Manifest), + }); + } else { ws.config().shell().warn(&format!( "no `Cargo.toml` file found when packaging `{}` (note the case of the file name).", pkg.name() @@ -278,19 +283,29 @@ fn build_ar_list( } if pkg.include_lockfile() { - result.push(ArchiveFile { - rel_path: PathBuf::from("Cargo.lock"), - rel_str: "Cargo.lock".to_string(), - contents: FileContents::Generated(GeneratedFile::Lockfile), - }); + let rel_str = "Cargo.lock"; + result + .entry(UniCase::new(rel_str)) + .or_insert_with(Vec::new) + .push(ArchiveFile { + rel_path: PathBuf::from(rel_str), + rel_str: rel_str.to_string(), + contents: FileContents::Generated(GeneratedFile::Lockfile), + }); } if let Some(vcs_info) = vcs_info { - result.push(ArchiveFile { - rel_path: PathBuf::from(VCS_INFO_FILE), - rel_str: VCS_INFO_FILE.to_string(), - contents: FileContents::Generated(GeneratedFile::VcsInfo(vcs_info)), - }); - } + let rel_str = VCS_INFO_FILE; + result + .entry(UniCase::new(rel_str)) + .or_insert_with(Vec::new) + .push(ArchiveFile { + rel_path: PathBuf::from(rel_str), + rel_str: rel_str.to_string(), + contents: FileContents::Generated(GeneratedFile::VcsInfo(vcs_info)), + }); + } + + let mut result = result.into_values().flatten().collect(); if let Some(license_file) = &pkg.manifest().metadata().license_file { let license_path = Path::new(license_file); let abs_file_path = paths::normalize_path(&pkg.root().join(license_path)); diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index a9bbcf035905..010523fda909 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -3042,64 +3042,6 @@ src/main.rs ); } -#[cargo_test] -#[cfg(windows)] // windows is the platform that is most consistently configured for case insensitive filesystems -fn no_manifest_found() { - let p = project() - .file("src/main.rs", r#"fn main() { println!("hello"); }"#) - .file("src/bar.txt", "") // should be ignored when packaging - .build(); - // Workaround `project()` making a `Cargo.toml` on our behalf - std::fs::remove_file(p.root().join("Cargo.toml")).unwrap(); - std::fs::write( - p.root().join("CARGO.TOML"), - r#" - [package] - name = "foo" - version = "0.0.1" - authors = [] - exclude = ["*.txt"] - license = "MIT" - description = "foo" - "#, - ) - .unwrap(); - - p.cargo("package") - .with_stderr( - "\ -[WARNING] manifest has no documentation[..] -See [..] -[WARNING] no `Cargo.toml` file found when packaging `foo` (note the case of the file name). -[PACKAGING] foo v0.0.1 ([CWD]) -[VERIFYING] foo v0.0.1 ([CWD]) -[COMPILING] foo v0.0.1 ([CWD][..]) -[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] -[PACKAGED] 3 files, [..] ([..] compressed) -", - ) - .run(); - assert!(p.root().join("target/package/foo-0.0.1.crate").is_file()); - p.cargo("package -l") - .with_stdout( - "\ -CARGO.TOML -Cargo.lock -src/main.rs -", - ) - .run(); - p.cargo("package").with_stdout("").run(); - - let f = File::open(&p.root().join("target/package/foo-0.0.1.crate")).unwrap(); - validate_crate_contents( - f, - "foo-0.0.1.crate", - &["CARGO.TOML", "Cargo.lock", "src/main.rs"], - &[], - ); -} - #[cargo_test] #[cfg(target_os = "linux")] // linux is generally configured to be case sensitive fn mixed_case() { @@ -3128,7 +3070,7 @@ See [..] [VERIFYING] foo v0.0.1 ([CWD]) [COMPILING] foo v0.0.1 ([CWD][..]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] -[PACKAGED] 6 files, [..] ([..] compressed) +[PACKAGED] 4 files, [..] ([..] compressed) ", ) .run(); @@ -3138,8 +3080,6 @@ See [..] "\ Cargo.lock Cargo.toml -Cargo.toml -Cargo.toml.orig Cargo.toml.orig src/main.rs ",