Skip to content

Commit

Permalink
fix(package): Avoid multiple package list entries
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@epage
epage committed Jul 26, 2023
1 parent 9b14e39 commit 98c862f
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 104 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
101 changes: 58 additions & 43 deletions src/cargo/ops/cargo_package.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(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()
))?;
}

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));
Expand Down
62 changes: 1 addition & 61 deletions tests/testsuite/package.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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();
Expand All @@ -3138,8 +3080,6 @@ See [..]
"\
Cargo.lock
Cargo.toml
Cargo.toml
Cargo.toml.orig
Cargo.toml.orig
src/main.rs
",
Expand Down

0 comments on commit 98c862f

Please sign in to comment.