From 5dd39df6f188885f843302d877f419f22af733b0 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 24 Jul 2023 15:42:54 -0500 Subject: [PATCH 1/6] test(package): Verify cargo.toml behavior --- tests/testsuite/package.rs | 57 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 3b432824295..44427023609 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -2983,3 +2983,60 @@ src/main.rs.bak ], ); } + +#[cargo_test] +#[cfg(windows)] // windows is the platform that is most consistently configured for case insensitive filesystems +fn normalize_case() { + 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 [..] +[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.lock +cargo.toml +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.lock", "cargo.toml", "src/main.rs"], + &[], + ); +} From bbb6aff67aa2302f84115eae518084f383a566dc Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 24 Jul 2023 16:01:59 -0500 Subject: [PATCH 2/6] fix(package): Normalize cargo.toml to Cargo.toml for windows --- src/cargo/ops/cargo_package.rs | 8 +++++--- tests/testsuite/package.rs | 7 ++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index a322afbb341..679883e4f70 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -239,15 +239,17 @@ fn build_ar_list( })? .to_string(); match rel_str.as_ref() { - "Cargo.toml" => { + "Cargo.toml" | + // normalize for case insensitive filesystems (like on Windows) + "cargo.toml" => { 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, - rel_str, + rel_path: PathBuf::from("Cargo.toml"), + rel_str: "Cargo.toml".to_string(), contents: FileContents::Generated(GeneratedFile::Manifest), }); } diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 44427023609..4d33b2f9822 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -3016,7 +3016,7 @@ See [..] [VERIFYING] foo v0.0.1 ([CWD]) [COMPILING] foo v0.0.1 ([CWD][..]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] -[PACKAGED] 3 files, [..] ([..] compressed) +[PACKAGED] 4 files, [..] ([..] compressed) ", ) .run(); @@ -3025,7 +3025,8 @@ See [..] .with_stdout( "\ Cargo.lock -cargo.toml +Cargo.toml +Cargo.toml.orig src/main.rs ", ) @@ -3036,7 +3037,7 @@ src/main.rs validate_crate_contents( f, "foo-0.0.1.crate", - &["Cargo.lock", "cargo.toml", "src/main.rs"], + &["Cargo.lock", "Cargo.toml", "Cargo.toml.orig", "src/main.rs"], &[], ); } From f4c97b24a8122e30c62a79ac498c00d861609e2d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 24 Jul 2023 16:41:15 -0500 Subject: [PATCH 3/6] test(package): Verify the no-manifest case --- tests/testsuite/package.rs | 56 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 4d33b2f9822..414073ea28a 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -3041,3 +3041,59 @@ 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 [..] +[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"], + &[], + ); +} From 3166e5f6146c168e7ce109bbea793c7743b045a1 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 24 Jul 2023 16:49:38 -0500 Subject: [PATCH 4/6] fix(package): Warn when manifest is not more generally found Being a bit cautious about not turning this into an error since this is most likely because of case insensitive filesystems. --- src/cargo/ops/cargo_package.rs | 10 ++++++++++ tests/testsuite/package.rs | 1 + 2 files changed, 11 insertions(+) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 679883e4f70..1493f48479d 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -229,6 +229,8 @@ fn build_ar_list( ) -> CargoResult> { let mut result = Vec::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())?; @@ -242,6 +244,7 @@ fn build_ar_list( "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(), @@ -267,6 +270,13 @@ fn build_ar_list( } } } + if !manifest_found { + ws.config().shell().warn(&format!( + "no `Cargo.toml` file found when packaging `{}` (note the case of the file name).", + pkg.name() + ))?; + } + if pkg.include_lockfile() { result.push(ArchiveFile { rel_path: PathBuf::from("Cargo.lock"), diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 414073ea28a..964d61616ba 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -3069,6 +3069,7 @@ fn no_manifest_found() { "\ [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][..]) From 9b14e39cd7783523c4f1fee9ce285707723d5c43 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 26 Jul 2023 10:10:21 -0500 Subject: [PATCH 5/6] test(package): Verify mixed-case Cargo.toml --- tests/testsuite/package.rs | 57 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 964d61616ba..a9bbcf03590 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -3041,6 +3041,7 @@ src/main.rs &[], ); } + #[cargo_test] #[cfg(windows)] // windows is the platform that is most consistently configured for case insensitive filesystems fn no_manifest_found() { @@ -3098,3 +3099,59 @@ src/main.rs &[], ); } + +#[cargo_test] +#[cfg(target_os = "linux")] // linux is generally configured to be case sensitive +fn mixed_case() { + let manifest = r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + exclude = ["*.txt"] + license = "MIT" + description = "foo" + "#; + let p = project() + .file("Cargo.toml", manifest) + .file("cargo.toml", manifest) + .file("src/main.rs", r#"fn main() { println!("hello"); }"#) + .file("src/bar.txt", "") // should be ignored when packaging + .build(); + + p.cargo("package") + .with_stderr( + "\ +[WARNING] manifest has no documentation[..] +See [..] +[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] 6 files, [..] ([..] compressed) +", + ) + .run(); + assert!(p.root().join("target/package/foo-0.0.1.crate").is_file()); + p.cargo("package -l") + .with_stdout( + "\ +Cargo.lock +Cargo.toml +Cargo.toml +Cargo.toml.orig +Cargo.toml.orig +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.lock", "Cargo.toml", "Cargo.toml.orig", "src/main.rs"], + &[], + ); +} From cc6b6c958479288b2ed27c5a6f993100e5988bc3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 26 Jul 2023 11:02:47 -0500 Subject: [PATCH 6/6] fix(package): Avoid multiple package list entries To keep things simple, especially in getting a `Hash` implementation correct, I'm leveraging `unicase` for case-insensitive comparisons which is an existing dependency and 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 b92088f6d1d..322cebc0fdc 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 1ee07b17e37..48b6e1bef15 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 1493f48479d..e904dba83f0 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::Ascii as UncasedAscii; 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(UncasedAscii::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(&UncasedAscii::new("Cargo.toml")).is_some() { + result + .entry(UncasedAscii::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(UncasedAscii::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(UncasedAscii::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(UncasedAscii::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 a9bbcf03590..010523fda90 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 ",