-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
Clean up dependency tracking in Rustbuild [1/2] #50904
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
Conversation
c73b239 to
5f40201
Compare
src/bootstrap/lib.rs
Outdated
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 comment should be modified for Codegen since we place its output in -codegen directory.
src/bootstrap/lib.rs
Outdated
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 instead combine all of the tools into one pattern or have them after each other so it's clearer what's going on
src/bootstrap/tool.rs
Outdated
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 believe we're calling CleanTools with Mode::Rustc instead of RustcTool in at least some places. Let's change the field to cause: Mode (or a different name if you can come up with something better) and switch these back to Mode::Std, Mode::Test and 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.
Hi, @Mark-Simulacrum Just to make sure I'm getting this right. Do you mean we should change the field mode: Mode on CleanTools to cause: Mode? Also, should the values just remain as they were? Thanks.
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.
Yes, change the field to cause, and change values to Mode::Std vs. Mode::ToolStd since the cause was Std, not a tool.
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.
Got it. Thanks again!
src/bootstrap/tool.rs
Outdated
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 self.mode instead
src/bootstrap/tool.rs
Outdated
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.
We'll want prepare_tool_cargo to take the mode as an argument; that'll make future changes easier. Generally speaking we should be threading tool modes through so that we never just pick "ToolRustc" for no reason.
src/bootstrap/tool.rs
Outdated
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.
Same as above, this should thread mode through from arguments
|
Hi @Mark-Simulacrum, this should now be ready for another look. Thanks! |
src/bootstrap/test.rs
Outdated
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 Mode::ToolRustc as we're building a tool
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.
Oops! let me fix this ASAP
src/bootstrap/test.rs
Outdated
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.
Same here, Mode::ToolRustc
src/bootstrap/builder.rs
Outdated
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 implement an is_tool method on Tool.
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.
By this, do you mean we should use a closure is_tool that checks if mode is in ToolRustc, ToolStd, etc?
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.
Not quite -- method on Tool, not a closure.
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.
It makes sense now. Thanks!
|
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 |
7f47fb6 to
2c9ded9
Compare
|
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 latest upstream changes (presumably #50892) made this pull request unmergeable. Please resolve the merge conflicts. |
|
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 |
|
Fixed! cc @Mark-Simulacrum |
src/bootstrap/builder.rs
Outdated
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.
Could we instead make this inherent, so mode.is_tool()? I think I may have previously mislead you; I intended this to be implemented on Mode.
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.
No worries! Going to fix this ASAP. Thanks
|
☔ The latest upstream changes (presumably #51138) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Hi @Mark-Simulacrum, this should be fixed now. Thanks! |
Mark-Simulacrum
left a comment
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 this is the last thing!
src/bootstrap/builder.rs
Outdated
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 !mode.is_tool().
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.
Oops! missed this.
|
Fixed! Thank you |
|
If you can clean up the commit history a little bit -- maybe merging in the |
make is_tool inherent prop of mode fix errors from rebase resolve issues from review
|
Done! thank you. |
|
@bors r+ |
|
📌 Commit 36eafe5 has been approved by |
Clean up dependency tracking in Rustbuild [1/2] Initial refactor of the `Mode` enum. Still a WIP Ref #50509 r? @Mark-Simulacrum
|
☀️ Test successful - status-appveyor, status-travis |
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
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
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
Initial refactor of the
Modeenum. Still a WIPRef #50509
r? @Mark-Simulacrum