Refactor context to extract dependencies calculation to a separate mod #5232
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| }) | ||
| } | ||
|
|
||
| pub fn lib_or_check_profile<'a, 'cfg>( |
There was a problem hiding this comment.
I think this may not need to be public, right?
| cx: &Context<'a, 'cfg>, | ||
| deps: &'b mut HashMap<Unit<'a>, Vec<Unit<'a>>>, | ||
| ) -> CargoResult<&'b [Unit<'a>]> { | ||
| if !deps.contains_key(unit) { |
There was a problem hiding this comment.
I might find this a bit clearer as:
match deps.entry(*unit) {
Entry::Occupied(v) => v.into_ref(), // or whatever the method is
Entry::Vacant(v) => v.insert(calculate_deps(...)),
}There was a problem hiding this comment.
I may be missing something here, though? Is it required to insert into the map before we do the recursive step? I would have figured that the recursion would have been ruled out by this point...
There was a problem hiding this comment.
.entry would not work here, because we need mutable access to deps in the call to calculate_deps :(
The recursion here is needed only for side effects: we want to fill dependencies not only for root units, but for the whole transitive closure. Ideally, this function should have returned (), but we actually call it recursively when we figure out deps for custom_build.
There was a problem hiding this comment.
Hm ok I'm still not 100% following but that's fine, can clean it up as need be in the future
| /// The `unit` provided must represent an execution of a build script, and | ||
| /// the returned set of units must all be run before `unit` is run. | ||
| pub fn dep_run_custom_build(&self, unit: &Unit<'a>) -> CargoResult<Vec<Unit<'a>>> { | ||
| // TODO: this ideally should be `-> &[Unit<'a>]` |
There was a problem hiding this comment.
Is this not possible due to various methods that take &mut self on Context?
There was a problem hiding this comment.
Yep! Specifically, this happens because target_filenames and target_metadatas are calculated lazily. I hope to refactor them out as well, and change a whole bunch of methods from &mut to &.
| }); | ||
| // Note there's a subtlety about this piece of code! The | ||
| // `build_script_overridden` map here is populated in | ||
| // `custom_build::build_map` which you need to call before inspecting |
There was a problem hiding this comment.
This is here because build_map requires dep_targets, right?
|
@bors: r+ |
|
📌 Commit 1b63fcf has been approved by |
Refactor context to extract dependencies calculation to a separate mod This makes unit dependency graph construction eager and moves it to a separate file. Hopefully, this makes `Context` slightly easier to understand :)
|
☀️ Test successful - status-appveyor, status-travis |
This makes unit dependency graph construction eager and moves it to a separate file.
Hopefully, this makes
Contextslightly easier to understand :)