Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crates_io_tarball: Ignore duplicate readme files #8802

Closed
wants to merge 1 commit into from

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Jun 6, 2024

cargo is currently producing crate files with multiple readme files under certain conditions. While this is a bug on cargo side, we should probably be liberal in this case and still accept the crate files.

see #8788 (comment)

cargo is currently producing crate files with multiple readme files under certain conditions. While this is a bug on cargo side, we should probably be liberal in this case and still accept the crate files.
@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-backend ⚙️ labels Jun 6, 2024
@Turbo87 Turbo87 requested a review from a team June 6, 2024 14:45
Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.65%. Comparing base (1f4d255) to head (8882318).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8802   +/-   ##
=======================================
  Coverage   88.65%   88.65%           
=======================================
  Files         276      276           
  Lines       27614    27622    +8     
=======================================
+ Hits        24480    24489    +9     
+ Misses       3134     3133    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given rust-lang/cargo#13722, do we need to extend this to licence files as well? (I can't immediately get cargo package to generate the same ambiguity there, but I haven't played around with it very much.)

I might be more inclined to revert to the previous behaviour (only checking manifests) for now pending alignment with t-cargo on the exact semantics here, but I don't feel strongly enough about this to not approve the PR.

@Turbo87
Copy link
Member Author

Turbo87 commented Jun 6, 2024

do we need to extend this to licence files as well?

I'm not sure. is LICENSE.md also a by-default file like README.md? I was under the impression that the license file is only relevant if license-file is used, which is usually only used if license is not set?

@LawnGnome
Copy link
Contributor

I'm not sure. is LICENSE.md also a by-default file like README.md? I was under the impression that the license file is only relevant if license-file is used, which is usually only used if license is not set?

I figured out how to reproduce this for licences as well:

# Create a new crate.
cargo new foo

# Create a licence file in the parent directory.
touch LICENSE

# Move into the crate and create another licence file with different casing.
cd foo
touch license

# Set the license-file in the manifest to point to ../LICENSE.
sed -i -e 's/edition = "2021"/&\nlicense-file = "..\/LICENSE"/' Cargo.toml

# Run cargo package.
cargo package --allow-dirty

# Finally, observe the problem.
tar ztvf target/package/foo-0.1.0.crate

Which results in:

-rw-r--r-- 0/0             147 1969-12-31 16:00 foo-0.1.0/Cargo.lock
-rw-r--r-- 0/0             566 1969-12-31 16:00 foo-0.1.0/Cargo.toml
-rw-r--r-- 0/0             102 2006-07-23 18:21 foo-0.1.0/Cargo.toml.orig
-rw-r--r-- 0/0              15 2006-07-23 18:21 foo-0.1.0/LICENSE
-rw-r--r-- 0/0              16 2006-07-23 18:21 foo-0.1.0/license
-rw-r--r-- 0/0              45 2006-07-23 18:21 foo-0.1.0/src/main.rs

Lockfiles are mentioned in rust-lang/cargo#13722 as well, although I think that one's less of an issue because it would be more obvious on the filesystem (because you'd have cargo.lock and Cargo.lock in the same directory) — unlike readmes and licence files, I don't think there's a way to have cargo pull in a file from elsewhere.

So my feeling is that we should either treat licence files the same way we treat readmes, whatever that may be.

Added problem that I obviously hadn't fully thought through before coffee had kicked in earlier: there's no particular guarantee the readme will be readme.md: it could be readme.txt or readme even just dealing with the file names cargo will implicitly use, not to mention other file formats like reStructuredText. It's even worse for licence files — you have the same diversity of formats, but also diversity in English, since I would call it LICENCE with a c and Americans would call it LICENSE with an s.

So, in summary: instead of this PR, I think we revert #8788 for now to go back to only preventing duplicate manifests (since that's much more problematic from a security perspective), then figure out what the semantics of handling case-insensitivity in crate files should be and (ideally) have cargo and crates.io do the same thing. (Spoiler: I think @kornelski is on the right path here.)

@LawnGnome
Copy link
Contributor

@Turbo87 and I talked about this and agreed that we'll go with a straight revert of #8788 for now. Closing this PR.

@LawnGnome LawnGnome closed this Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants