fix(package): Normalize paths in Cargo.toml
#13729
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
A-manifest
|
Would it be worth asserting that |
I had wondered about something along those lines but then almost everything else will already be broken in cargo that I shrugged my shoulders. For example, every dependency you try to use will have |
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.
A windows user could use `\` and no Linux or Mac user could use the package. This normalizes the separator to what works on all platforms.
epage
commented
Apr 10, 2024
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 10, 2024
Update cargo 11 commits in 28e7b2bc0a812f90126be30f48a00a4ada990eaa..74fd5bc730b828dbc956335b229ac34ba47f7ef7 2024-04-05 19:31:01 +0000 to 2024-04-10 18:40:49 +0000 - chore: downgrade to openssl v1.1.1 (again) (rust-lang/cargo#13731) - fix(cargo-fix): dont apply same suggestion twice (rust-lang/cargo#13728) - refactor: make `resolve_with_previous` clearer (rust-lang/cargo#13727) - fix(package): Normalize paths in `Cargo.toml` (rust-lang/cargo#13729) - refactor: Track when MSRV is explicitly set, either way (rust-lang/cargo#13732) - [fix]:Build script not rerun when target rustflags change (rust-lang/cargo#13560) - feat(add): Stabilize MSRV-aware version req selection (rust-lang/cargo#13608) - Fix github fast path redirect. (rust-lang/cargo#13718) - Add release notes for 1.77.1 (rust-lang/cargo#13717) - doc(semver): remove mention of deprecated tool rust-semverver (rust-lang/cargo#13715) - chore: fix some typos (rust-lang/cargo#13714) r? ghost
bors
added a commit
that referenced
this pull request
Apr 29, 2024
fix(toml): Warn, rather than fail publish, if a target is excluded ### 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 #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 (#5335) but were insta-stable. `autobins = false` did nothing until 1.32 (#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 - #13729 ensured our generated target paths don't have `\` in them - #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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR try to resolve?
On the surface, this resolves problems that aren't too big of a deal
.////.//foo.rsbeingfoo.rs) which is just visual\in them to/However, this is prep for #13713 where these will be a much bigger deal
./foo.rs, we might fail to compare that with the list of files included in the package\and need to make sure the packages can still be used on WindowsHow should we test and review this PR?
Additional information