include patterns should report invalid paths

[folkertdev]

Problem

In rust-lang/libz-sys#205 I fixed a case where a typo in the include list meant that some files were not included in the published package. The fix was simple:

include = [
    # ...
-    "src/zlib-ng/arch/s390x/**.[ch]",
+   "src/zlib-ng/arch/s390/**.[ch]",
    # ...
]

The fact that this invalid path did not get reported meant that the package was broken for the s390x target. Not great.

Proposed Solution

Intuitively I think every include line should match at least one file. In any other cases, it's redundant (can be removed) or a typo (very valuable to report to the user).

If that sounds reasonable I'd be happy have a go at implementing it.

Notes

No response

[epage]

Its hard for an exclude to always match because they might be for content that is conditionally there. Use cases like that for include are a little less obvious. For myself, I copy/paste a set of includes between projects. Not every project has everything that I reference.

We can't break error for this because of compatibility. We likely can't transition it to an error because there are use cases. This could potentially be a warning. A big implementation challenge is knowing when an include is used as we delegate this logic to third-party helpers.

[folkertdev]

A warning is fine I think. It's clear what happens if you ignore that warning: there is nothing there to include.

A big implementation challenge is knowing when an include is used as we delegate this logic to third-party helpers.

that's what I suspected. is git-oxide used for that already? https://github.com/Byron/gitoxide/tree/main/gix-ignore exists at least, and seems to have some logic for finding matching files.

[weihanglo]

that's what I suspected. is git-oxide used for that already?

It is still the ignore crate.

cargo/src/cargo/sources/path.rs

Lines 477 to 500 in 7ac34d3

	let mut include_builder = GitignoreBuilder::new(root);
	for rule in pkg.manifest().include() {
	include_builder.add_line(None, rule)?;
	}
	let ignore_include = include_builder.build()?;
	let ignore_should_package = |relative_path: &Path, is_dir: bool| {
	// "Include" and "exclude" options are mutually exclusive.
	if no_include_option {
	!ignore_exclude
	.matched_path_or_any_parents(relative_path, is_dir)
	.is_ignore()
	} else {
	if is_dir {
	// Generally, include directives don't list every
	// directory (nor should they!). Just skip all directory
	// checks, and only check files.
	return true;
	}
	ignore_include
	.matched_path_or_any_parents(relative_path, /* is_dir */ false)
	.is_ignore()
	}
	};

Another challenge is that we should avoid flooding people with those warnings. #12235 is one way to make it configurable.

[workingjubilee]

I am... sympathetic to folkertdev's concern, at least. If it weren't for the CI I recently added literally last Friday, that runs a packaging test using cargo +nightly package --workspace -Zpackage-workspace, I would have published another broken version of pgrx after doing another refactoring of the repo. Refactoring designed to separate out code that, last I checked, must currently be --no-verify published, mind, from code that can be verified to at least independently build.

You see, I moved the contents of the build.rs... which is now quite huge and basically its own build system... into another crate, and replaced the other crate's build.rs with this:

diff --git a/pgrx-pg-sys/bindgen.rs b/pgrx-pg-sys/bindgen.rs
new file mode 100644
index 00000000..5cecbe61
--- /dev/null
+++ b/pgrx-pg-sys/bindgen.rs
@@ -0,0 +1,4 @@
+// not a build.rs so that it doesn't inherit the git history of the build.rs
+
+// little-known Rust quirk: you can import main from wherever
+use pgrx_bindgen::build::main;

...and from the same diff, you may notice something that wasn't changed, but should have:

diff --git a/pgrx-pg-sys/Cargo.toml b/pgrx-pg-sys/Cargo.toml
index 85b33575..0dc2af1c 100644
--- a/pgrx-pg-sys/Cargo.toml
+++ b/pgrx-pg-sys/Cargo.toml
@@ -20,6 +20,7 @@ documentation = "https://docs.rs/pgrx-pg-sys"
 readme = "README.md"
 edition = "2021"
 include = ["src/**/*", "README.md", "include/*", "cshim/*", "build.rs", "build/*"]
+build = "bindgen.rs"

...yeah.

[workingjubilee]

Is there any chance the existing little warnings like these can at least be made more prominent or repeated after the list of packages whizzes by? They're hard to pick out amidst the rest of everything.

$ cargo +nightly package --workspace -Zpackage-workspace --allow-dirty
   Packaging pgrx-pg-config v0.0.999-rc.999 (/home/jubilee/tcdi/pgrx/pgrx-pg-config)
    Packaged 7 files, 35.5KiB (9.5KiB compressed)
   Packaging pgrx-sql-entity-graph v0.0.999-rc.999 (/home/jubilee/tcdi/pgrx/pgrx-sql-entity-graph)
    Packaged 55 files, 393.4KiB (65.9KiB compressed)
   Packaging cargo-pgrx v0.0.999-rc.999 (/home/jubilee/tcdi/pgrx/cargo-pgrx)
    Updating crates.io index
    Packaged 39 files, 434.9KiB (102.5KiB compressed)
   Packaging pgrx-macros v0.0.999-rc.999 (/home/jubilee/tcdi/pgrx/pgrx-macros)
    Packaged 7 files, 63.5KiB (14.5KiB compressed)
   Packaging pgrx-bindgen v0.0.999-rc.999 (/home/jubilee/tcdi/pgrx/pgrx-bindgen)
    Packaged 7 files, 125.4KiB (31.9KiB compressed)
   Packaging pgrx-pg-sys v0.0.999-rc.999 (/home/jubilee/tcdi/pgrx/pgrx-pg-sys)
warning: ignoring `package.build` as `bindgen.rs` is not included in the published package
    Packaged 45 files, 12.5MiB (1.7MiB compressed)
   Packaging pgrx v0.0.999-rc.999 (/home/jubilee/tcdi/pgrx/pgrx)
    Packaged 103 files, 881.7KiB (187.0KiB compressed)
   Packaging pgrx-tests v0.0.999-rc.999 (/home/jubilee/tcdi/pgrx/pgrx-tests)
warning: ignoring test `ui` as `tests/ui.rs` is not included in the published package
    Updating crates.io index
    Packaged 10 files, 97.8KiB (28.1KiB compressed)
   Verifying pgrx-pg-config v0.0.999-rc.999 (/home/jubilee/tcdi/pgrx/pgrx-pg-config)
    Updating crates.io index
   Compiling proc-macro2 v1.0.86
# another ~100 lines of compiling messages

This is partly because we've only just started on compiling the "root" packages all others depend on, and will be crawling up them to the main "facade" packages. These roots, having no dependencies on other packages (in the workspace, anyways), may be in a position where they essentially always build without problem.

[epage]

Switching things around to put those messages last is a very invasive change though there are other paths for improving this.

Overall, I'd like to improve compilation output so that it doesn't take up so much permanent space (#8889). I'm particularly interested in this for improving the cargo-script experience.

We are also working on adding control over cargo warnings by turning them into lints (#12235) and to turn warning lints into errors (#8424).

[workingjubilee]

Ah! Yeah, I figured that'd likely be one of those "surprisingly difficult things", but it seemed like something to ask rather than assume.

Yeah, it requires opt-in so has the discoverability issue but if there's a set of lints we can opt in to denying, someone could write up a "how to set up prepublish CI with cargo" article and they would probably become common knowledge fairly fast.

[ehuss]

The cargo team discussed this today and felt like it would be good for this to be a lint warning. However, as noted above, it's unclear how feasible that is with the ignore crate since it doesn't tell us which patterns had no effect on the resulting list of files. Some of these complexities are noted at #9667 (comment).

Since I believe this is essentially a duplicate of #9667, closing in favor of that.