[sources/path] Add gitignore-like pattern matching and warn on mismatches #4270

behnam · 2017-07-11T02:27:01Z

Add gitignore-like pattern matching logic to list_files() and throw
warnings for paths getting different inclusion/exclusion results from
the old and the new methods.

Migration Tracking: <#4268>

rust-highfive · 2017-07-11T02:27:13Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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.

behnam · 2017-07-11T02:29:51Z

This is a WIP, @alexcrichton.

Writing extensive unit test for the exclude option, I noticed that there are surprising changes for some patterns. I'm still investigating them, but I'm guessing they could be expected gitignore-style behavior, or a bug in ignore crate. (If expected, it's going to be annoying for the migration.)

So, the details for the pattern matching needs some work. Would appreciate feedback on overall direction of the patch.

behnam · 2017-07-11T08:25:04Z

Turns out ripgrep's ignore also has some issues in edge cases. I have added integration tests that reveal the bugs: BurntSushi/ripgrep#551

I'm going to expand the test and cover more cases and make sure ignore package is in good shape before we move forward here.

alexcrichton · 2017-07-11T14:10:44Z

src/cargo/sources/path.rs

+
+        let glob_should_package = |relative_path: &Path| -> bool {
+            // include and exclude options are mutually exclusive.
+            if no_include_option {


I think this condition is different from before, right? The exclude array is now ignored if include is provided? (looks accidental?)

No, it's been in the code for a couple of years:
https://github.com/rust-lang/cargo/blob/master/src/cargo/sources/path.rs#L112-L113

And the docs also clearly say this: http://doc.crates.io/manifest.html#the-exclude-and-include-fields-optional

The options are mutually exclusive: setting include will override an exclude.

That's why I suggested (in #4268) to stop that after we're done with the other fixes, and have include work as the first step, and exclude as a second step.

Ah yes, my mistake!

alexcrichton · 2017-07-11T14:12:11Z

src/cargo/sources/path.rs

+
+        let mut exclude_builder = GitignoreBuilder::new(root);
+        for rule in pkg.manifest().exclude() {
+            exclude_builder.add_line(None, rule)?;


Just to double check, how can this fail? This may be an error we wish to handle if valid glob-crate patterns are invalid gitignore patterns right now

I have not looks at the details, but at the moment, I guess it's bad usage of special characters (which are *, ?, !), which, in most cases, result also in bad glob patterns. (Well, basically, every line of gitignore is a glob, with the addition of negation by leading !, which cannot fail.)

So, I don't think falling back to a Glob object helps much.

But, maybe we should just throw warnings here and move on. But, I think a bad include/exclude rule is bad enough to break the packaging step.

Also, I don't expect any error to be introduced with the migration. Still need to check if patterns with ! can throw errors. Let me take a look at that and get back here.

Okay, I checked and the only type of error returned here is from Glob (globset::Glob::Error), which happens in cases like ***. So, there's no reason to fallback here.

Ok thanks for checking, sounds good to me!

alexcrichton · 2017-07-12T14:10:28Z

This is looking great to me, feel free to just ping me when CI is passing and I'll r+!

bors · 2017-07-12T21:54:49Z

☔ The latest upstream changes (presumably #4275) made this pull request unmergeable. Please resolve the merge conflicts.

behnam · 2017-07-18T00:23:29Z

We have fixed ignore in version 0.2.2) and all the tests are passing now with expected behavior, which is, for exclude field, we will only have additional exclusion, but no patterns changing in the other direction.

Also, I have updated the warning text to inform users with the context of the change in a few words. Here's an example:

"Pattern matching for Cargo's include/exclude fields is changing and \
file `{}` WILL be excluded in the next Cargo version.\n\
See https://github.com/rust-lang/cargo/issues/4268 for more info",

I think we're good to go, if nothing goes wrong with CI. I will submit a diff for Stage 2—to be landed after the next release. Thanks, @alexcrichton, for all the prompt responses and awesome feedbacks!

behnam · 2017-07-18T00:28:10Z

Uh... because of list of paths not being deterministic, we're getting this kind of failures:

failures:
---- exclude stdout ----
	running `/home/travis/build/rust-lang/cargo/target/debug/cargo package --no-verify -v`
thread 'exclude' panicked at '
Expected: execs
    but: differences:
  3 - |[WARNING] [..] file `dir_root_1[/]some_dir[/]file` WILL be excluded [..]|
    + |warning: Pattern matching for Cargo's include/exclude fields is changing and file `dir_root_3/some_dir/file` WILL be excluded in the next Cargo version.|
  5 - |[WARNING] [..] file `file_root_2` WILL be excluded [..]|
    + |warning: Pattern matching for Cargo's include/exclude fields is changing and file `dir_root_2/some_dir/file` WILL be excluded in the next Cargo version.|
  7 - |[WARNING] [..] file `dir_root_2[/]some_dir[/]file` WILL be excluded [..]|
    + |warning: Pattern matching for Cargo's include/exclude fields is changing and file `dir_root_1/some_dir/file` WILL be excluded in the next Cargo version.|
  9 - |[WARNING] [..] file `some_dir[/]dir_deep_1[/]some_dir[/]file` WILL be excluded [..]|
    + |warning: Pattern matching for Cargo's include/exclude fields is changing and file `some_dir/dir_deep_5/some_dir/file` WILL be excluded in the next Cargo version.|
 13 - |[WARNING] [..] file `some_dir[/]dir_deep_5[/]some_dir[/]file` WILL be excluded [..]|
    + |warning: Pattern matching for Cargo's include/exclude fields is changing and file `some_dir/dir_deep_1/some_dir/file` WILL be excluded in the next Cargo version.|
 19 - |[WARNING] [..] file `dir_root_3[/]some_dir[/]file` WILL be excluded [..]|
    + |warning: Pattern matching for Cargo's include/exclude fields is changing and file `file_root_2` WILL be excluded in the next Cargo version.|
other output:
``', /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/hamcrest-0.1.1/src/core.rs:31
note: Run with `RUST_BACKTRACE=1` for a backtrace.
failures:
    exclude

Not sure if it's worth trying to fix this by making the output be deterministic, or just use [..] for file names in the test cases. Looking into it...

…ches Add gitignore-like pattern matching logic to `list_files()` and throw warnings for paths getting different inclusion/exclusion results from the old and the new methods. Migration Tracking: <rust-lang#4268>

behnam · 2017-07-18T06:40:05Z

src/cargo/sources/path.rs

+        // See <https://github.com/rust-lang/cargo/issues/4268>
+        // and <https://github.com/rust-lang/cargo/pull/4270>
+        let mut entries: Vec<fs::DirEntry> = fs::read_dir(path)?.map(|e| e.unwrap()).collect();
+        entries.sort_by(|a, b| a.path().as_os_str().cmp(b.path().as_os_str()));


To get the warnings be thrown in a deterministic order, I have put this sort method here for the transition period. We can drop it as soon as we drop the checks for warnings.

This will have some extra heap allocation during packaging. However, it does not slow down any build or test functionalities. So, I think it's fine to pay this cost for a few months while making sure correct warnings are thrown for the big change coming.

What do you think, @alexcrichton? Does this sound like a reasonable compromise?

Nah yeah seems fine by me!

alexcrichton · 2017-07-18T16:04:09Z

@bors: r+

Thanks @behnam!

bors · 2017-07-18T16:04:10Z

📌 Commit c072ba4 has been approved by alexcrichton

bors · 2017-07-18T16:04:16Z

⌛ Testing commit c072ba4 with merge 8bbb703...

[sources/path] Add gitignore-like pattern matching and warn on mismatches Add gitignore-like pattern matching logic to `list_files()` and throw warnings for paths getting different inclusion/exclusion results from the old and the new methods. Migration Tracking: <#4268>

bors · 2017-07-18T18:20:43Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 8bbb703 to master...

behnam · 2017-07-18T23:32:11Z

Tracking issue has the goal and roadmap now: #4268

Next diff is out for adding a notice to the online guide: #4298

Changelog: Version 1.21.0 (2017-10-12) ========================== Language -------- - [You can now use static references for literals.][43838] Example: ```rust fn main() { let x: &'static u32 = &0; } ``` - [Relaxed path syntax. Optional `::` before `<` is now allowed in all contexts.][43540] Example: ```rust my_macro!(Vec<i32>::new); // Always worked my_macro!(Vec::<i32>::new); // Now works ``` Compiler -------- - [Upgraded jemalloc to 4.5.0][43911] - [Enabled unwinding panics on Redox][43917] - [Now runs LLVM in parallel during translation phase.][43506] This should reduce peak memory usage. Libraries --------- - [Generate builtin impls for `Clone` for all arrays and tuples that are `T: Clone`][43690] - [`Stdin`, `Stdout`, and `Stderr` now implement `AsRawFd`.][43459] - [`Rc` and `Arc` now implement `From<&[T]> where T: Clone`, `From<str>`, `From<String>`, `From<Box<T>> where T: ?Sized`, and `From<Vec<T>>`.][42565] Stabilized APIs --------------- [`std::mem::discriminant`] Cargo ----- - [You can now call `cargo install` with multiple package names][cargo/4216] - [Cargo commands inside a virtual workspace will now implicitly pass `--all`][cargo/4335] - [Added a `[patch]` section to `Cargo.toml` to handle prepublication dependencies][cargo/4123] [RFC 1969] - [`include` & `exclude` fields in `Cargo.toml` now accept gitignore like patterns][cargo/4270] - [Added the `--all-targets` option][cargo/4400] - [Using required dependencies as a feature is now deprecated and emits a warning][cargo/4364] Misc ---- - [Cargo docs are moving][43916] to [doc.rust-lang.org/cargo](https://doc.rust-lang.org/cargo) - [The rustdoc book is now available][43863] at [doc.rust-lang.org/rustdoc](https://doc.rust-lang.org/rustdoc) - [Added a preview of RLS has been made available through rustup][44204] Install with `rustup component add rls-preview` - [`std::os` documentation for Unix, Linux, and Windows now appears on doc.rust-lang.org][43348] Previously only showed `std::os::unix`. Compatibility Notes ------------------- - [Changes in method matching against higher-ranked types][43880] This may cause breakage in subtyping corner cases. [A more in-depth explanation is available.][info/43880] - [rustc's JSON error output's byte position start at top of file.][42973] Was previously relative to the rustc's internal `CodeMap` struct which required the unstable library `libsyntax` to correctly use. - [`unused_results` lint no longer ignores booleans][43728] [42565]: rust-lang/rust#42565 [42973]: rust-lang/rust#42973 [43348]: rust-lang/rust#43348 [43459]: rust-lang/rust#43459 [43506]: rust-lang/rust#43506 [43540]: rust-lang/rust#43540 [43690]: rust-lang/rust#43690 [43728]: rust-lang/rust#43728 [43838]: rust-lang/rust#43838 [43863]: rust-lang/rust#43863 [43880]: rust-lang/rust#43880 [43911]: rust-lang/rust#43911 [43916]: rust-lang/rust#43916 [43917]: rust-lang/rust#43917 [44204]: rust-lang/rust#44204 [cargo/4123]: rust-lang/cargo#4123 [cargo/4216]: rust-lang/cargo#4216 [cargo/4270]: rust-lang/cargo#4270 [cargo/4335]: rust-lang/cargo#4335 [cargo/4364]: rust-lang/cargo#4364 [cargo/4400]: rust-lang/cargo#4400 [RFC 1969]: rust-lang/rfcs#1969 [info/43880]: rust-lang/rust#44224 (comment) [`std::mem::discriminant`]: https://doc.rust-lang.org/std/mem/fn.discriminant.html

Enable gitignore-like pattern matching for package's `include`/`exclude` rules, as suggested in rust-lang#4268 and initially implemented in rust-lang#4270. We still warn on projects affected by the divergent behavior. We can drop the warning and clean up the source and tests in one or more releases.

Remove include/exclude glob warning. This removes the warning when a package include/exclude pattern produces different results from glob vs gitignore. The pre-warnings were added in 1.21 (released 2017-10-12) #4270. The migration was done in 1.36 (released 2019-07-04) #6924. This will remove the warnings in 1.38 (to be released 2019-09-26). Closes #4268

rust-highfive assigned alexcrichton Jul 11, 2017

behnam mentioned this pull request Jul 11, 2017

[ignore] Add extensive test for gitignore matching BurntSushi/ripgrep#551

Merged

alexcrichton reviewed Jul 11, 2017

View reviewed changes

behnam force-pushed the ignore branch 7 times, most recently from 3943582 to d126bbc Compare July 18, 2017 00:08

behnam changed the title ~~WIP: [path] Add gitignore-like logic and warn on mismatches~~ [sources/path] Add gitignore-like pattern matching and warn on mismatches Jul 18, 2017

behnam force-pushed the ignore branch from d126bbc to d1579e1 Compare July 18, 2017 00:45

[sources/path] Add gitignore-like pattern matching and warn on mismat…

c072ba4

…ches Add gitignore-like pattern matching logic to `list_files()` and throw warnings for paths getting different inclusion/exclusion results from the old and the new methods. Migration Tracking: <rust-lang#4268>

behnam force-pushed the ignore branch from d1579e1 to c072ba4 Compare July 18, 2017 06:14

behnam commented Jul 18, 2017

View reviewed changes

bors merged commit c072ba4 into rust-lang:master Jul 18, 2017

behnam deleted the ignore branch July 18, 2017 22:00

alexcrichton added the relnotes Release-note worthy label Jul 18, 2017

behnam mentioned this pull request Aug 10, 2017

Change Cargo include/exclude rules to gitignore patterns #4268

Closed

alexcrichton mentioned this pull request Sep 11, 2017

Updated RELEASES.md for 1.21.0 rust-lang/rust#44481

Merged

ehuss mentioned this pull request Jul 23, 2019

Remove include/exclude glob warning. #7170

Merged

ehuss added this to the 1.21.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sources/path] Add gitignore-like pattern matching and warn on mismatches #4270

[sources/path] Add gitignore-like pattern matching and warn on mismatches #4270

behnam commented Jul 11, 2017 •

edited

Loading

rust-highfive commented Jul 11, 2017

behnam commented Jul 11, 2017

behnam commented Jul 11, 2017

alexcrichton Jul 11, 2017

behnam Jul 11, 2017

alexcrichton Jul 12, 2017

alexcrichton Jul 11, 2017

behnam Jul 11, 2017

behnam Jul 11, 2017

alexcrichton Jul 12, 2017

alexcrichton commented Jul 12, 2017

bors commented Jul 12, 2017

behnam commented Jul 18, 2017

behnam commented Jul 18, 2017

behnam Jul 18, 2017

alexcrichton Jul 18, 2017

alexcrichton commented Jul 18, 2017

bors commented Jul 18, 2017

bors commented Jul 18, 2017

bors commented Jul 18, 2017

behnam commented Jul 18, 2017

[sources/path] Add gitignore-like pattern matching and warn on mismatches #4270

[sources/path] Add gitignore-like pattern matching and warn on mismatches #4270

Conversation

behnam commented Jul 11, 2017 • edited Loading

rust-highfive commented Jul 11, 2017

behnam commented Jul 11, 2017

behnam commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jul 12, 2017

bors commented Jul 12, 2017

behnam commented Jul 18, 2017

behnam commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jul 18, 2017

bors commented Jul 18, 2017

bors commented Jul 18, 2017

bors commented Jul 18, 2017

behnam commented Jul 18, 2017

behnam commented Jul 11, 2017 •

edited

Loading