fix(package): Avoid multiple package list entries

epage · epage · commit cc6b6c958479 · 2023-07-26T11:02:47.000-05:00
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.
diff --git a/Cargo.lock b/Cargo.lock
diff --git 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
@@ -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,70 +228,84 @@ fn build_ar_list(
     src_files: Vec<PathBuf>,
     vcs_info: Option<VcsInfo>,
 ) -> CargoResult<Vec<ArchiveFile>> {
-    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()
         ))?;
     }
 
     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
@@ -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
 ",
