Skip to content
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

[package] include Cargo directive does not override .gitignore #4135

Closed
pnkfelix opened this issue Jun 7, 2017 · 5 comments · Fixed by #4180
Closed

[package] include Cargo directive does not override .gitignore #4135

pnkfelix opened this issue Jun 7, 2017 · 5 comments · Fixed by #4180

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Jun 7, 2017

I have a crate. It uses tango to keep a set of .md and .rs files in sync, but only the .md file is meant to be checked in, so the generated .rs are listed via a regexp in the .gitignore.

I was observing a problem: Edits to a generated .rs were failing to cause the crate's build script to be re-run, while edits to the .md were correctly causing the build script to be re-run. (The basic premise of tango is that touching either the .rs or .md file should cause the build script to be re-run.)

After some interactive debugging with @alexcrichton we determined that the issue appeared to be the fact that I had the generated .rs files listed in the .gitignore (as mentioned in the first sentence), as explained here: According to http://doc.crates.io/manifest.html#the-exclude-and-include-fields-optional :

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

So, okay: From what I can tell, that meant that updates to the .rs file was causing my build script to not run.

But also according to that same document:

setting include will override an exclude.

So apparently I should be able to list the files that my crate depends on explicitly in the include? Yet when I tried to do this by making a directive:

[package]
...
include = ["src/**/*.rs", "src/**/*.md", "tango-build.rs", "Cargo.toml"]

the crate nonetheless continued to fail to rebuild in response to modifications to the generated .rs files.


I have made a small branch to reproduce the problem without relying on subcrates like tango:

https://github.com/pnkfelix/mon-artist/tree/reduction-build-issue

The idea with this reduction of the bug is that this build script is a no-op, so you need to run cargo --verbose to observe the problem. The crucial issue is that after building the crate, doing the following should cause the build script to be re-run, but does not:

% touch src/lit/src/mod.rs
% cargo build --verbose
   Compiling reduction v0.1.1 (file:///Users/fklock/Dev/Rust/reduction)
     Running `rustc --crate-name mon_artist src/lib.rs --crate-type lib --emit=dep-info,link -C debuginfo=2 -C metadata=f443131a805a9905 -C extra-filename=-f443131a805a9905 --out-dir /Users/fklock/Dev/Rust/reduction/target/debug/deps -L dependency=/Users/fklock/Dev/Rust/reduction/target/debug/deps`
    Finished dev [unoptimized + debuginfo] target(s) in 0.14 secs
%
@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 7, 2017

(in case its not clear, when I remove the line src/lit/**/*.rs from the .gitignore, then things operate as I expect again. That's why I think the problem is something with a bad interaction between the implicit handling of .gitignore and [package] include.)

@alexcrichton
Copy link
Member

FWIW I believe this code is all located here and I think that we basically just apply gitignore filters before checking include/exclude, whereas we should do the opposite!

@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 8, 2017

@alexcrichton is there any reason why we could/should not change the code so that instead of include being expressed as a filter atop git-list-files, we could instead express the effect of include like so:

file-list = set-union( include-filter(list-files-walk), exclude-filter(list-files-git) )

In english: under this scheme we always apply the include filter to a walk of the whole tree (though of course we could skip this work if the include is empty), and combine it with the result of excluding a walk of the git-tree.

The main objections I could imagine to this are:

  • Its inefficient to walk the whole tree ourselves if we can avoid it
  • It changes the semantics of include so that it is additive (rather than replacing the output of the exclude)

(But personally I would find using an additive include to be more intuitive and ergonomic than the current semantics as documented...)

Update: (plus, one could recreate the current semantics under this scheme by just putting in an exclude filter of **/*; then the exclude-filter would yield the empty set, so the set-union would yield just the result of the include-filter)

@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 8, 2017

(my previous suggestion can probably be filed in a followup bug; it seems like it should be easy to fix this one while retaining the documented semantics for include, regardless of whether it is the "right thing" long term.)

@alexcrichton
Copy link
Member

Nah yeah that makes sense to me! It's sort of what I would have expected to happen before anyway. Traversing include should be fast in the sense that we're just doing work the user told us to do, we don't necessarily have to walk the whole tree itself.

pnkfelix added a commit to pnkfelix/cargo that referenced this issue Jun 16, 2017
pnkfelix added a commit to pnkfelix/cargo that referenced this issue Jun 21, 2017
bors added a commit that referenced this issue Jun 21, 2017
Issue 4135: include = [...] should override git file list

Fix #4135: if theres `include = [...]`, then do not prepopulate file list via git.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants