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

Change Cargo include/exclude rules to gitignore patterns #4268

Open
behnam opened this Issue Jul 10, 2017 · 37 comments

Comments

Projects
None yet
8 participants
@behnam
Contributor

behnam commented Jul 10, 2017

Based on discussion on this PR: #4256

Current status on master: Stage 1.1.

FAQ

How to migrate?

If you are receiving warnings about some include/exclude pattern (like xyz) matching new files in the future, and that's not what you want, please consider adding a leading slash to the pattern (xyz becomes /xyz). (Pending on PR: #4378)

Got problems with the Warnings?

If you are here because of a cargo warning that doesn't look right, or you cannot find a fix that works in both old (current) and new interpretations, please file an issue, with content of the package.include/package.exclude configs in your manifest file, as well as a copy of the warnings you have received.

Also, don't forget to put a link to this issue in your report, and mention the POC (@behnam) in the issue.

Goal

The current interpretation of Cargo's package.include and package.exclude configs is based on UNIX Globs, as implemented in the glob crate. Now we want these configs to work as similar to gitignore as possible. The gitignore specification is also based on Globs, but has a bunch of additional features that enable easier pattern writing and more control. Therefore, we are migrating the interpretation for the rules of these configs to use the ignore crate, and treat each include/exclude rule as a single line in a gitignore file.

Major features supported by the new (gitignore-like) interpretation are:

  • Allow matching patterns fixed on the package root by using a leading slash (/) character.

    For example, exclude = [ "/data" ] will exclude file/directory named data under the root package directory. No other file/directory named data (like, /src/data) will be excluded.

  • Matching all paths under a directory by only naming the directory itself.

    For example, you can have include = [ "/src" ] instead of include = [ "/src/*" ] or include = [ "/src/**" ].

  • Support patterns with a trailing slash (/), which mean only match directories.

    For example, you can have exclude = [ "tables/" ] to exclude all directories named tables, with no effect on files.

Migration Stages

Stage 1. Implement new interpretation of include/exclude rules (the gitignore sytanx), compare the results with existing method, and warn if they don't agree on paths.

Also, error on any usage of negated patterns (rules staring with !), as they are treated literally at the moment, and will get a special meaning after the switch, which we want disabled by default. (! is the only new special character.)

Stage 1.1. Add leading-slash support to existing glob-based matching, to enable migration for patterns relying on matching the items in root. (Like "src/**/*", which should match other directories named src, except in the root.)

Stage 2. After at least one release and updating the docs, change the default to the new approach; and still warn if the new approach doesn't agree with the old approach.

Stage 3. In a later release, drop the old approach.

Possible Follow-ups

  • Enable negated patterns (rules staring with !) for both include/exclude rules. This gives more control to users.

  • Allow having both include/exclude rules at the same time. Not exactly related to this change, but easy and intuitive to do after these changes.

  • If needed, update workspace.exclude to stay close to how package.exclude works.

Fixes #3578

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 11, 2017

[path] Add gitignore-like logic and warn on mismatches
Add gitignore-like pattern matching logic to `list_files()` and set
warning for paths getting different inclusion/exclusion results with the
old and new methods.

Migration Tracking: <rust-lang#4268>

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 11, 2017

[path] Add gitignore-like logic and warn on mismatches
Add gitignore-like pattern matching logic to `list_files()` and set
warning for paths getting different inclusion/exclusion results with the
old and new methods.

Migration Tracking: <rust-lang#4268>

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 11, 2017

WIP: [path] Add gitignore-like logic and warn on mismatches
NOTE: Lines with a `XXX` marker denote unexpected result which need to
be addressed and documented before merging.

Add gitignore-like pattern matching logic to `list_files()` and set
warning for paths getting different inclusion/exclusion results with the
old and new methods.

Migration Tracking: <rust-lang#4268>
@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.

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 13, 2017

WIP: [path] Add gitignore-like logic and warn on mismatches
NOTE: Lines with a `XXX` marker denote unexpected result which need to
be addressed and documented before merging.

Add gitignore-like pattern matching logic to `list_files()` and set
warning for paths getting different inclusion/exclusion results with the
old and new methods.

Migration Tracking: <rust-lang#4268>

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 13, 2017

WIP: [path] Add gitignore-like logic and warn on mismatches
NOTE: Lines with a `XXX` marker denote unexpected result which need to
be addressed and documented before merging.

Add gitignore-like pattern matching logic to `list_files()` and set
warning for paths getting different inclusion/exclusion results with the
old and new methods.

Migration Tracking: <rust-lang#4268>

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 13, 2017

WIP: [path] Add gitignore-like logic and warn on mismatches
NOTE: Need to wait for <BurntSushi/ripgrep#554>
to land and get `ignore:0.2.2` released.

Add gitignore-like pattern matching logic to `list_files()` and set
warning for paths getting different inclusion/exclusion results with the
old and new methods.

Migration Tracking: <rust-lang#4268>

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 18, 2017

WIP: [path] Add gitignore-like logic and warn on mismatches
NOTE: Need to wait for <BurntSushi/ripgrep#554>
to land and get `ignore:0.2.2` released.

Add gitignore-like pattern matching logic to `list_files()` and set
warning for paths getting different inclusion/exclusion results with the
old and new methods.

Migration Tracking: <rust-lang#4268>

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 18, 2017

WIP: [path] Add gitignore-like logic and warn on mismatches
NOTE: Need to wait for <BurntSushi/ripgrep#554>
to land and get `ignore:0.2.2` released.

Add gitignore-like pattern matching logic to `list_files()` and set
warning for paths getting different inclusion/exclusion results with the
old and new methods.

Migration Tracking: <rust-lang#4268>

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 18, 2017

