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

Fix dep info showing up with a build script #4711

Merged
merged 1 commit into from
Nov 12, 2017

Conversation

alexcrichton
Copy link
Member

Cargo would erroneously bail out early and accidentally forget to emit a
dep info file if any dependency used a build script, so this fixes that!

@rust-highfive
Copy link

r? @matklad

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

@alexcrichton
Copy link
Member Author

cc @nirbheek

@alexcrichton
Copy link
Member Author

This is also small and relatively seriously enough that I think I'll backport this when landed

if let Some(output) = context.build_state.outputs.lock().unwrap().get(&key) {
for path in &output.rerun_if_changed {
deps.insert(path.into());
// Add rerun-if-changed dependencies
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am probably reading this incorrectly, but shouldn't this be outside of if not custom_build if? Looks like rerun-if-changed is specifically about custom builds.

Cargo would erroneously bail out early and accidentally forget to emit a
dep info file if any dependency used a build script, so this fixes that!
@alexcrichton
Copy link
Member Author

@matklad updated!

@matklad
Copy link
Member

matklad commented Nov 12, 2017

@bors r+

+1 to backporting this.

Did the original version manage to get through the tests? If so, we could add a test case here :)

@bors
Copy link
Collaborator

bors commented Nov 12, 2017

📌 Commit 9e4f4d1 has been approved by matklad

@bors
Copy link
Collaborator

bors commented Nov 12, 2017

⌛ Testing commit 9e4f4d1 with merge a3ad53d249f733cd22e33c02ef9b18865156cdf4...

@alexcrichton
Copy link
Member Author

@matklad ah yeah I think this just never worked with build scripts (by accident!), but I updated a test in this PR which exercises this behavior

@matklad
Copy link
Member

matklad commented Nov 12, 2017

And what about the original version of this PR, where rerun-if-changed was ignored? Did it fail some tests?

@alexcrichton
Copy link
Member Author

Nah I don't think it matters where that code went, it just felt more future-proof to leave it on the outside. I don't think it needs to, for today, go on the outside for correctness.

@alexcrichton
Copy link
Member Author

@bors: retry

@bors
Copy link
Collaborator

bors commented Nov 12, 2017

⌛ Testing commit 9e4f4d1 with merge de48f75...

bors added a commit that referenced this pull request Nov 12, 2017
Fix dep info showing up with a build script

Cargo would erroneously bail out early and accidentally forget to emit a
dep info file if any dependency used a build script, so this fixes that!
@bors
Copy link
Collaborator

bors commented Nov 12, 2017

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

@bors bors merged commit 9e4f4d1 into rust-lang:master Nov 12, 2017
bors added a commit that referenced this pull request Nov 12, 2017
[beta] Fix dep info showing up with a build script

This is a backport of #4711
@alexcrichton alexcrichton deleted the fix-dep-info branch December 18, 2017 19:47
@ehuss ehuss added this to the 1.23.0 milestone Feb 6, 2022
@ehuss ehuss modified the milestones: 1.23.0, 1.22.0 Feb 14, 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.

5 participants