-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 #50509
Comments
@Mark-Simulacrum Yeah, I would like to take this up. Thanks! |
Clean up dependency tracking in Rustbuild [1/2] Initial refactor of the `Mode` enum. Still a WIP Ref #50509 r? @Mark-Simulacrum
@collin5 Haven't heard from you in a while -- are you interested in implementing the second step here? No worries if not -- just wanted to check in. |
Oops! Sorry got caught up with some stuff lately. Will have something for you to look at as soon as possible. Thanks for the remainder 👍 . |
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
Triage; @Mark-Simulacrum Can this issue now be closed? If not what work is left to be done? |
Yes, I believe this issue is mostly done (and keeping it open is not helping, anyway). |
The primary idea here is that throughout compile, check, tool, and test.rs we call
builder.cargo
, with a series ofclear_if_dirty
calls before it depending on the implicit dependencies of the given crate.I'd like for us to instead utilize the
Mode
argument passed. That should ensure that we appropriately do this without having to think about it when adding new tools and tests.I believe the appropriate first step here is to refactor the existing
Mode
enum into something like the below; then we'd have to change the uses of it throughout the codebase to apply this strategy:Mode::{Std, Test, Rustc, Codegen}
respectively. Primarily the changes here are in tool.rs.Mode::Librustc
would becomeMode::RustcTool
, and same for the other combinations. There isn't aMode::CodegenTool
as those don't currently exist to my knowledge, but we can add it if necessary.Once this initial refactor is done, I think that's a good point to file a PR as it's an incremental change that can be reviewed on its own. The second change (which will close this issue) is to change
Builder::cargo
to have a match on the passed mode which will call clear_if_dirty with the stamp dependencies of the given Mode. Note that this means forMode::Rustc
we need to call clear_if_dirty on std, test stamps, whereasMode::RustcTool
needs to call clear_if_dirty for std, test, and rustc stamps. This is mostly just inlining code.cc @collin5 -- if you're interested please let me know
The text was updated successfully, but these errors were encountered: