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

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

Merged
merged 1 commit into from Jul 18, 2017

Conversation

Projects
None yet
4 participants
@behnam
Contributor

behnam commented Jul 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jul 11, 2017

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.

rust-highfive commented Jul 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Jul 11, 2017

Contributor

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.

Contributor

behnam commented Jul 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Jul 11, 2017

Contributor

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.

Contributor

behnam commented Jul 11, 2017

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.

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

This comment has been minimized.

@alexcrichton

alexcrichton Jul 11, 2017

Member

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

@alexcrichton

alexcrichton Jul 11, 2017

Member

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

This comment has been minimized.

@behnam

behnam Jul 11, 2017

Contributor

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.

@behnam

behnam Jul 11, 2017

Contributor

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.

This comment has been minimized.

@alexcrichton

alexcrichton Jul 12, 2017

Member

Ah yes, my mistake!

@alexcrichton

alexcrichton Jul 12, 2017

Member

Ah yes, my mistake!

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

This comment has been minimized.

@alexcrichton

alexcrichton Jul 11, 2017

Member

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

@alexcrichton

alexcrichton Jul 11, 2017

Member

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

This comment has been minimized.

@behnam

behnam Jul 11, 2017

Contributor

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.

@behnam

behnam Jul 11, 2017

Contributor

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.

This comment has been minimized.

@behnam

behnam Jul 11, 2017

Contributor

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.

@behnam

behnam Jul 11, 2017

Contributor

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.

This comment has been minimized.

@alexcrichton

alexcrichton Jul 12, 2017

Member

Ok thanks for checking, sounds good to me!

@alexcrichton

alexcrichton Jul 12, 2017

Member

Ok thanks for checking, sounds good to me!

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 12, 2017

Member

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

Member

alexcrichton commented Jul 12, 2017

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

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 12, 2017

Contributor

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

Contributor

bors commented Jul 12, 2017

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

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

@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Jul 18, 2017

Contributor

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!

Contributor

behnam commented Jul 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Jul 18, 2017

Contributor

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...

Contributor

behnam commented Jul 18, 2017

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...

[sources/path] Add gitignore-like pattern matching and warn on mismat…
…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: <#4268>
// 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()));

This comment has been minimized.

@behnam

behnam Jul 18, 2017

Contributor

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?

@behnam

behnam Jul 18, 2017

Contributor

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?

This comment has been minimized.

@alexcrichton

alexcrichton Jul 18, 2017

Member

Nah yeah seems fine by me!

@alexcrichton

alexcrichton Jul 18, 2017

Member

Nah yeah seems fine by me!

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 18, 2017

Member

@bors: r+

Thanks @behnam!

Member

alexcrichton commented Jul 18, 2017

@bors: r+

Thanks @behnam!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 18, 2017

Contributor

📌 Commit c072ba4 has been approved by alexcrichton

Contributor

bors commented Jul 18, 2017

📌 Commit c072ba4 has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 18, 2017

Contributor

⌛️ Testing commit c072ba4 with merge 8bbb703...

Contributor

bors commented Jul 18, 2017

⌛️ Testing commit c072ba4 with merge 8bbb703...

bors added a commit that referenced this pull request Jul 18, 2017

Auto merge of #4270 - behnam:ignore, r=alexcrichton
[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

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 18, 2017

Contributor

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

Contributor

bors commented Jul 18, 2017

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

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@behnam behnam deleted the behnam:ignore branch Jul 18, 2017

@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Jul 18, 2017

Contributor

Tracking issue has the goal and roadmap now: #4268

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

Contributor

behnam commented Jul 18, 2017

Tracking issue has the goal and roadmap now: #4268

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

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 3, 2017

Update to 1.21.0
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment