fix(toml): Warn, rather than fail publish, if a target is excluded #13713

epage · 2024-04-05T19:02:36Z

What does this PR try to resolve?

We have a couple of problems with publishing

Inconsistent errors: if a target that package doesn't verify is missing path, it will error, while one with path won't, see cargo publish has bad error message when explicit [[bench]] isn't in package.include #13456
Users may want to exclude targets and their choices are
- Go ahead and include them. I originally excluded my examples before doc-scraping was a think. The problem was if I had to set required-features, I then could no longer exclude them
- Muck with Cargo.toml during publish and pass --allow-dirty

This fixes both by auto-stripping targets on publish. We will warn the user that we did so.

This is a mostly-one-way door on behavior because we are turning an error case into a warning.
For the most part, I think this is the right thing to do.
My biggest regret is that the warning is only during package/publish as it will be too late to act on it and people who want to know will want to know when the problem is introduced.
The error is also very late in the process but at least its before a non-reversible action has been taken.
Dry-run and yank help.

Fixes #13456
Fixes #5806

How should we test and review this PR?

Tests are added in the first commit and you can then follow the commits to see how the test output evolved.

The biggest risk factors for this change are

If the target-stripping logic mis-identifies a path as excluded because of innocuous path differences (e.g. case)
Setting a minimum MSRV for published packages: auto* were added in 1.27 (Introduce autoXXX keys for target auto-discovery #5335) but were insta-stable. autobins = false did nothing until 1.32 (Make autodiscovery disable inferred targets #6329). I have not checked to see how this behaves pre-1.32 or pre-1.27. Since my memory of that error is vague, I believe it will either do a redundant discovery or it will implicitly skip discovery

Resolved risks

fix(package): Normalize paths in Cargo.toml #13729 ensured our generated target paths don't have \ in them
fix(package): Normalize paths in Cargo.toml #13729 ensures the paths are normalize so the list of packaged paths

For case-insensitive filesystems, I added tests to show the original behavior (works locally but will fail when depended on from a case-sensitive filesystem) and tracked how that changed with this PR (on publish warn that those targets are stripped). We could try to normalize the case but it will also follow symlinks and is likely indicative of larger casing problems that the user had. Weighing how broken things are now , it didn't seem changing behavior on this would be too big of a deal.

We should do a Call for Testing when this hits nightly to have people to cargo package and look for targets exclusion warnings that don't make sense.

Additional information

This builds on #13701 and the work before it.

By enumerating all targets in Cargo.toml, it makes it so rust-lang/crates.io#5882 and rust-lang/crates.io#814 can be implemented without any other filesystem interactions.

A follow up PR is need to make much of a difference in performance because we unconditionally walk the file system just in case autodiscover != Some(false) or a target is missing a path.

We cannot turn off auto-discovery of libs, so that will always be done for bin-only packages.

rustbot · 2024-04-05T19:02:41Z

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

epage · 2024-04-05T19:02:56Z

Blocked on #13701. Posting now to get Windows testing.

For now, this is more for visual consistency. However, this blocks rust-lang#13713 as we need to be able to make these paths comparable to what is included in the package.

fix(package): Normalize paths in `Cargo.toml` ### What does this PR try to resolve? On the surface, this resolves problems that aren't too big of a deal - Clean up redundant information in paths (e.g. `.////.//foo.rs` being `foo.rs`) which is just visual - Switch paths with `\` in them to `/` However, this is prep for #13713 where these will be a much bigger deal - If the user does `./foo.rs`, we might fail to compare that with the list of files included in the package - We'll generate paths with `\` and need to make sure the packages can still be used on Windows ### How should we test and review this PR? ### Additional information

I left off the explicit `path` cases because I hope that will become moot

Not much of a performance gain; this is mostly done to be consistent with the target work.

This could offer a minor performance gain when reading this manifest since the target doesn't need to be discovered.

This could offer performance gains when parsing a published manifest since the targets don't need to be discovered. To see this, we'd first need to stop discovering potential targets even when it isn't needed.

weihanglo · 2024-04-29T18:21:49Z

@bors r+

Thanks!

bors · 2024-04-29T18:21:52Z

📌 Commit 8cc2f39 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-04-29T18:22:59Z

⌛ Testing commit 8cc2f39 with merge ba6fb40...

bors · 2024-04-29T18:52:21Z

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

glandium · 2024-04-29T20:01:56Z

How is this related to #5806?

epage · 2024-04-29T20:07:46Z

If I understand correctly cargo vendor will vendor "published" Cargo.toml files. With #5806, you could drop a build.rs and it would be automatically picked up. After this change, it will be ignored and the only way to add a build.rs is to hand-edit the Cargo.toml which will result in a checksum error.

glandium · 2024-04-29T20:29:52Z

So, if I understand correctly, this only "fixes" #5806 for crates published to crates.io, but e.g. crates vendored from a git repo, will still have the problem.

epage · 2024-04-30T02:32:47Z

From my understanding cargo vendor effectively runs cargo package on every dependency, regardless of the source. This change affects cargo package.

I tried this out locally by running cargo vendor on https://github.com/crate-ci/cargo-release/ which has git dev-dependencies. The vendored versions of those git dev-dependencies had the standard "this is from cargo package" boilerplate at the top and they had all of the auto* fields set to false from this change.

So I believe this fixes it for git as well. Feel free to test it when this hits nightly and let us know if it works or not!

Update cargo 15 commits in b60a1555155111e962018007a6d0ef85207db463..6087566b3fa73bfda29702632493e938b12d19e5 2024-04-26 16:37:29 +0000 to 2024-04-30 20:45:20 +0000 - fix(cargo-fix): dont fix into standard library (rust-lang/cargo#13792) - refactor: Move diagnostic printing to Shell (rust-lang/cargo#13813) - Populate git information when building Cargo from Rust's source tarball (rust-lang/cargo#13832) - docs: fix several typos found by `typos-cli` (rust-lang/cargo#13831) - fix(alias): Aliases without subcommands should not panic (rust-lang/cargo#13819) - fix(toml): Improve granularity of traces (rust-lang/cargo#13830) - fix(toml): Warn, rather than fail publish, if a target is excluded (rust-lang/cargo#13713) - test(cargo-lints): Add a test to ensure cap-lints works (rust-lang/cargo#13829) - fix(toml)!: Remove support for inheriting badges (rust-lang/cargo#13788) - chore(ci): Don't check `cargo` against beta channel (rust-lang/cargo#13827) - Fix target entry in .gitignore (rust-lang/cargo#13817) - Bump to 0.81.0; update changelog (rust-lang/cargo#13823) - Add failing test: artifact_dep_target_specified (rust-lang/cargo#13816) - fix(cargo-lints): Don't always inherit workspace lints (rust-lang/cargo#13812) - Update SleepTraker returns_in_order unit test (rust-lang/cargo#13811) r? ghost

glandium · 2024-05-02T06:23:27Z

I tested 1.80.0-nightly (c987ad527 2024-05-01), and vendoring indeed adds the auto* and build fields (and more).

rustbot assigned ehuss Apr 5, 2024

epage marked this pull request as draft April 5, 2024 19:02

rustbot added A-manifest Area: Cargo.toml issues Command-package S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2024

epage force-pushed the no-auto branch 3 times, most recently from f36f0d8 to 4217b7b Compare April 9, 2024 02:01

epage mentioned this pull request Apr 9, 2024

Option to disable installation with install #13699

Closed

epage mentioned this pull request Apr 10, 2024

fix(package): Normalize paths in Cargo.toml #13729

Merged

epage force-pushed the no-auto branch 2 times, most recently from f058f8e to e2e0fb4 Compare April 10, 2024 02:30

epage force-pushed the no-auto branch 10 times, most recently from c3bae94 to b2b704f Compare April 10, 2024 21:45

epage force-pushed the no-auto branch 2 times, most recently from dc4389f to 87ca923 Compare April 29, 2024 15:41

epage added 6 commits April 29, 2024 12:25

test(package): Show current case behavior

340050e

test(package): Show different crate discovery cases

0cf29c5

I left off the explicit `path` cases because I hope that will become moot

perf(toml): Avoid looking up readme on published packages

39f1a21

Not much of a performance gain; this is mostly done to be consistent with the target work.

fix(toml): Warn, rather than fail publish, if build.rs is excluded

1e60477

This could offer a minor performance gain when reading this manifest since the target doesn't need to be discovered.

fix(toml): Warn, rather than fail publish, if targets are excluded

06a5714

This could offer performance gains when parsing a published manifest since the targets don't need to be discovered. To see this, we'd first need to stop discovering potential targets even when it isn't needed.

docs(toml): Clarify what resolving a TOML means

8cc2f39

epage force-pushed the no-auto branch from 87ca923 to 8cc2f39 Compare April 29, 2024 17:26

weihanglo approved these changes Apr 29, 2024

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 Apr 29, 2024

bors merged commit ba6fb40 into rust-lang:master Apr 29, 2024
21 checks passed

bors mentioned this pull request Apr 29, 2024

Strip cargo lints table #13806

Closed

epage deleted the no-auto branch April 29, 2024 20:05

rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Apr 30, 2024

weihanglo mentioned this pull request May 1, 2024

Update cargo rust-lang/rust#124572

Merged

rustbot added this to the 1.80.0 milestone May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(toml): Warn, rather than fail publish, if a target is excluded #13713

fix(toml): Warn, rather than fail publish, if a target is excluded #13713

epage commented Apr 5, 2024 •

edited

rustbot commented Apr 5, 2024

epage commented Apr 5, 2024

weihanglo commented Apr 29, 2024

bors commented Apr 29, 2024

bors commented Apr 29, 2024

bors commented Apr 29, 2024

glandium commented Apr 29, 2024

epage commented Apr 29, 2024

glandium commented Apr 29, 2024

epage commented Apr 30, 2024

glandium commented May 2, 2024

fix(toml): Warn, rather than fail publish, if a target is excluded #13713

fix(toml): Warn, rather than fail publish, if a target is excluded #13713

Conversation

epage commented Apr 5, 2024 • edited

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Apr 5, 2024

epage commented Apr 5, 2024

weihanglo commented Apr 29, 2024

bors commented Apr 29, 2024

bors commented Apr 29, 2024

bors commented Apr 29, 2024

glandium commented Apr 29, 2024

epage commented Apr 29, 2024

glandium commented Apr 29, 2024

epage commented Apr 30, 2024

glandium commented May 2, 2024

epage commented Apr 5, 2024 •

edited