fix(package): Recognize and normalize `cargo.toml` #12399

epage · 2023-07-24T21:58:54Z

What does this PR try to resolve?

This solution is a blend of conservative and easy

Normalizes cargo.toml to Cargo.toml on publish
- Ensuring we publish the prepare_for_publish version and include Cargo.toml.orig
- Avoids dealing with trying to push existing users to Cargo.toml
All other cases of Cargo.toml are warnings
- We could either normalize or turn this into an error in the future
- When helping users with case previously, we've only handle the cargo.toml case
- We already should have a fallback in case a manifest isn't detected
- I didn't want to put in too much effort to make the code more complicated to handle this

As a side effect, if a Linux user has cargo.toml and Cargo.toml, we'll only put one of them in the .crate file. We can extend this out to also include a warning for portability for case insensitive filesystems but I left that for after #12235.

How should we test and review this PR?

A PR at a time will show how the behavior changed as the source was edited

This does add a direct dependency on unicase to help keep case-insensitive comparisons easy / clear and to avoid riskier areas for bugs like writing an appropriate Hash implementation. unicase is an existing transitive dependency of cargo.

Additional information

Fixes #12384

Discussion on Zulip

Being a bit cautious about not turning this into an error since this is most likely because of case insensitive filesystems.

rustbot · 2023-07-24T21:58:58Z

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

weihanglo

Another case this does not address (though slightly changes) is when a Linux system has multiple cases of Cargo.toml

Will this cause problems for projects already having both Cargo.toml and cargo.toml? Should we examine existing packages on crates.io?

Apart from that, looks pretty good!

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 rust-lang#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.

epage · 2023-07-26T16:05:39Z

Will this cause problems for projects already having both Cargo.toml and cargo.toml? Should we examine existing packages on crates.io?

To lazy to examine them so I took an approach for handling it. Thoughts?

ChrisDenton · 2023-07-26T17:41:06Z

Hm... unicase seems overkill no? IIRC, cargo's special filenames are all ascii.

epage · 2023-07-26T17:44:53Z

Hm... unicase seems overkill no? IIRC, cargo's special filenames are all ascii.

I wrote this with an eye for portability hazards in the future. Unsure if filesystems are case insensitive by ascii or by unicode but thought I'd play it safe rather than people in the future building on the this work and not taking this into account.

ChrisDenton · 2023-07-26T17:48:59Z

Sure but I mean, pure ascii filenames are the only ones that can be compared portably.

What version of the Unicode spec does a filesystem use to do case-insensitive comparisons? Trick question! It could be using any version or its own weird case table. So the only portable thing would be to ask the filesystem, unless you stick to ascii only.

epage · 2023-07-26T18:34:25Z

@ChrisDenton to clarify, how much of your concern is with

unicase::UniCase
unicase as a dependency
any dependency for this

ChrisDenton · 2023-07-26T19:09:28Z

Well I do prefer having fewer dependencies but that's only a minor concern.

And I'm not against using unicase::UniCase exactly. It'll work for Cargo.toml, but then so would an ascii comparison. My concern is a bit more fundamental in that using any single case algorithm is wrong. With ascii comparisons the limitation is at least clear to the reader and the results correct within its domain.

The "correct" way to do this for arbitrary names outside of ascii is to try opening with the expected file name then comparing the canonical name of the opened file with the expected name. But I realise this might be more involved then is wanted here, especially considering cross-platform concerns.

epage · 2023-07-26T20:14:23Z

Thanks for the clarification.

I've updated to ASCII comparisons. I kept unicase because it makes life easier in a couple of ways, in particular by choosing to use a HashMap, it makes it easy to get a correct Hash implementation. It is already a transitive dependency of cargo.

tests/testsuite/package.rs

weihanglo

Looks good to me, and thanks everybody here for helping review :)

weihanglo · 2023-07-29T21:09:09Z

tests/testsuite/package.rs

+ [package]
+ name = "foo"
+ version = "0.0.1"
+ authors = []
+ exclude = ["*.txt"]
+ license = "MIT"
+ description = "foo"
+ "#,


nit: One less indentation?

weihanglo · 2023-07-29T21:55:43Z

@bors r

bors · 2023-07-29T21:55:45Z

📌 Commit cc6b6c9 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-07-29T21:55:51Z

⌛ Testing commit cc6b6c9 with merge c863660...

bors · 2023-07-29T22:50:10Z

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

