feat: Fixing parallel targets in the same pass #82
Conversation
Pyr0de
force-pushed
the
issue-52
branch
2 times, most recently
from
52c0909 to
dfb14d8
Compare
| impl PartialEq for Kind { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| match (self, other) { | ||
| (Kind::Other(a), Kind::Other(b)) => a == b, | ||
| _ => std::mem::discriminant(self) == std::mem::discriminant(other), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Hash for Kind { | ||
| fn hash<H: std::hash::Hasher>(&self, state: &mut H) { | ||
| match self { | ||
| Self::CustomBuild => state.write_u8(1), | ||
| Self::Lib(l) => { | ||
| state.write_u8(2); | ||
| l.hash(state); | ||
| } | ||
| Self::Bin(b) => { | ||
| state.write_u8(3); | ||
| b.hash(state); | ||
| } | ||
| Self::Other(a) => { | ||
| state.write_u8(4); | ||
| a.hash(state); | ||
| } | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Would it be alright to do this?
There was a problem hiding this comment.
Custom PartialEq, Eq, and Hash are tricky to get correct and I'd recommend re-evaluating a design to use it.
|
Is there a way I could test these changes? |
| pub enum Kind { | ||
| Bin, | ||
| Example, | ||
| Test, | ||
| Bench, | ||
| CustomBuild, | ||
| #[serde(untagged)] | ||
| Lib(LibKind), | ||
| #[serde(untagged)] | ||
| Bin(BinKind), | ||
| #[serde(untagged)] | ||
| Other(String), |
There was a problem hiding this comment.
I'm confused by this change doesn't look the same as cargo's TargetKind which is included in the json output
There was a problem hiding this comment.
I tried to have the target kinds separated into the 3 groups like I mentioned here. So just the variant would be need to be checked to know whether a fix can be applied in the current pass or not
There was a problem hiding this comment.
Could we also get rid of Vec from kind: Vec<Kind>,?
I think you had mentioned about it being just a single TargetKind than an vec in cargo.
Since we would need these 3 types to be done together
[ lib ] == [ lib, rlib ] == [ lib, dylib, cdylib ]
There was a problem hiding this comment.
Oh, I had missed the #[serde(untagged)]
This is a nice idea for simplifying the categorization!
The downside is when we merge this into cargo. We'll be wanting to read json messages using cargo's definition. In doing so, we'll have to revert to the original TargetKind and a new enum and adapt between the two. Might be good for us to skip ahead to us creating a new enum independent of the json output for use in processing, making the adoption into cargo easier.
There was a problem hiding this comment.
Could we also get rid of Vec from kind: Vec,?
I think you had mentioned about it being just a single TargetKind than an vec in cargo.
Unsure about it only ever having one element. I'd have to look at notes on that or dig back into the code. However, it is only one element for all cases but Lib which is more of an Other branch, see https://github.com/rust-lang/cargo/blob/0bfd820f837d32f1704561de8caaff10fcf2b57f/src/cargo/core/manifest.rs#L226-L241
There was a problem hiding this comment.
Do you have any suggestions on how I should redesign this? While comparing a BuildUnit, I would need to compare the whole build unit except TargetKind(& CrateType?) and then compare the TargetKind to determine if it should run in the current pass
There was a problem hiding this comment.
Sorry, I had thought it was assumed. We should have our own data structure for identifying the need for a unique Fix Pass.
| current_target = None; | ||
| iteration = 0; | ||
| } else { | ||
| break; | ||
| seen.insert(target.clone()); | ||
| } | ||
| iteration = 0; | ||
| current_target = IndexSet::new(); |
There was a problem hiding this comment.
Like moving iteration = 0; out of the if/for, could you move minor changes in this PR into refactor commits before hand to help make reviewing this commit easier?
|
Sorry, looks like I forgot about us double checking how some corner cases might look like and if they affect this. Started a conversation at I forgot there might be corner cases with fixing in parallel, see rust-lang/cargo#13214 (comment) |
2 participants
Fixes #52