From d4b6e90fc1f94856ca4a49f2b55d8a3b6353aea6 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 9 May 2019 16:18:14 -0700 Subject: [PATCH 1/3] Migrate package include/exclude to gitignore patterns. --- src/cargo/sources/path.rs | 35 ++++--- src/doc/src/reference/manifest.md | 71 ++++++++----- tests/testsuite/package.rs | 164 ++++++++++++++++++++++++++---- 3 files changed, 209 insertions(+), 61 deletions(-) diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index b2cd4e82483..de56d301b49 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -101,10 +101,10 @@ impl<'cfg> PathSource<'cfg> { /// stages are: /// /// 1) Only warn users about the future change iff their matching rules are - /// affected. (CURRENT STAGE) + /// affected. /// /// 2) Switch to the new strategy and update documents. Still keep warning - /// affected users. + /// affected users. (CURRENT STAGE) /// /// 3) Drop the old strategy and no more warnings. /// @@ -122,7 +122,6 @@ impl<'cfg> PathSource<'cfg> { p }; Pattern::new(pattern) - .map_err(|e| failure::format_err!("could not parse glob pattern `{}`: {}", p, e)) }; let glob_exclude = pkg @@ -130,14 +129,19 @@ impl<'cfg> PathSource<'cfg> { .exclude() .iter() .map(|p| glob_parse(p)) - .collect::, _>>()?; + .collect::, _>>(); let glob_include = pkg .manifest() .include() .iter() .map(|p| glob_parse(p)) - .collect::, _>>()?; + .collect::, _>>(); + + // Don't warn about glob mismatch if it doesn't parse. + let glob_is_valid = glob_exclude.is_ok() && glob_include.is_ok(); + let glob_exclude = glob_exclude.unwrap_or_else(|_| Vec::new()); + let glob_include = glob_include.unwrap_or_else(|_| Vec::new()); let glob_should_package = |relative_path: &Path| -> bool { fn glob_match(patterns: &[Pattern], relative_path: &Path) -> bool { @@ -210,20 +214,20 @@ impl<'cfg> PathSource<'cfg> { let glob_should_package = glob_should_package(relative_path); let ignore_should_package = ignore_should_package(relative_path)?; - if glob_should_package != ignore_should_package { + if glob_is_valid && glob_should_package != ignore_should_package { if glob_should_package { if no_include_option { self.config.shell().warn(format!( - "Pattern matching for Cargo's include/exclude fields is changing and \ - file `{}` WILL be excluded in a future Cargo version.\n\ + "Pattern matching for Cargo's include/exclude fields has changed and \ + file `{}` is now excluded.\n\ See for more \ information.", relative_path.display() ))?; } else { self.config.shell().warn(format!( - "Pattern matching for Cargo's include/exclude fields is changing and \ - file `{}` WILL NOT be included in a future Cargo version.\n\ + "Pattern matching for Cargo's include/exclude fields has changed and \ + file `{}` is no longer included.\n\ See for more \ information.", relative_path.display() @@ -231,16 +235,16 @@ impl<'cfg> PathSource<'cfg> { } } else if no_include_option { self.config.shell().warn(format!( - "Pattern matching for Cargo's include/exclude fields is changing and \ - file `{}` WILL NOT be excluded in a future Cargo version.\n\ + "Pattern matching for Cargo's include/exclude fields has changed and \ + file `{}` is NOT excluded.\n\ See for more \ information.", relative_path.display() ))?; } else { self.config.shell().warn(format!( - "Pattern matching for Cargo's include/exclude fields is changing and \ - file `{}` WILL be included in a future Cargo version.\n\ + "Pattern matching for Cargo's include/exclude fields has changed and \ + file `{}` is now included.\n\ See for more \ information.", relative_path.display() @@ -248,8 +252,7 @@ impl<'cfg> PathSource<'cfg> { } } - // Update to `ignore_should_package` for Stage 2. - Ok(glob_should_package) + Ok(ignore_should_package) }; // Attempt Git-prepopulate only if no `include` (see rust-lang/cargo#4135). diff --git a/src/doc/src/reference/manifest.md b/src/doc/src/reference/manifest.md index 521b1264c5f..3e762bde314 100644 --- a/src/doc/src/reference/manifest.md +++ b/src/doc/src/reference/manifest.md @@ -73,11 +73,11 @@ examples, etc. #### The `build` field (optional) -This field specifies a file in the package root which is a [build script][1] for -building native code. More information can be found in the build script -[guide][1]. +This field specifies a file in the package root which is a [build script] for +building native code. More information can be found in the [build script +guide][build script]. -[1]: reference/build-scripts.html +[build script]: reference/build-scripts.html ```toml [package] @@ -121,15 +121,37 @@ may be replaced by docs.rs links. #### The `exclude` and `include` fields (optional) -You can explicitly specify to Cargo that a set of [globs][globs] should be -ignored or included for the purposes of packaging and rebuilding a package. The -globs specified in the `exclude` field identify a set of files that are not -included when a package is published as well as ignored for the purposes of -detecting when to rebuild a package, and the globs in `include` specify files -that are explicitly included. - -If a VCS is being used for a package, the `exclude` field will be seeded with -the VCS’ ignore settings (`.gitignore` for git for example). +You can explicitly specify that a set of file patterns should be ignored or +included for the purposes of packaging. The patterns specified in the +`exclude` field identify a set of files that are not included, and the +patterns in `include` specify files that are explicitly included. + +The patterns should be [gitignore]-style patterns. Briefly: + +- `foo` matches any file or directory with the name `foo` anywhere in the + package. This is equivalent to the pattern `**/foo`. +- `/foo` matches any file or directory with the name `foo` only in the root of + the package. +- `foo/` matches any *directory* with the name `foo` anywhere in the package. +- Common glob patterns like `*`, `?`, and `[]` are supported: + - `*` matches zero or more characters except `/`. For example, `*.html` + matches any file or directory with the `.html` extension anywhere in the + package. + - `?` matches any character except `/`. For example, `foo?` matches `food`, + but not `foo`. + - `[]` allows for matching a range of characters. For example, `[ab]` + matches either `a` or `b`. `[a-z]` matches letters a through z. +- `**/` prefix matches in any directory. For example, `**/foo/bar` matches the + file or directory `bar` anywhere that is directly under directory `foo`. +- `/**` suffix matches everything inside. For example, `foo/**` matches all + files inside directory `foo`, including all files in subdirectories below + `foo`. +- `/**/` matches zero or more directories. For example, `a/**/b` matches + `a/b`, `a/x/b`, `a/x/y/b`, and so on. +- `!` prefixed patterns are not supported. + +If git is being used for a package, the `exclude` field will be seeded with +the `gitignore` settings from the repository. ```toml [package] @@ -148,21 +170,14 @@ The options are mutually exclusive: setting `include` will override an necessary source files may not be included. The package's `Cargo.toml` is automatically included. -[globs]: https://docs.rs/glob/0.2.11/glob/struct.Pattern.html - -#### Migrating to `gitignore`-like pattern matching +The include/exclude list is also used for change tracking in some situations. +For targets built with `rustdoc`, it is used to determine the list of files to +track to determine if the target should be rebuilt. If the package has a +[build script] that does not emit any `rerun-if-*` directives, then the +include/exclude list is used for tracking if the build script should be re-run +if any of those files change. -The current interpretation of these configs is based on UNIX Globs, as -implemented in the [`glob` crate](https://crates.io/crates/glob). We want -Cargo's `include` and `exclude` configs to work as similar to `gitignore` as -possible. [The `gitignore` specification](https://git-scm.com/docs/gitignore) 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](https://crates.io/crates/ignore), and treat them each rule as a single -line in a `gitignore` file. See [the tracking -issue](https://github.com/rust-lang/cargo/issues/4268) for more details on the -migration. +[gitignore]: https://git-scm.com/docs/gitignore #### The `publish` field (optional) @@ -615,6 +630,8 @@ and also be a member crate of another workspace (contain `package.workspace`). Most of the time workspaces will not need to be dealt with as `cargo new` and `cargo init` will handle workspace configuration automatically. +[globs]: https://docs.rs/glob/0.2.11/glob/struct.Pattern.html + #### Virtual Manifest In workspace manifests, if the `package` table is present, the workspace root diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index cef755450a2..09efadd6da4 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -4,6 +4,7 @@ use std::io::prelude::*; use std::path::Path; use crate::support::cargo_process; +use crate::support::paths::CargoPathExt; use crate::support::registry::Package; use crate::support::{ basic_manifest, git, path2url, paths, project, publish::validate_crate_contents, registry, @@ -363,17 +364,17 @@ fn exclude() { "\ [WARNING] manifest has no description[..] See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info. -[WARNING] [..] file `dir_root_1/some_dir/file` WILL be excluded [..] +[WARNING] [..] file `dir_root_1/some_dir/file` is now excluded. See [..] -[WARNING] [..] file `dir_root_2/some_dir/file` WILL be excluded [..] +[WARNING] [..] file `dir_root_2/some_dir/file` is now excluded. See [..] -[WARNING] [..] file `dir_root_3/some_dir/file` WILL be excluded [..] +[WARNING] [..] file `dir_root_3/some_dir/file` is now excluded. See [..] -[WARNING] [..] file `some_dir/dir_deep_1/some_dir/file` WILL be excluded [..] +[WARNING] [..] file `some_dir/dir_deep_1/some_dir/file` is now excluded. See [..] -[WARNING] [..] file `some_dir/dir_deep_3/some_dir/file` WILL be excluded [..] +[WARNING] [..] file `some_dir/dir_deep_3/some_dir/file` is now excluded. See [..] -[WARNING] [..] file `some_dir/file_deep_1` WILL be excluded [..] +[WARNING] [..] file `some_dir/file_deep_1` is now excluded. See [..] [PACKAGING] foo v0.0.1 ([..]) [ARCHIVING] [..] @@ -388,12 +389,6 @@ See [..] [ARCHIVING] [..] [ARCHIVING] [..] [ARCHIVING] [..] -[ARCHIVING] [..] -[ARCHIVING] [..] -[ARCHIVING] [..] -[ARCHIVING] [..] -[ARCHIVING] [..] -[ARCHIVING] [..] [ARCHIVING] .cargo_vcs_info.json ", ) @@ -407,18 +402,12 @@ See [..] "\ .cargo_vcs_info.json Cargo.toml -dir_root_1/some_dir/file -dir_root_2/some_dir/file -dir_root_3/some_dir/file file_root_3 file_root_4 file_root_5 -some_dir/dir_deep_1/some_dir/file some_dir/dir_deep_2/some_dir/file -some_dir/dir_deep_3/some_dir/file some_dir/dir_deep_4/some_dir/file some_dir/dir_deep_5/some_dir/file -some_dir/file_deep_1 some_dir/file_deep_2 some_dir/file_deep_3 some_dir/file_deep_4 @@ -1162,3 +1151,142 @@ fn include_cargo_toml_implicit() { .with_stdout("Cargo.toml\nsrc/lib.rs\n") .run(); } + +fn include_exclude_test(include: &str, exclude: &str, files: &[&str], expected: &str) { + let mut pb = project().file( + "Cargo.toml", + &format!( + r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + license = "MIT" + description = "foo" + documentation = "foo" + homepage = "foo" + repository = "foo" + include = {} + exclude = {} + "#, + include, exclude + ), + ); + for file in files { + pb = pb.file(file, ""); + } + let p = pb.build(); + + p.cargo("package --list") + .with_stdout(expected) + // .with_stderr("") Add this back when warnings are removed. + .run(); + p.root().rm_rf(); +} + +#[test] +fn package_include_ignore_only() { + // Test with a gitignore pattern that fails to parse with glob. + // This is a somewhat nonsense pattern, but is an example of something git + // allows and glob does not. + assert!(glob::Pattern::new("src/abc**").is_err()); + + include_exclude_test( + r#"["Cargo.toml", "src/abc**", "src/lib.rs"]"#, + "[]", + &["src/lib.rs", "src/abc1.rs", "src/abc2.rs", "src/abc/mod.rs"], + "Cargo.toml\n\ + src/abc/mod.rs\n\ + src/abc1.rs\n\ + src/abc2.rs\n\ + src/lib.rs\n\ + ", + ) +} + +#[test] +fn gitignore_patterns() { + include_exclude_test( + r#"["Cargo.toml", "foo"]"#, // include + "[]", + &["src/lib.rs", "foo", "a/foo", "a/b/foo", "x/foo/y", "bar"], + "Cargo.toml\n\ + a/b/foo\n\ + a/foo\n\ + foo\n\ + x/foo/y\n\ + ", + ); + + include_exclude_test( + r#"["Cargo.toml", "/foo"]"#, // include + "[]", + &["src/lib.rs", "foo", "a/foo", "a/b/foo", "x/foo/y", "bar"], + "Cargo.toml\n\ + foo\n\ + ", + ); + + include_exclude_test( + "[]", + r#"["foo/"]"#, // exclude + &["src/lib.rs", "foo", "a/foo", "x/foo/y", "bar"], + "Cargo.toml\n\ + a/foo\n\ + bar\n\ + foo\n\ + src/lib.rs\n\ + ", + ); + + include_exclude_test( + "[]", + r#"["*.txt", "[ab]", "[x-z]"]"#, // exclude + &[ + "src/lib.rs", + "foo.txt", + "bar/foo.txt", + "other", + "a", + "b", + "c", + "x", + "y", + "z", + ], + "Cargo.toml\n\ + c\n\ + other\n\ + src/lib.rs\n\ + ", + ); + + include_exclude_test( + r#"["Cargo.toml", "**/foo/bar"]"#, // include + "[]", + &["src/lib.rs", "a/foo/bar", "foo", "bar"], + "Cargo.toml\n\ + a/foo/bar\n\ + ", + ); + + include_exclude_test( + r#"["Cargo.toml", "foo/**"]"#, // include + "[]", + &["src/lib.rs", "a/foo/bar", "foo/x/y/z"], + "Cargo.toml\n\ + foo/x/y/z\n\ + ", + ); + + include_exclude_test( + r#"["Cargo.toml", "a/**/b"]"#, // include + "[]", + &["src/lib.rs", "a/b", "a/x/b", "a/x/y/b"], + "Cargo.toml\n\ + a/b\n\ + a/x/b\n\ + a/x/y/b\n\ + ", + ); +} From 3ca96e90eb9b79c3bcf278aceca23256a2268869 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 9 May 2019 17:55:25 -0700 Subject: [PATCH 2/3] Add support for ! negate gitignore patterns. --- src/cargo/sources/path.rs | 20 +++++---- src/doc/src/reference/manifest.md | 4 +- tests/testsuite/package.rs | 75 ++++++++++++++++++++++++++++--- 3 files changed, 84 insertions(+), 15 deletions(-) diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index de56d301b49..548363e0da5 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -138,8 +138,16 @@ impl<'cfg> PathSource<'cfg> { .map(|p| glob_parse(p)) .collect::, _>>(); + // Don't warn if using a negate pattern, since those weren't ever + // previously supported. + let has_negate = pkg + .manifest() + .exclude() + .iter() + .chain(pkg.manifest().include().iter()) + .any(|p| p.starts_with("!")); // Don't warn about glob mismatch if it doesn't parse. - let glob_is_valid = glob_exclude.is_ok() && glob_include.is_ok(); + let glob_is_valid = glob_exclude.is_ok() && glob_include.is_ok() && !has_negate; let glob_exclude = glob_exclude.unwrap_or_else(|_| Vec::new()); let glob_include = glob_include.unwrap_or_else(|_| Vec::new()); @@ -180,10 +188,7 @@ impl<'cfg> PathSource<'cfg> { { Match::None => Ok(true), Match::Ignore(_) => Ok(false), - Match::Whitelist(pattern) => Err(failure::format_err!( - "exclude rules cannot start with `!`: {}", - pattern.original() - )), + Match::Whitelist(_) => Ok(true), } } else { match ignore_include @@ -191,10 +196,7 @@ impl<'cfg> PathSource<'cfg> { { Match::None => Ok(false), Match::Ignore(_) => Ok(true), - Match::Whitelist(pattern) => Err(failure::format_err!( - "include rules cannot start with `!`: {}", - pattern.original() - )), + Match::Whitelist(_) => Ok(false), } } }; diff --git a/src/doc/src/reference/manifest.md b/src/doc/src/reference/manifest.md index 3e762bde314..09c0c65e70f 100644 --- a/src/doc/src/reference/manifest.md +++ b/src/doc/src/reference/manifest.md @@ -148,7 +148,9 @@ The patterns should be [gitignore]-style patterns. Briefly: `foo`. - `/**/` matches zero or more directories. For example, `a/**/b` matches `a/b`, `a/x/b`, `a/x/y/b`, and so on. -- `!` prefixed patterns are not supported. +- `!` prefix negates a pattern. For example, a pattern of `src/**.rs` and + `!foo.rs` would match all files with the `.rs` extension inside the `src` + directory, except for any file named `foo.rs`. If git is being used for a package, the `exclude` field will be seeded with the `gitignore` settings from the repository. diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 09efadd6da4..7a491a6bbf4 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -1152,7 +1152,13 @@ fn include_cargo_toml_implicit() { .run(); } -fn include_exclude_test(include: &str, exclude: &str, files: &[&str], expected: &str) { +fn include_exclude_test( + include: &str, + exclude: &str, + files: &[&str], + expected: &str, + has_warnings: bool, +) { let mut pb = project().file( "Cargo.toml", &format!( @@ -1177,10 +1183,13 @@ fn include_exclude_test(include: &str, exclude: &str, files: &[&str], expected: } let p = pb.build(); - p.cargo("package --list") - .with_stdout(expected) - // .with_stderr("") Add this back when warnings are removed. - .run(); + let mut e = p.cargo("package --list"); + if has_warnings { + e.with_stderr_contains("[..]"); + } else { + e.with_stderr(""); + } + e.with_stdout(expected).run(); p.root().rm_rf(); } @@ -1201,6 +1210,7 @@ fn package_include_ignore_only() { src/abc2.rs\n\ src/lib.rs\n\ ", + false, ) } @@ -1216,6 +1226,7 @@ fn gitignore_patterns() { foo\n\ x/foo/y\n\ ", + true, ); include_exclude_test( @@ -1225,6 +1236,7 @@ fn gitignore_patterns() { "Cargo.toml\n\ foo\n\ ", + false, ); include_exclude_test( @@ -1237,6 +1249,7 @@ fn gitignore_patterns() { foo\n\ src/lib.rs\n\ ", + true, ); include_exclude_test( @@ -1259,6 +1272,7 @@ fn gitignore_patterns() { other\n\ src/lib.rs\n\ ", + false, ); include_exclude_test( @@ -1268,6 +1282,7 @@ fn gitignore_patterns() { "Cargo.toml\n\ a/foo/bar\n\ ", + false, ); include_exclude_test( @@ -1277,6 +1292,7 @@ fn gitignore_patterns() { "Cargo.toml\n\ foo/x/y/z\n\ ", + false, ); include_exclude_test( @@ -1288,5 +1304,54 @@ fn gitignore_patterns() { a/x/b\n\ a/x/y/b\n\ ", + false, + ); +} + +#[test] +fn gitignore_negate() { + include_exclude_test( + r#"["Cargo.toml", "*.rs", "!foo.rs", "\\!important"]"#, // include + "[]", + &["src/lib.rs", "foo.rs", "!important"], + "!important\n\ + Cargo.toml\n\ + src/lib.rs\n\ + ", + false, + ); + + // NOTE: This is unusual compared to git. Git treats `src/` as a + // short-circuit which means rules like `!src/foo.rs` would never run. + // However, because Cargo only works by iterating over *files*, it doesn't + // short-circuit. + include_exclude_test( + r#"["Cargo.toml", "src/", "!src/foo.rs"]"#, // include + "[]", + &["src/lib.rs", "src/foo.rs"], + "Cargo.toml\n\ + src/lib.rs\n\ + ", + false, + ); + + include_exclude_test( + r#"["Cargo.toml", "src/**.rs", "!foo.rs"]"#, // include + "[]", + &["src/lib.rs", "foo.rs", "src/foo.rs", "src/bar/foo.rs"], + "Cargo.toml\n\ + src/lib.rs\n\ + ", + false, + ); + + include_exclude_test( + "[]", + r#"["*.rs", "!foo.rs", "\\!important"]"#, // exclude + &["src/lib.rs", "foo.rs", "!important"], + "Cargo.toml\n\ + foo.rs\n\ + ", + false, ); } From db3328ecb1b26c95256a5b2d27a67dbe36ff0932 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 9 May 2019 19:05:31 -0700 Subject: [PATCH 3/3] Add a warning if both package.include and package.exclude are specified. --- src/cargo/ops/cargo_package.rs | 6 ++++++ tests/testsuite/package.rs | 1 + 2 files changed, 7 insertions(+) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 76f2603e750..fc39b51796a 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -56,6 +56,12 @@ pub fn package(ws: &Workspace<'_>, opts: &PackageOpts<'_>) -> CargoResult