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

Run rustc for information fewer times #5249

Merged
merged 1 commit into from Mar 27, 2018

Conversation

alexcrichton
Copy link
Member

Currently if you pass --target the same as rustc's host Cargo will run rustc
on extra time to learn information, but we only need to run it once to learn
such information!

@rust-highfive
Copy link

r? @matklad

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

@@ -246,11 +246,17 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
/// all the units mentioned in `units`.
pub fn probe_target_info(&mut self) -> CargoResult<()> {
debug!("probe_target_info");
self.probe_target_info_kind(Kind::Target)?;
if self.requested_target().is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, interesting, so the previous logic kinda handled this in the case where target was not passed at all....

Perhaps add a helper Context::is_cross_compiling which handles both "no target" and "explicit target the same as the host", so that we don't duplicated this logic incorrectly elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm gonna hold off on this suggestion for now because the deduction here is different than Cargo's historical deduction of "am I cross compiling?" (aka is --target passed?). I'd hope though we could improve this in the future!

self.host_info = self.target_info.clone();
} else {
self.probe_target_info_kind(Kind::Host)?;
self.probe_target_info_kind(Kind::Target)?;
Copy link
Member

@matklad matklad Mar 27, 2018

Choose a reason for hiding this comment

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

Oh, actually this side-effectual mutating API looks horrible to me :) Do you feel like refactoring it to

self.host_info = self.probe_target_info_kind(Kind::Host);
self.target_info = if host_target_same { 
  self.host_info.clone() 
} else { 
  self.probe_target_info_kind(Kind::Target) 
}

?

@matklad
Copy link
Member

matklad commented Mar 27, 2018

Hm, what about normalizing --target flag earlier, so that "no target" and "target the same as host" are exactly equivalent? I think this might have some over benefits. For example, at the moment running

cargo build --target host_triple

and

cargo build

will build everything twice, because the command line and thus the fingerprints would differ.

Currently if you pass `--target` the same as rustc's host Cargo will run `rustc`
on extra time to learn information, but we only need to run it once to learn
such information!
@alexcrichton
Copy link
Member Author

Updated!

Leaving out the normalization for now as I think it runs the risk for breakage, but we can fix that later for sure I think.

@matklad
Copy link
Member

matklad commented Mar 27, 2018

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 27, 2018

📌 Commit 0eb7edf has been approved by matklad

bors added a commit that referenced this pull request Mar 27, 2018
Run `rustc` for information fewer times

Currently if you pass `--target` the same as rustc's host Cargo will run `rustc`
on extra time to learn information, but we only need to run it once to learn
such information!
@bors
Copy link
Collaborator

bors commented Mar 27, 2018

⌛ Testing commit 0eb7edf with merge ca7c19c...

@bors
Copy link
Collaborator

bors commented Mar 27, 2018

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

@bors bors merged commit 0eb7edf into rust-lang:master Mar 27, 2018
bors added a commit that referenced this pull request Apr 18, 2018
One hard cs problem

Closes #5313

r? @alexcrichton

Note that, due to the first commit, this still gives us all the benefits of #5249: if RUSTFLAGS is empty, we will run only a single rustc process, even if we can't cache it across different cargo invocations.
@alexcrichton alexcrichton deleted the run-rustc-less branch April 25, 2018 19:28
@ehuss ehuss added this to the 1.26.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.

None yet

5 participants