-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Clean up dependency tracking in Rustbuild [2/2] #52036
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, left some nits but should be mostly minor.
src/bootstrap/builder.rs
Outdated
}; | ||
|
||
if cmd == "doc" { | ||
if mode == Mode::Rustc || mode == Mode::ToolRustc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or Codegen
src/bootstrap/builder.rs
Outdated
// This is for the original compiler, but if we're forced to use stage 1, then | ||
// std/test/rustc stamps won't exist in stage 2, so we need to get those from stage 1, since | ||
// we copy the libs forward. | ||
let compiler = if self.force_use_stage1(compiler, target) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be careful here and only use this compiler for the stamp files -- otherwise I think we'll run into trouble lower down in this function with the compiler path being different than expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, going to fix this. Thanks!
src/bootstrap/builder.rs
Outdated
@@ -705,6 +705,68 @@ impl<'a> Builder<'a> { | |||
) -> Command { | |||
let mut cargo = Command::new(&self.initial_cargo); | |||
let out_dir = self.stage_out(compiler, mode); | |||
|
|||
let mut my_out = match cmd { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment here about the difference between this and out_dir
defined just above? (Ideally we'd unify the two).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had thought of unifying it but looks like this (out_dir
) is used lower down https://github.com/rust-lang/rust/blob/master/src/bootstrap/builder.rs#L709 and is always the same no matter the passed cmd
yet my_out
should be different for each passed cmd
. Maybe I'm missing something here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right -- yes, that's correct. I think we'll eventually want to resolve that to be the same (since presumably that's ultimately what we want) but for now it should be enough to just add the comment -- basically saying that this is a command-specific path, unlike out_dir which is broader in scope.
src/bootstrap/builder.rs
Outdated
self.clear_if_dirty(&my_out, &self.rustc(compiler)); | ||
}, | ||
Mode::Rustc => { | ||
self.clear_if_dirty(&my_out, &libstd_stamp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These will all want to be gated on self.rustc(compiler)
, I believe, since we can't guarantee that std's stamp was touched (IIRC, I had some scenario where that was the case, though I could be wrong). If we're repeating anything though we should repeat everything (e.g., rustc would be compiler/std/test).
src/bootstrap/builder.rs
Outdated
Mode::Std => { | ||
self.clear_if_dirty(&my_out, &self.rustc(compiler)); | ||
}, | ||
Mode::Rustc => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can change the order here to be std, test, rustc that'd be good
src/bootstrap/builder.rs
Outdated
self.clear_if_dirty(&my_out, &libtest_stamp); | ||
self.clear_if_dirty(&my_out, &librustc_stamp); | ||
} | ||
_ => { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a fully exhaustive list, even if some variants are nothing -- which is probably false; we for example for ToolBoostrap
need to clear if the bootstrap compiler changed, as we still go through build/bootstrap/debug/rustc
in that case.
src/bootstrap/tool.rs
Outdated
fn run(self, builder: &Builder) { | ||
let compiler = self.compiler; | ||
let target = self.target; | ||
fn run(self, _builder: &Builder) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we can completely remove this struct now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't sure about this at first, but yeah looks like we won't need it anymore 👍
@alexcrichton I've slowly been moving towards this approach -- wanted to get feedback on the general idea of centralizing the |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This looks pretty good to me, thanks! I'd recommend being sure to try out a few combinations of commands locally, this'll be easy to accidentally introduce a regression as we don't have many workflow-like tests |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
You can now take another look at this. Thank you! |
src/bootstrap/builder.rs
Outdated
self.clear_if_dirty(&my_out, &libstd_stamp); | ||
self.clear_if_dirty(&my_out, &libtest_stamp); | ||
}, | ||
Mode::Codegen => { }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codegen also needs to be cleared if rustc/std/test/rustc are dirty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to fix this. Thanks 👍
target, | ||
cause: Mode::Std, | ||
}); | ||
builder.cargo(target_compiler, Mode::ToolStd, target, "clean"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, is this (and similar statements below) actually necessary? I would've hoped this would happen on its own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. It looks like some steps are missed when we don't explicitly call Builder::cargo
here. i.e running ./x.py test src/bootstrap
without doing this shows that some steps were actually not called. I'm not completely sure about this though, maybe I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Step resolution shouldn't depend on files existing on the system - do you happen to have a concrete example? If we actually do need this then can we factor out the cleaning logic from Builder::cargo
and call that separately here? It feels a little odd to be creating the full command and then not running it (i.e., using implicit side-effects).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't feel like dealing with this though feel free to let me know -- I'm fine with leaving these in for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i.e
removing both Builder::cargo
https://github.com/rust-lang/rust/pull/52036/files#diff-fa9668c926a2788f7849514fe3ed66a9R248 for StdLink
and https://github.com/rust-lang/rust/pull/52036/files#diff-fa9668c926a2788f7849514fe3ed66a9R446 for TestLink
then doing ./x.py test src/bootstrap
, Assertion
Line 1660 in 1ecf692
assert_eq!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's leave this as is for now and look into the details later -- it seems fine to leave this somewhat half done pending a more thorough investigation as to how exactly the dependency edge is created.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@collin5 If you can try out a few workflows/commands and make sure they continue to work (i.e., no weird linker/compiler errors related to duplicate deps or unexpected rebuilding), such as:
@bors delegate+ If you feel about the above "workflows" being correct then feel free to r=me, you should have permission now. |
✌️ @collin5 can now approve this pull request |
@bors r=Mark-Simulacrum |
📌 Commit 0d26932a19e60014da2ee02d17f00167b4952fc4 has been approved by |
⌛ Testing commit 0d26932a19e60014da2ee02d17f00167b4952fc4 with merge d1abe7496cbf9edf09a646b7d17f54c72958c553... |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Hmm, Looks like I've failed to reproduce this. Running
Do you please mind taking a look at this? Maybe there is something I'm missing here. Thank you! cc @Mark-Simulacrum |
Hm, have you tried clearing out the build directory ( |
Yeah, I did that. Even removed all the docker images and cache but still getting the same result. |
also indicate difference between out_dir and my_out
5653421
to
9681e13
Compare
I rebased and force pushed, I think I can reproduce this now. |
Awesome! thanks a lot. Going to pull the changes in a few. |
😌 Whew! . Finally this should do it. |
📌 Commit 5ae40be has been approved by |
Clean up dependency tracking in Rustbuild [2/2] Make `clear_if_dirty` calls in `Builder::cargo` with stamp dependencies for the given Mode. Continuation of #50904 Ref issue #50509 r? @Mark-Simulacrum
☀️ Test successful - status-appveyor, status-travis |
Make
clear_if_dirty
calls inBuilder::cargo
with stamp dependencies for the given Mode.Continuation of #50904
Ref issue #50509
r? @Mark-Simulacrum