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

Reserve filename Cargo.toml.orig in cargo package #10551

Merged
merged 3 commits into from
Apr 12, 2022

Conversation

jeremyBanks
Copy link
Contributor

@jeremyBanks jeremyBanks commented Apr 9, 2022

What does this PR try to resolve?

(previously discussed on Zulip)

Currently, cargo package will detect if there is an existing file named .cargo_vcs_info.json in the project folder and raise an error. This is to avoid collisions with the file it generates of the same name.

However, there is no such logic for the other file it generates, Cargo.toml.orig. If such a file exists in the project folder, instead of an error, or one file taking precedence, cargo will generate a tarball containing two files with the same name. For example, from a package I uploaded as a test:

curl https://crates.io/api/v1/crates/a-/0.0.0--a-/download -L | gunzip | tar -tv
-rw-r--r--  0 0      0           8 29 Nov  1973 a--0.0.0--a-/.gitignore
-rw-r--r--  0 0      0         150 31 Dec  1969 a--0.0.0--a-/Cargo.lock
-rw-r--r--  0 0      0         641 31 Dec  1969 a--0.0.0--a-/Cargo.toml
-rw-r--r--  0 0      0         160 29 Nov  1973 a--0.0.0--a-/Cargo.toml.orig <-- generated
-rw-r--r--  0 0      0          14 29 Nov  1973 a--0.0.0--a-/Cargo.toml.orig <-- existing
-rw-r--r--  0 0      0          45 29 Nov  1973 a--0.0.0--a-/src/main.rs

This PR is a two-line change to add this filename to the existing check for .cargo_vcs_info.json.

How should we test and review this PR?

I have added a unit test to verify that the expected error is produced, copying the existing unit test for .cargo_vcs_info.json. I have manually tested the change locally and confirmed that it raises an error if the file exists, and has no effect if it does not. Given the small size of this change, I think this is sufficient, but please let me know if anything else is expected.

Additional information

This raises a separate question of whether Cargo or crates.io should prohibit tarballs with duplicate filenames. It seems like most (all?) tar implementations will give precedence to the last file (which will be the existing copy here, not the generated one), but I don't believe this is specified behaviour, and it's possible that scripts scanning through tarballs directly (without first extracting to the filesystem) may not handle this case correctly. For example, I was working on a rough script to compare packaged libraries to their corresponding Git commits where possible, and this wasn't a case I had anticipated.

In any case, it seems like preventing such tarballs from being created by accident (this PR) is a good place to start.

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 9, 2022
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Looks good!

One little suggestion: can you make Cargo.toml.orig a constant, like what VCS_INFO_FILE does? Line 222-233 also need to update accordingly. Thanks!

@jeremyBanks
Copy link
Contributor Author

@weihanglo Sure, done. 🙂

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks!

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 11, 2022

📌 Commit e71fb81 has been approved by weihanglo

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2022
@bors
Copy link
Contributor

bors commented Apr 11, 2022

⌛ Testing commit e71fb81 with merge 50d52eb...

@weihanglo weihanglo added the relnotes Release-note worthy label Apr 11, 2022
@bors
Copy link
Contributor

bors commented Apr 12, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 50d52eb to master...

@bors bors merged commit 50d52eb into rust-lang:master Apr 12, 2022
@jeremyBanks jeremyBanks deleted the Cargo.toml.orig branch April 12, 2022 00:33
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 14, 2022
Update cargo

11 commits in e2e2dddebe66dfc1403a312653557e332445308b..dba5baf4345858c591517b24801902a062c399f8
2022-04-05 17:04:53 +0000 to 2022-04-13 21:58:27 +0000
- Part 6 of RFC2906 - Switch the inheritance source from `workspace` to… (rust-lang/cargo#10564)
- Part 5 of RFC2906 - Add support for inheriting `rust-version` (rust-lang/cargo#10563)
- Add support for rustc --check-cfg well known names and values (rust-lang/cargo#10486)
- Reserve filename `Cargo.toml.orig` in `cargo package` (rust-lang/cargo#10551)
- Retry command invocation with argfile (rust-lang/cargo#10546)
- Add a progress indicator for `cargo clean` (rust-lang/cargo#10236)
- Ensure host units don't depend on Docscrape units, fixes rust-lang/cargo#10545 (rust-lang/cargo#10549)
- Fix docs: Bindeps env vars are passed to build script at runtime (rust-lang/cargo#10550)
- Part 4 of RFC2906 - Add support for inheriting `readme` (rust-lang/cargo#10548)
- Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` (rust-lang/cargo#10538)
- Bump to 0.63.0, update changelog (rust-lang/cargo#10544)
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2022
Update cargo

11 commits in e2e2dddebe66dfc1403a312653557e332445308b..dba5baf4345858c591517b24801902a062c399f8
2022-04-05 17:04:53 +0000 to 2022-04-13 21:58:27 +0000
- Part 6 of RFC2906 - Switch the inheritance source from `workspace` to… (rust-lang/cargo#10564)
- Part 5 of RFC2906 - Add support for inheriting `rust-version` (rust-lang/cargo#10563)
- Add support for rustc --check-cfg well known names and values (rust-lang/cargo#10486)
- Reserve filename `Cargo.toml.orig` in `cargo package` (rust-lang/cargo#10551)
- Retry command invocation with argfile (rust-lang/cargo#10546)
- Add a progress indicator for `cargo clean` (rust-lang/cargo#10236)
- Ensure host units don't depend on Docscrape units, fixes rust-lang/cargo#10545 (rust-lang/cargo#10549)
- Fix docs: Bindeps env vars are passed to build script at runtime (rust-lang/cargo#10550)
- Part 4 of RFC2906 - Add support for inheriting `readme` (rust-lang/cargo#10548)
- Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` (rust-lang/cargo#10538)
- Bump to 0.63.0, update changelog (rust-lang/cargo#10544)
@ehuss ehuss added this to the 1.62.0 milestone Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Release-note worthy S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants