Cargo: Change Cargo include/exclude rules to gitignore patterns

[behnam]

Based on discussion on this PR: https://github.com/rust-lang/cargo/pull/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: https://github.com/rust-lang/cargo/pull/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 https://github.com/rust-lang/cargo/issues/3578

[behnam]

Turns out ripgrep's ignore also has some issues in edge cases. I have added integration tests that reveal the bugs: https://github.com/BurntSushi/ripgrep/pull/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]

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 https://github.com/rust-lang/cargo/pull/4297#pullrequestreview-50676432

[alexcrichton]

@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]

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]

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

[behnam]

Sure, @alexcrichton. Please go ahead and land https://github.com/rust-lang/cargo/pull/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: https://github.com/BurntSushi/ripgrep/pull/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]

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]

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]

Ah yes indeed! That all makes sense to me!

[retep998]

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]

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

[retep998]

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]

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]

@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]

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]

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]

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!

[behnam]

I have update the migration plan and will submit a PR to get us to Stage 1.1.

Thanks, @retep998, for bringing up the issue. :)

[kornelski]

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]

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.

[kornelski]

@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 (305bc25d5 2017-07-28) the rubbish directory is not included in the package.

[behnam]

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]

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]

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]

@nrc But you are using include!

https://github.com/rust-lang-nursery/rustfmt/blob/7298ce9d527625f8a9fdeb0bf2e5e92b9cd49274/Cargo.toml#L10

[nrc]

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

[kornelski]

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]

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

[alexcrichton]

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]

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]

Interesting find, @Diggsey! I submitted a PR to add unit-tests for it in the grepset repo (https://github.com/BurntSushi/ripgrep/pull/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]

Yeah that's what I've done for now, but it means I'll need a follow up fix
once the behaviour is corrected.

On 5 Feb 2018 2:41 am, "Behnam Esfahbod ❄" [e-mail] wrote:

Interesting find, @Diggsey https://github.com/diggsey! I submitted a PR
to add unit-tests for it in the grepset repo (BurntSushi/ripgrep#773
https://github.com/BurntSushi/ripgrep/pull/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?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/cargo/issues/4268#issuecomment-362966778,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAbi-cJW7u6P-glfqFqWtyfSWerdyfxdks5tRmpNgaJpZM4OTLfj
.

[matklad]

@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]

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]

Sounds good to me!

[matklad]

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

[behnam]

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.

[nox]

The following warning is emitted when I publish inert:

Pattern matching for Cargo's include/exclude fields is changing and file .vscode/settings.json WILL be excluded in a future Cargo version.

With the following package.exclude setting:

https://github.com/nox/inert/blob/ea06ac74f2cf3ee83672ed9ccabc09635dd5aa1a/Cargo.toml#L12

[behnam]

@nox, I believe that's because "/.vscode" doesn't match any files with the glob rules (matches the dir only), and changing it to "/.vscode/**" should silent the warning.

Also, AFAIU, that's an indicator of the current (glob) pattern not doing what you think it's doing, and the content of that dir, /.vscode, is present in your package, specifically /.vscode/settings.json. Could you plz double-check this, to make sure we're understanding the situation correctly.

[behnam]

About the status of this migration, I have tried getting back to it, but cannot get cargo package build properly on my MacPort-enabled system (conflicts of git and iconv libs). I'm not sure when I can spend time on it again.

If anyone can volunteer to take this step, it would be really appreciated. See 6b61d98fb030f4cbd2db15071f15a0468dfddc83 for the changes need to get to Stage 2.

[nox]

@nox, I believe that's because "/.vscode" doesn't match any files with the glob rules (matches the dir only), and changing it to "/.vscode/**" should silent the warning.

Oh I see. Shouldn't it already behave as if I specified "/.vscode/**" though? Why is the glob needed? Is that going to change when Cargo starts relying on .gitignore? The Git mechanism does allow to forbid an entire tree by just ignoring its top directory.

[mathstuf]

@nox Yes, that's what the warning is about. Cargo doesn't behave as if /** were appended now, but once the gitignore behavior lands, it will. Hence the notice about the behavior change.

[DoumanAsh]

Could please someone clarify the current state of cargo pattern matching?

I got warnings suddenly and looking at this issue, decided to change from using include to exclude for single directory for C sources
"/brotli/" - which I expect to exclude directory brotli in root of crate.

But currently it throws warnings on all files inside of it, so I'm a bit unsure whether we still use glob patterns in nightly or not?
The same if I remove leading slash, but I want to exclude only directory in root of crate.

[ehuss]

@DoumanAsh the latest nightly just switched to gitignore-style patterns (#6924) (docs).

/brotli/ differs in how it is interpreted by the old system (glob) and the new one (ignore). It's just showing a warning to let you know that it has changed. I'm planning to remove the warning in 1.38. If I'm reading your description correctly, you can ignore it if you only use nightly to publish, since /brotli/ matches everything. If you want to avoid the warning, I think /brotli/** works on both systems without warnings. When in doubt, run cargo package --list to see what will be included.

[DoumanAsh]

Ah I see, thank you. --list confirms that it works as I intended.

[XAMPPRocky]

Is there anything left to be implemented or can this issue be closed? Probably at very least there needs to be a PR to update the manifest documentation.

[ehuss]

@XAMPPRocky I plan to remove the warning in 1.38, so I'd like to leave it open until then.

The documentation was updated in the last PR. You can put "nightly" in the docs path to see the latest.