include dotfiles in packages #7680
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
Thanks for the PR! I read it to be only ones explicitly in the |
got rid of extra test for dotfiles
|
I tried to implement a variant where dot-files/dirs are excluded by default (current behavior) and found some other unexpected behavior which is not documented here
the documentation does not mention a different behavior when a git repo is present nor does it mention that dot-files/dirs are always ignored when there is not git repo present. |
|
Yea, I would expect if a dotfile is listed in the
It does say "If git is being used for a package, the exclude field will be seeded with the gitignore settings from the repository." It definitely would be good to mention the dot exclusion behavior when git is not used. |
With my latest changes dotfiles in then
yes, that's what I meant, this behavior change is not mentioned in the documentation. |
src/cargo/sources/path.rs
Outdated
| let root = pkg.root(); | ||
| let mut exclude_dot_files_dir_builder = GitignoreBuilder::new(root); | ||
| exclude_dot_files_dir_builder.add_line(None, ".*")?; | ||
| exclude_dot_files_dir_builder.add_line(None, "*/.*/*")?; |
There was a problem hiding this comment.
good catch, it is already covered with the other pattern
|
Seems ok to me, would like to see if @alexcrichton has any opinion. |
|
I don't really remember the reason for original inclusion of this rule, I'd sort of expect that if git is present we use that exclusively to guide what files are included (dotfiles or not) and only if that's not present do we try to have some other heuristics. Directives like |
|
@bors: r=ehuss |
|
📌 Commit 5efda6a has been approved by |
bors
added
S-waiting-on-bors
include dotfiles in packages This PR solves #7183 It changes the behavior of `cargo package` to also include dotfiles by default. It should be discussed if this is intended or if the implementation should be changed to only include dotfiles which are specified in the `include` section. From the [existing comment](https://github.com/stefanhoelzl/cargo/blob/40885dfab40a1bf62b22aa03f732ef45163c013f/src/cargo/sources/path.rs#L358) it is a little bit unclear to me, but I supposed it was intended only to exclude directories starting with a dot?
|
☀️ Test successful - checks-azure |
Respect Cargo.toml `[package.exclude]` even not in a git repo. May resolves #9054 This bug (or feature?) has been lingering for a while. #7680 fixed the `cargo package` part but `cargo vendor` is still affected by the heuristic rule of ignoring dotfiles. ~~I propose to drop the rule and include dotfiles by default even if the package is not under git-controlled~~. See below. ## Updated: Changes Summary `cargo vendor` vendors dependencies without git-controlled but `cargo package` often runs under a VCS like git. [These lines](https://github.com/rust-lang/cargo/blob/1ca930b/src/cargo/sources/path.rs#L161-L168) are where they diverges: `fn list_files_walk_except_dot_files_and_dirs` builds [its own ignore instance], which cannot merge with other filter rules from `[package.exclude]`. This causes some patterns to not work as expected, such as re-including file after ignoring dotfiles `[.*, !negated_file]`. To make re-include (negate) rule works, this patch adds the excluding dotfiles rule directly into the `package.exclude` ignore instance if **_no include option nor git repo exists_**. Other old behaviors should not change in this patch. [its own ignore instance]: https://github.com/rust-lang/cargo/blob/1ca930b6/src/cargo/sources/path.rs#L364-L366
This PR solves #7183
It changes the behavior of
cargo packageto also include dotfiles by default.It should be discussed if this is intended or if the implementation should be changed to only include dotfiles which are specified in the
includesection.From the existing comment it is a little bit unclear to me, but I supposed it was intended only to exclude directories starting with a dot?