Update cargo 14 commits in 7ac9416d82cd4fc5e707c9ec3574d22dff6466e5..c91a693e7977e33a1064b63a5daf5fb689f01651 2023-07-24 14:29:38 0000 to 2023-07-31 00:26:46 0000 - fix: align `cargo --help` text (rust-lang/cargo#12418) - fix: normalize relative git submodule urls with `ssh://` (rust-lang/cargo#12411) - test: relax help text assertion (rust-lang/cargo#12416) - test: relax assertions of panic handler message format (rust-lang/cargo#12413) - fix(package): Recognize and normalize `cargo.toml` (rust-lang/cargo#12399) - Clarify `lto` setting passing `-Clinker-plugin-lto` (rust-lang/cargo#12407) - doc: add missing reference to `CARGO_PROFILE_<name>_STRIP` in environment variables docs (rust-lang/cargo#12408) - Update curl-sys to pull in curl 8.2.1 (rust-lang/cargo#12406) - docs: raise awareness of resolver used inside workspace (rust-lang/cargo#12388) - chore: update `home` to 0.5.7 (rust-lang/cargo#12401) - Update curl-sys to pull in curl 8.2.0 (rust-lang/cargo#12400) - test(cli): Track --help output (rust-lang/cargo#11912) - refactor(test): Move cargo-config into a dir (rust-lang/cargo#12398) - refactor(tests): Name init ui tests more consistently (rust-lang/cargo#12397) r? `@ghost`

epage added 4 commits July 24, 2023 16:13

test(package): Verify cargo.toml behavior

5dd39df

fix(package): Normalize cargo.toml to Cargo.toml for windows

bbb6aff

test(package): Verify the no-manifest case

f4c97b2

fix(package): Warn when manifest is not more generally found

3166e5f

Being a bit cautious about not turning this into an error since this is most likely because of case insensitive filesystems.

rustbot assigned weihanglo Jul 24, 2023

rustbot added Command-package S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2023

epage force-pushed the normalize branch 2 times, most recently from 1d9fcb2 to 3166e5f Compare July 25, 2023 00:39

weihanglo reviewed Jul 26, 2023

View reviewed changes

epage added 2 commits July 26, 2023 10:10

test(package): Verify mixed-case Cargo.toml

9b14e39

epage force-pushed the normalize branch from 98c862f to cc6b6c9 Compare July 26, 2023 20:10

riking reviewed Jul 28, 2023

View reviewed changes

tests/testsuite/package.rs Show resolved Hide resolved

weihanglo approved these changes Jul 29, 2023

View reviewed changes

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 Jul 29, 2023

bors merged commit c863660 into rust-lang:master Jul 29, 2023
21 checks passed

weihanglo mentioned this pull request Jul 31, 2023

Update cargo rust-lang/rust#114277

Merged

epage deleted the normalize branch August 4, 2023 17:43

ehuss added this to the 1.73.0 milestone Aug 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(package): Recognize and normalize `cargo.toml` #12399

fix(package): Recognize and normalize `cargo.toml` #12399

epage commented Jul 24, 2023 •

edited

Loading

rustbot commented Jul 24, 2023

weihanglo left a comment •

edited

Loading

epage commented Jul 26, 2023

ChrisDenton commented Jul 26, 2023

epage commented Jul 26, 2023

ChrisDenton commented Jul 26, 2023

epage commented Jul 26, 2023

ChrisDenton commented Jul 26, 2023 •

edited

Loading

epage commented Jul 26, 2023

weihanglo left a comment

weihanglo Jul 29, 2023

weihanglo commented Jul 29, 2023

bors commented Jul 29, 2023

bors commented Jul 29, 2023

bors commented Jul 29, 2023

fix(package): Recognize and normalize cargo.toml #12399

fix(package): Recognize and normalize cargo.toml #12399

Conversation

epage commented Jul 24, 2023 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Jul 24, 2023

weihanglo left a comment • edited Loading

Choose a reason for hiding this comment

epage commented Jul 26, 2023

ChrisDenton commented Jul 26, 2023

epage commented Jul 26, 2023

ChrisDenton commented Jul 26, 2023

epage commented Jul 26, 2023

ChrisDenton commented Jul 26, 2023 • edited Loading

epage commented Jul 26, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Jul 29, 2023

Choose a reason for hiding this comment

weihanglo commented Jul 29, 2023

bors commented Jul 29, 2023

bors commented Jul 29, 2023

bors commented Jul 29, 2023

fix(package): Recognize and normalize `cargo.toml` #12399

fix(package): Recognize and normalize `cargo.toml` #12399

epage commented Jul 24, 2023 •

edited

Loading

weihanglo left a comment •

edited

Loading

ChrisDenton commented Jul 26, 2023 •

edited

Loading