-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix(toml): Warn, rather than fail publish, if a target is excluded #13713
Conversation
Blocked on #13701. Posting now to get Windows testing. |
f36f0d8
to
4217b7b
Compare
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.
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.
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.
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.
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.
f058f8e
to
e2e0fb4
Compare
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
c3bae94
to
b2b704f
Compare
☀️ Test successful - checks-actions |
How is this related to #5806? |
If I understand correctly |
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. |
From my understanding I tried this out locally by running 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
I tested 1.80.0-nightly (c987ad527 2024-05-01), and vendoring indeed adds the |
With rust-lang#13713, we enumerate all targets in `Cargo.toml` on `cargo publish` and `cargo vendor`. However, the order of the targets is non-determistic. This can be annoying for comparing the published `Cargo.toml` across releases but even worse is the churn it causes for `cargo vendor`. So we sort all the targets during publish. This keeps costs minimal with the following risks - If the non-determinism shows up in a way that affects developers during development - If there is a reason the user wants to control target order for explicit targets Fixes rust-lang#13988
With rust-lang#13713, we enumerate all targets in `Cargo.toml` on `cargo publish` and `cargo vendor`. However, the order of the targets is non-determistic. This can be annoying for comparing the published `Cargo.toml` across releases but even worse is the churn it causes for `cargo vendor`. So we sort all the targets during publish. This keeps costs minimal with the following risks - If the non-determinism shows up in a way that affects developers during development - If there is a reason the user wants to control target order for explicit targets Fixes rust-lang#13988
fix(toml): Ensure targets are in a deterministic order ### What does this PR try to resolve? With #13713, we enumerate all targets in `Cargo.toml` on `cargo publish` and `cargo vendor`. However, the order of the targets is non-determistic. This can be annoying for comparing the published `Cargo.toml` across releases but even worse is the churn it causes for `cargo vendor`. So we sort all the targets during publish. This keeps costs minimal with the following risks - If the non-determinism shows up in a way that affects developers during development - If there is a reason the user wants to control target order for explicit targets Fixes #13988 ### How should we test and review this PR? As for writing of tests, I'm unsure why none of our existing tests failed which makes it unclear to me what would be needed to write a test for this. ### Additional information
FYI: This PR might have introduced a "warning" regression: #14290. |
What does this PR try to resolve?
We have a couple of problems with publishing
package
doesn't verify is missingpath
, it will error, while one withpath
won't, see cargo publish has bad error message when explicit[[bench]]
isn't inpackage.include
#13456required-features
, I then could no longer exclude themCargo.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
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 discoveryResolved risks
Cargo.toml
#13729 ensured our generated target paths don't have\
in themCargo.toml
#13729 ensures the paths are normalize so the list of packaged pathsFor 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 apath
.We cannot turn off auto-discovery of libs, so that will always be done for bin-only packages.