Correctly handle a case where build dependencies are also declared as non-build dependencies #1508
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wycats (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see CONTRIBUTING.md for more information. |
| (dep, pkg.dependencies().iter().find(|d| { | ||
| d.name() == dep.name() | ||
| }).unwrap()) | ||
| }); |
There was a problem hiding this comment.
Could this logic be left here actually? It's still used in multiple places below, and the call to find could be replaced with filter and then the cases below could use the iterator if necessary.
There was a problem hiding this comment.
I tried to do that, but then the compiler complains: dep does not live long enough
There was a problem hiding this comment.
You may have to use the move keyword on the closure to ensure it doesn't try to capture a local reference
There was a problem hiding this comment.
OK that's fine. But the cases would be rewritten like this
base.extend(deps.filter(|&(_, dep)| dep.any(|d| d.is_build()))
.map(|(id, _)| (id, Stage::Libraries)));
And the compiler compains again "cannot move out of borrowed content". I tried ref dep and ref mut dep but still it won't work out.
There was a problem hiding this comment.
Could you generate a patch that I could apply locally? I may have to poke a bit as well
There was a problem hiding this comment.
OK. See here master...mohtar:e0779b6
There was a problem hiding this comment.
Ah I see what's going on, I guess this won't work out as nicely as I hoped :(
Oh well, sorry for the runaround!
|
Nice! Thanks for fixing this! Could you add some tests for this as well? |
| .filter(|id| pkg.dependencies().iter() | ||
| .find(|d| d.name() == id.name() && d.is_build()) | ||
| .is_some()) | ||
| .map(|id| (id, Stage::Libraries))); |
There was a problem hiding this comment.
This formatting is a bit confusing because the alignment of .filter with .extend normally indicates that the filter is happening on the return value of extend, perhaps this could be formatted as:
base.extend(deps.filter(|id| {
pkg.dependencies().iter().find(|d| {
d.name() == id.name() && d.is_build()
}).is_some()
}).map(|id| (id, Stage::Libraries)));There was a problem hiding this comment.
Done. I also replaced find(...).is_some() with any(...)
|
I'm really glad someone found the time to pick this up, thanks @mohtar !! I thought I'd found a bug but it was a false alarm. Safe to ignore the rest of this comment. I've pulled your patch and am noticing some failure when building I'm going to try to distill into a failing test case or at the least a minimal cargo project exhibiting the bug. |
|
Sorry, false alarm! This had to do with me trying to compile for a target architecture without updating my |
|
☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-32, cargo-win-64 |
Fix #876.