[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: <rust-lang#4268>

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 18, 2017

[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: <rust-lang#4268>

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 18, 2017

[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: <rust-lang#4268>

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 18, 2017

[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: <rust-lang#4268>

bors added a commit that referenced this issue 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>

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 18, 2017

behnam added a commit to behnam/rust-cargo that referenced this issue Jul 19, 2017

bors added a commit that referenced this issue Jul 19, 2017

Auto merge of #4298 - behnam:ignore, r=alexcrichton
[src/doc/manifest] Add section on Migrating to `gitignore`-like pattern matching

Tracking issue: #4268
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 19, 2017

Member

When munging around in the manifests today I realized that we've got another place where we're using the glob crate but probably shouldn't be. The workspace::expand_member_path function uses glob to expand glob inputs and it's actually a bug that the is_excluded function doesn't respect globs, but that's being fixed in #4297 (review)

Member

alexcrichton commented Jul 19, 2017

When munging around in the manifests today I realized that we've got another place where we're using the glob crate but probably shouldn't be. The workspace::expand_member_path function uses glob to expand glob inputs and it's actually a bug that the is_excluded function doesn't respect globs, but that's being fixed in #4297 (review)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 19, 2017

Member

@behnam would you be interested in looking to remove the usage of glob in workspace interpretation? I found it to be somewhat nontrivial but I don't think we'll have many backwards compatibility concerns as it was very very recently added.

Member

alexcrichton commented Jul 19, 2017

@behnam would you be interested in looking to remove the usage of glob in workspace interpretation? I found it to be somewhat nontrivial but I don't think we'll have many backwards compatibility concerns as it was very very recently added.

@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Jul 19, 2017

Contributor

Right, @alexcrichton. I added that to the follow up list yesterday and was thinking to get to it when getting to Stage 2. But thinking about it again, we can add a similar warning mechanism there and move forward changes on workspace.exclude (and related configs) alongside the current plan for package.exclude (and friends).

So, yes, I'll start working on it later today.

Contributor

behnam commented Jul 19, 2017

Right, @alexcrichton. I added that to the follow up list yesterday and was thinking to get to it when getting to Stage 2. But thinking about it again, we can add a similar warning mechanism there and move forward changes on workspace.exclude (and related configs) alongside the current plan for package.exclude (and friends).

So, yes, I'll start working on it later today.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 20, 2017

Member

Thanks @behnam! Although I still need to get #4297 landed...

Member

alexcrichton commented Jul 20, 2017

Thanks @behnam! Although I still need to get #4297 landed...

@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Jul 24, 2017

Contributor

Sure, @alexcrichton. Please go ahead and land #4297.

Getting gitignore-like matching work for workspaces is a bit more complicated than for package include/exclude.

In package include/exclude, all paths are guaranteed to be a child of the root path, which is where the cargo manifest sits. This is similar to gitignore behavior, where rules only apply to children paths of some root, which is where the .gitignore file sits, or the git repo root. That's why I have hard-coded the assumption as debug-assertion in the matching logic: BurntSushi/ripgrep#551.

In workspace members/exclude configs—similar to package dependencies—the path can point to directories outside of the package root, usually a sibling directory. If we want to change their interpretation to gitignore-like matching, we need to first specify the behavior.

The new behavior may not be hard to define, but it would be something new that we need to keep track of. Also, there would be a question of where it belongs to the ripgrep implementation, or should we hard-code this special case in a local abstraction in cargo.

I'm still pondering about how to implement it. Would be glad to hear your thoughts on this, too.

Contributor

behnam commented Jul 24, 2017

Sure, @alexcrichton. Please go ahead and land #4297.

Getting gitignore-like matching work for workspaces is a bit more complicated than for package include/exclude.

In package include/exclude, all paths are guaranteed to be a child of the root path, which is where the cargo manifest sits. This is similar to gitignore behavior, where rules only apply to children paths of some root, which is where the .gitignore file sits, or the git repo root. That's why I have hard-coded the assumption as debug-assertion in the matching logic: BurntSushi/ripgrep#551.

In workspace members/exclude configs—similar to package dependencies—the path can point to directories outside of the package root, usually a sibling directory. If we want to change their interpretation to gitignore-like matching, we need to first specify the behavior.

The new behavior may not be hard to define, but it would be something new that we need to keep track of. Also, there would be a question of where it belongs to the ripgrep implementation, or should we hard-code this special case in a local abstraction in cargo.

I'm still pondering about how to implement it. Would be glad to hear your thoughts on this, too.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 25, 2017

Member

Hm yeah I wasn't sure if this'd be a great application for it :(. I wonder if maybe just exclude should be gitignore-like and otherwise include shoudl be glob based?

Member

alexcrichton commented Jul 25, 2017

Hm yeah I wasn't sure if this'd be a great application for it :(. I wonder if maybe just exclude should be gitignore-like and otherwise include shoudl be glob based?

@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Jul 26, 2017

Contributor

otherwise include should be glob based?

You mean workspace.members, right? Yes, I think globs make much more sense in that case. They are supposed to match directories (and not files), and they are usually expected to list patterns matching non-children paths.

Then, we can limit workspace.exclude to only paths under the root, since that's where auto-inclusion happens. And that resolves the aforementioned matching design issue.

From How to teach this? perspective, I think we can be more explicit with the package root directory concept, and that's the only place with auto-inclusion of files and packages. And, we can also say that in all package.include, package.exclude, workspace.exclude options, the patterns match against this package root directory.

Contributor

behnam commented Jul 26, 2017

otherwise include should be glob based?

You mean workspace.members, right? Yes, I think globs make much more sense in that case. They are supposed to match directories (and not files), and they are usually expected to list patterns matching non-children paths.

Then, we can limit workspace.exclude to only paths under the root, since that's where auto-inclusion happens. And that resolves the aforementioned matching design issue.

From How to teach this? perspective, I think we can be more explicit with the package root directory concept, and that's the only place with auto-inclusion of files and packages. And, we can also say that in all package.include, package.exclude, workspace.exclude options, the patterns match against this package root directory.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 26, 2017

Member

Ah yes indeed! That all makes sense to me!

Member

alexcrichton commented Jul 26, 2017

Ah yes indeed! That all makes sense to me!

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Jul 30, 2017

Member

I currently have include = ["src/**/*", ...] in my Cargo.toml. How do I modify this to have the same behavior under both the old rules and the new rules. Also would be nice to not be spammed with warnings every single time I build.

Member

retep998 commented Jul 30, 2017

I currently have include = ["src/**/*", ...] in my Cargo.toml. How do I modify this to have the same behavior under both the old rules and the new rules. Also would be nice to not be spammed with warnings every single time I build.

@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Jul 30, 2017

Contributor

@retep998, could you please also post the path resulting in the warning (or the full warning)?

Contributor

behnam commented Jul 30, 2017

@retep998, could you please also post the path resulting in the warning (or the full warning)?

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Jul 30, 2017

Member
PS C:\Users\Peter\Code\winapi-rs> cargo build
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\d3d12\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\d3dcompiler\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\dbghelp\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\dxguid\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\hid\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\httpapi\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\kernel32\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\oleaut32\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\opengl32\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\runtimeobject\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\setupapi\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\shell32\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\user32\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\winhttp\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\winmm\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\winscard\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\winusb\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
Member

retep998 commented Jul 30, 2017

PS C:\Users\Peter\Code\winapi-rs> cargo build
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\d3d12\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\d3dcompiler\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\dbghelp\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\dxguid\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\hid\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\httpapi\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\kernel32\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\oleaut32\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\opengl32\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\runtimeobject\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\setupapi\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\shell32\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\user32\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\winhttp\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\winmm\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\winscard\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `lib\winusb\src\lib.rs` WILL be included in the next Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Jul 30, 2017

Contributor

Coo! So, what's happening is that the src in your rule is matching any directory named src in the tree, even the ones under /lib/. Your fix would be to add a leading slash (/) to the pattern, making it:

include = ["/src/**/*", ...]

or, just a simpler format:

include = ["/src/**", ...]

Does this help, @retep998?

Contributor

behnam commented Jul 30, 2017

Coo! So, what's happening is that the src in your rule is matching any directory named src in the tree, even the ones under /lib/. Your fix would be to add a leading slash (/) to the pattern, making it:

include = ["/src/**/*", ...]

or, just a simpler format:

include = ["/src/**", ...]

Does this help, @retep998?

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Jul 30, 2017

Member

@behnam No, because a leading slash is not understood by the old system which causes an even longer wall of warnings. I want something that is compatible with both the old and new and doesn't trigger this wall of text.

Member

retep998 commented Jul 30, 2017

@behnam No, because a leading slash is not understood by the old system which causes an even longer wall of warnings. I want something that is compatible with both the old and new and doesn't trigger this wall of text.

@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Jul 30, 2017

Contributor

Right, @retep998. I'm afraid this is one of those cases that we've missed in the migration plan. Unfortunately, for this setup, I can't think of any way to get the include pattern to work on both current logic and the future one.

One question for you, @retep998: would switching to exclude rule would be an option for you? If not, could you elaborate?

Also, please note that this is just a warning and the logic has not changed yet. So, the packaging is not affected at the moment, and we will give you an option to update your manifest so that it works in both stable and nightly.

That said, we need to think of a way to address this for all users. Maybe we need to add the leading-slash logic first, as a 1.1 stage, and then completely switch to gitignore in another release after that. What do you think, @alexcrichton?

Contributor

behnam commented Jul 30, 2017

Right, @retep998. I'm afraid this is one of those cases that we've missed in the migration plan. Unfortunately, for this setup, I can't think of any way to get the include pattern to work on both current logic and the future one.

One question for you, @retep998: would switching to exclude rule would be an option for you? If not, could you elaborate?

Also, please note that this is just a warning and the logic has not changed yet. So, the packaging is not affected at the moment, and we will give you an option to update your manifest so that it works in both stable and nightly.

That said, we need to think of a way to address this for all users. Maybe we need to add the leading-slash logic first, as a 1.1 stage, and then completely switch to gitignore in another release after that. What do you think, @alexcrichton?

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Jul 30, 2017

Member

I prefer include over exclude because with an explicit whitelist I never have to worry about files accidentally sneaking in. If I use a blacklist then when I create a temporary file to check something there is the risk of that file slipping into the packages I publish. With a whitelist I just specify what I want in the published package, with a blacklist I have to prepare for every contingency.

Member

retep998 commented Jul 30, 2017

I prefer include over exclude because with an explicit whitelist I never have to worry about files accidentally sneaking in. If I use a blacklist then when I create a temporary file to check something there is the risk of that file slipping into the packages I publish. With a whitelist I just specify what I want in the published package, with a blacklist I have to prepare for every contingency.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 31, 2017

Member

Maybe we need to add the leading-slash logic first, as a 1.1 stage, and then completely switch to gitignore in another release after that. What do you think, @alexcrichton?

@behnam sounds like a great idea to me!

Member

alexcrichton commented Jul 31, 2017

Maybe we need to add the leading-slash logic first, as a 1.1 stage, and then completely switch to gitignore in another release after that. What do you think, @alexcrichton?

@behnam sounds like a great idea to me!

bors added a commit that referenced this issue Aug 8, 2017

Auto merge of #4378 - behnam:ignore, r=alexcrichton
[sources/path] Support leading slash in glob patterns

Background: #4268

This diff takes us to **Stage 1.1** of the migration plan by allowing
glob patterns to include a leading slash, so that glob patterns can be
updated, if needed, to start with a slash, closer to the future behavior
with gitignore-like matching.

Why is this stage needed?

It's common to have `package.include` set like this:
```
include = ["src/**"]
```
In old interpretation, this would only include all files under the `src`
directory under the package root. With the new interpretation, this
would match any path with some directory called `src`, even if it's not
directly under the package root.

After this patch, package owners can start marking glob patters with a
leading slash to fix the warning thrown, if any.

One thing to notice here is that there are no extra matchings, but, if
a manifest has already a pattern with a leading slash, this would
silently start matching it with the paths. I believe this is fine, since
the old behavior would have been for the pattern to not match anything,
therefore the item was useless.

See also <#4377> for suggestion
to throw warning on useless/invalid patterns in these fields.
@kornelski

This comment has been minimized.

Show comment
Hide comment
@kornelski

kornelski Aug 8, 2017

Contributor

The new patterns are better, but I've ran into a problem. It doesn't seem to support .git/info/exclude. This file is a hidden addition to .gitignore. Previously Cargo supported it (probably as a side effect of using git to list files?), but now I'm getting warnings that files excluded in .git/info/exclude will be included.

I use .git/info/exclude for my very private, machine-specific garbage that I have in my project directories (text editor config, unfinished code files, inputs/outputs I use for manual testing). These are usually not worth including in publicly-visible .gitignore, and definitely should not be published.

Contributor

kornelski commented Aug 8, 2017

The new patterns are better, but I've ran into a problem. It doesn't seem to support .git/info/exclude. This file is a hidden addition to .gitignore. Previously Cargo supported it (probably as a side effect of using git to list files?), but now I'm getting warnings that files excluded in .git/info/exclude will be included.

I use .git/info/exclude for my very private, machine-specific garbage that I have in my project directories (text editor config, unfinished code files, inputs/outputs I use for manual testing). These are usually not worth including in publicly-visible .gitignore, and definitely should not be published.

@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Aug 8, 2017

Contributor

Thanks for the feedback, @pornel. Let's look at the two different areas you have mentioned:

  1. Cargo getting default inclusion list based on VCS, like git. In this area, this plan has no effect and everything should be the same as before, unless another issue/PR is changing that, or the git2 crate has changed its behavior.

  2. Cargo interpreting include/exclude rules similar to what gitignore does. This is the scope of this issue, and what's being changed here.

I believe the warnings you're receiving are a result of (2), but somehow appear to be a result of changes in (1). If you share your package.exclude config and the warnings, I can look at what's happening.

Contributor

behnam commented Aug 8, 2017

Thanks for the feedback, @pornel. Let's look at the two different areas you have mentioned:

  1. Cargo getting default inclusion list based on VCS, like git. In this area, this plan has no effect and everything should be the same as before, unless another issue/PR is changing that, or the git2 crate has changed its behavior.

  2. Cargo interpreting include/exclude rules similar to what gitignore does. This is the scope of this issue, and what's being changed here.

I believe the warnings you're receiving are a result of (2), but somehow appear to be a result of changes in (1). If you share your package.exclude config and the warnings, I can look at what's happening.

bors added a commit that referenced this issue Aug 9, 2017

Auto merge of #4378 - behnam:ignore, r=alexcrichton
[sources/path] Support leading slash in glob patterns

Background: #4268

This diff takes us to **Stage 1.1** of the migration plan by allowing
glob patterns to include a leading slash, so that glob patterns can be
updated, if needed, to start with a slash, closer to the future behavior
with gitignore-like matching.

Why is this stage needed?

It's common to have `package.include` set like this:
```
include = ["src/**"]
```
In old interpretation, this would only include all files under the `src`
directory under the package root. With the new interpretation, this
would match any path with some directory called `src`, even if it's not
directly under the package root.

After this patch, package owners can start marking glob patters with a
leading slash to fix the warning thrown, if any.

One thing to notice here is that there are no extra matchings, but, if
a manifest has already a pattern with a leading slash, this would
silently start matching it with the paths. I believe this is fine, since
the old behavior would have been for the pattern to not match anything,
therefore the item was useless.

See also <#4377> for suggestion
to throw warning on useless/invalid patterns in these fields.
@kornelski

This comment has been minimized.

Show comment
Hide comment
@kornelski

kornelski Aug 9, 2017

Contributor

@behnam To reproduce:

cargo new foo; cd foo
mkdir rubbish
touch somefile rubbish/somefile
echo rubbish/ >> .git/info/exclude

and add include = ["somefile"] to Cargo.toml

I'm getting:

warning: Pattern matching for Cargo's include/exclude fields is changing and file rubbish/somefile WILL be included in the next Cargo version.

As of cargo 0.22.0-nightly (305bc25 2017-07-28) the rubbish directory is not included in the package.

Contributor

kornelski commented Aug 9, 2017

@behnam To reproduce:

cargo new foo; cd foo
mkdir rubbish
touch somefile rubbish/somefile
echo rubbish/ >> .git/info/exclude

and add include = ["somefile"] to Cargo.toml

I'm getting:

warning: Pattern matching for Cargo's include/exclude fields is changing and file rubbish/somefile WILL be included in the next Cargo version.

As of cargo 0.22.0-nightly (305bc25 2017-07-28) the rubbish directory is not included in the package.

@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Aug 9, 2017

Contributor

Thanks, @pornel. First, in Cargo's package table, if you have include field set, there will be absolutely no VCS-based pre-population. You can see the code here: https://github.com/rust-lang/cargo/blob/master/src/cargo/sources/path.rs#L246-L252

So, for this repro, basically, the git-prepopulation has not been working in the first place. The only thing changing is the behavior of pattern-matching, which used to only match something on the root, but now with gitignore-like matching (we really need a better name for this one!!!) it also matching any other file that's named something anywhere in under the package root.

And, yes, it's just a warning for now, because we want to make sure everyone's ready for the change.

With the next nightly that has #4378 landed, you will be able to change your pattern to /something and get it work as before, without any warning.

I'm also sending a patch for beta, so we can be sure we have this leading-slash solution out in rustc:1.20.0, so everyone can start fixing their patterns before the change takes affect.

Contributor

behnam commented Aug 9, 2017

Thanks, @pornel. First, in Cargo's package table, if you have include field set, there will be absolutely no VCS-based pre-population. You can see the code here: https://github.com/rust-lang/cargo/blob/master/src/cargo/sources/path.rs#L246-L252

So, for this repro, basically, the git-prepopulation has not been working in the first place. The only thing changing is the behavior of pattern-matching, which used to only match something on the root, but now with gitignore-like matching (we really need a better name for this one!!!) it also matching any other file that's named something anywhere in under the package root.

And, yes, it's just a warning for now, because we want to make sure everyone's ready for the change.

With the next nightly that has #4378 landed, you will be able to change your pattern to /something and get it work as before, without any warning.

I'm also sending a patch for beta, so we can be sure we have this leading-slash solution out in rustc:1.20.0, so everyone can start fixing their patterns before the change takes affect.

@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Aug 10, 2017

Contributor

Update: #4270 is not yet in the beta channel (rust-1.20.0 branch), so there's no need to backport #4378. Both are available on nightly now, which makes it stable enough for wait until one or two release cycles.

Contributor

behnam commented Aug 10, 2017

Update: #4270 is not yet in the beta channel (rust-1.20.0 branch), so there's no need to backport #4378. Both are available on nightly now, which makes it stable enough for wait until one or two release cycles.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Aug 27, 2017

Member

I'm confused, I'm not using any include/exclude, but I get the following warnings:

warning: Pattern matching for Cargo's include/exclude fields is changing and file `src/bin/cargo-fmt.rs` WILL NOT be included in a future Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `src/bin/rustfmt-format-diff.rs` WILL NOT be included in a future Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `src/bin/rustfmt.rs` WILL NOT be included in a future Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info

The only parts of my Cargo.toml using naming those files is:

[[bin]]
name = "rustfmt"

[[bin]]
name = "cargo-fmt"

[[bin]]
name = "rustfmt-format-diff"

[features]
default = ["cargo-fmt", "rustfmt-format-diff"]
cargo-fmt = []
rustfmt-format-diff = []

Do I need to add some kind if include where previously they were implicitly included?

Link to the whole Cargo.toml

Member

nrc commented Aug 27, 2017

I'm confused, I'm not using any include/exclude, but I get the following warnings:

warning: Pattern matching for Cargo's include/exclude fields is changing and file `src/bin/cargo-fmt.rs` WILL NOT be included in a future Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `src/bin/rustfmt-format-diff.rs` WILL NOT be included in a future Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info
warning: Pattern matching for Cargo's include/exclude fields is changing and file `src/bin/rustfmt.rs` WILL NOT be included in a future Cargo version.
See https://github.com/rust-lang/cargo/issues/4268 for more info

The only parts of my Cargo.toml using naming those files is:

[[bin]]
name = "rustfmt"

[[bin]]
name = "cargo-fmt"

[[bin]]
name = "rustfmt-format-diff"

[features]
default = ["cargo-fmt", "rustfmt-format-diff"]
cargo-fmt = []
rustfmt-format-diff = []

Do I need to add some kind if include where previously they were implicitly included?

Link to the whole Cargo.toml

@retep998

This comment has been minimized.

Show comment
Hide comment
@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Aug 27, 2017

Member

Ah, doh! I am, guess I was misled by looking for the filenames, when it is the *.rs that is the issue

Member

nrc commented Aug 27, 2017

Ah, doh! I am, guess I was misled by looking for the filenames, when it is the *.rs that is the issue

@kornelski

This comment has been minimized.

Show comment
Hide comment
@kornelski

kornelski Nov 14, 2017

Contributor

Which stable release started supporting absolute paths in include? (I'm not sure if I can start switching to the new patterns without breaking users downstream)

Contributor

kornelski commented Nov 14, 2017

Which stable release started supporting absolute paths in include? (I'm not sure if I can start switching to the new patterns without breaking users downstream)

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Nov 14, 2017

Member

@pornel include/exclude only affects when you're doing cargo package so it should have no effect on downstream users of your crate. You just need to make sure include/exclude are set correctly for your current version of cargo when you publish your crate.

Member

retep998 commented Nov 14, 2017

@pornel include/exclude only affects when you're doing cargo package so it should have no effect on downstream users of your crate. You just need to make sure include/exclude are set correctly for your current version of cargo when you publish your crate.

SimonSapin added a commit to servo/skia that referenced this issue Dec 7, 2017

Fix cargo warning about exclude syntax
```
warning: Pattern matching for Cargo's include/exclude fields is changing and file `platform_tools/nacl/tests/index.html` WILL be excluded in a future Cargo version.
See rust-lang/cargo#4268 for more info
```

SimonSapin added a commit to servo/skia that referenced this issue Dec 7, 2017

Fix cargo warning about exclude syntax
```
warning: Pattern matching for Cargo's include/exclude fields is changing and file `platform_tools/nacl/tests/index.html` WILL be excluded in a future Cargo version.
See rust-lang/cargo#4268 for more info
```

@alexcrichton alexcrichton self-assigned this Jan 22, 2018

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 25, 2018

Member

I believe the current state of this issue is that we're continuing to let the warnings bake. I think there's still an issue with workspace include/exclude members but that's likely orthogonal to the packaging aspect of this issue in particular.

Member

alexcrichton commented Jan 25, 2018

I believe the current state of this issue is that we're continuing to let the warnings bake. I think there's still an issue with workspace include/exclude members but that's likely orthogonal to the packaging aspect of this issue in particular.

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Feb 4, 2018

I have a pattern which will work with the new rules, but currently includes too many files. Is there a way I can shut the warnings off?

The pattern looks like /a/*/b and at the moment it (erroneously) matches /a/x/y/b, when I only want it to match /a/x/b

Diggsey commented Feb 4, 2018

I have a pattern which will work with the new rules, but currently includes too many files. Is there a way I can shut the warnings off?

The pattern looks like /a/*/b and at the moment it (erroneously) matches /a/x/y/b, when I only want it to match /a/x/b

@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Feb 5, 2018

Contributor

Interesting find, @Diggsey! I submitted a PR to add unit-tests for it in the grepset repo (BurntSushi/ripgrep#773).

Unfortunately, no, there are no flags to suppress the warnings. Maybe we should have built one, but I guess it's just too late now that we're getting ready to migrate.

In the meanwhile, with this broken behavior, looks like all you need to do is to replace /a/*/b patters with /a/**/b, which will be matching the same set of paths in both interpretations. What do you think?

Contributor

behnam commented Feb 5, 2018

Interesting find, @Diggsey! I submitted a PR to add unit-tests for it in the grepset repo (BurntSushi/ripgrep#773).

Unfortunately, no, there are no flags to suppress the warnings. Maybe we should have built one, but I guess it's just too late now that we're getting ready to migrate.

In the meanwhile, with this broken behavior, looks like all you need to do is to replace /a/*/b patters with /a/**/b, which will be matching the same set of paths in both interpretations. What do you think?

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Feb 5, 2018

Diggsey commented Feb 5, 2018

BurntSushi added a commit to BurntSushi/ripgrep that referenced this issue Feb 6, 2018

globset: add more tests for single-asterisk pattern
This adds a few tests that check for bugs reported here:
rust-lang/cargo#4268

The bugs reported in the aforementioned issue are probably caused by not
enabling the `literal_separator` option in `GlobBuilder`. Enabling that
in the tests under question fixes the issue.

Closes #773
@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad Apr 16, 2018

Member

@behnam curious, what is the current state of affairs here? I think we were intending to fix some rather bad bug with this (don't remember which one), but looks like this is stalled and the original bug is still present? It may be me just imaging things of course :-)

Member

matklad commented Apr 16, 2018

@behnam curious, what is the current state of affairs here? I think we were intending to fix some rather bad bug with this (don't remember which one), but looks like this is stalled and the original bug is still present? It may be me just imaging things of course :-)

@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Apr 17, 2018

Contributor

We haven't had a report of any issues for a couple of releases, so it should be safe now to flip the switch. @alexcrichton?

Contributor

behnam commented Apr 17, 2018

We haven't had a report of any issues for a couple of releases, so it should be safe now to flip the switch. @alexcrichton?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 17, 2018

Member

Sounds good to me!

Member

alexcrichton commented Apr 17, 2018

Sounds good to me!

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad Jul 20, 2018

Member

@behnam would you want to send the PR to change the behavior then?

Member

matklad commented Jul 20, 2018

@behnam would you want to send the PR to change the behavior then?

@behnam

This comment has been minimized.

Show comment
Hide comment
@behnam

behnam Jul 20, 2018

Contributor

I can do so in 3-4 weeks from now. If anyone else can do so earlier, please feel free to take over.

Side note: I think we want to still keep both logic and earn against mismatched, but updating the wording to denote the change.

Also need to update the docs at the same time as flipping the switch.

Contributor

behnam commented Jul 20, 2018

I can do so in 3-4 weeks from now. If anyone else can do so earlier, please feel free to take over.

Side note: I think we want to still keep both logic and earn against mismatched, but updating the wording to denote the change.

Also need to update the docs at the same time as flipping the switch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment