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

Issue 4135: include = [...] should override git file list #4180

Merged
merged 3 commits into from
Jun 21, 2017

Conversation

pnkfelix
Copy link
Member

Fix #4135: if theres include = [...], then do not prepopulate file list via git.

@rust-highfive
Copy link

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

Looks good to me! I think though there's a test failure on AppVeyor? It can probably be fixed with usage of [/] instead of a literal / (as it's \ on Windows)

@pnkfelix
Copy link
Member Author

It can probably be fixed with usage of [/] instead of a literal / (as it's \ on Windows)

Ah I was wondering what the [/] was for, that makes sense. Will do.

@sfackler
Copy link
Member

sfackler commented Jun 20, 2017

Will this also cause .gitignore-based exclusions to be disabled?

EDIT: ah, looks like it will.

@pnkfelix
Copy link
Member Author

@sfackler yeah it will. Or rather, this will cause the code to actually conform to what is documented here, namely:

The options are mutually exclusive: setting include will override an exclude. Note that include must be an exhaustive list of files as otherwise necessary source files may not be included.

However, as I noted on the issue, I think we should plan in the future to adopt a different semantics, where include is purely additive. (I.e. exclude will operate as a blacklist filter atop the git index, and include be be a whitelist filter atop the file system.)

@pnkfelix
Copy link
Member Author

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Jun 21, 2017

@pnkfelix: 🔑 Insufficient privileges: Not in reviewers

@Mark-Simulacrum
Copy link
Member

@bors r=alexcrichton

You can make a PR to https://github.com/rust-lang/rust-central-station to get r+ rights.

@bors
Copy link
Contributor

bors commented Jun 21, 2017

📌 Commit 749ec6e has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jun 21, 2017

⌛ Testing commit 749ec6e with merge d23510c...

bors added a commit that referenced this pull request Jun 21, 2017
Issue 4135: include = [...] should override git file list

Fix #4135: if theres `include = [...]`, then do not prepopulate file list via git.
@bors
Copy link
Contributor

bors commented Jun 21, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing d23510c to master...

@bors bors merged commit 749ec6e into rust-lang:master Jun 21, 2017
bors added a commit that referenced this pull request Apr 15, 2019
…ichton

Fix test include_overrides_gitignore.

This test was added in #4180 and was disabled in #4218.  I don't think it is really feasible to get filetime into the test. The test was also using some questionable behavior of modifying contents of `src` from a `build.rs` script.  I rewrote the test to directly test the original change of having `package.include` override `.gitignore`.
@ehuss ehuss added this to the 1.20.0 milestone Feb 6, 2022
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 this pull request may close these issues.

[package] include Cargo directive does not override .gitignore
8 participants