Fix dependency resolution when building #1139
Conversation
As of Carthage#1100, dependency resolution for the "build" command uses a comparison sort to resolve ordering of dependencies that need to be built. This change does not build dependencies in the correct order in significantly complex dependency graphs, causing "module not found" errors. To resolve this, Kahn's topological sort is used in place of a comparison sort to ensure that the dependency graph is built in the correct order. In our test project with 43 unique dependencies (including transitive dependencies), this change fixed the "module not found" issues. Additionally, since this is a generic algorithm, it can be easily tested (once I get some guidance on a place to put it).
erichoracek
force-pushed
the
fix-build-dependencies
branch
from
4455620
to
be73616
Compare
| } | ||
| .reduce([:]) { (working: DependencyGraph, next: DependencyGraph) in | ||
| var result = working | ||
| next.forEach { result.updateValue($1, forKey: $0) } |
There was a problem hiding this comment.
Could you make these named parameters? I think it will make it more obvious how this works.
There was a problem hiding this comment.
I think we could actually use mergeGraphs here instead. It doesn't seem like this properly handles all cases of shared, recursive dependencies as-is.
Also, I had no idea you could use updateValue like this. 😕
I left a few notes about the structure and some of the details. I still need to look over |
| // Open Question: Is there a place that a generic topological | ||
| // sort should live? | ||
| func topologicalSort<Node: Equatable>(edges: Dictionary<Node, Set<Node>>, @noescape isOrderedBefore: (Node, Node) -> Bool) -> [Node] { | ||
| var nodeQueue: [Node] = edges.filter { $1.isEmpty }.map { return $0.0 } |
There was a problem hiding this comment.
I'd prefer to use named variables in the blocks. I think that makes it easier to read the code. It will probably make sense to split the filter and map on separate lines with the added characters.
|
Great stuff. ⚡ I left some comments that I think would make the code more readable, but the implementation looks spot on. If you're up for it, I'd love to see tests around |
| } | ||
| } | ||
| return result | ||
| .reduce([:]) { (working: DependencyGraph, next: DependencyGraph) in |
There was a problem hiding this comment.
@mdiep I couldn't figure out how to use mergeGraphs here—it would seem that there's two types of DependencyGraph in CarthageKit.
There was a problem hiding this comment.
Oh, that's right. This is just a typealias in this method. 😜
|
This is looking really great. Just a few more small issues. |
|
Awesome. ⚡ |
Fix dependency resolution when building
|
👏 |
As of #1100, the
buildcommand uses a comparison sort to resolve ordering of dependencies to build. That change resulted in frameworks being built out of order in significantly complex dependency graphs, causing "module not found" errors.To resolve this, Kahn's topological sort is used in place of a comparison sort to ensure that the dependency graph is built in the correct order.
In our project with 43 unique dependencies (including transitive dependencies), this change fixed "module not found" errors when using the build command in the most recent release. Additionally, since this is a generic algorithm, it can be easily tested (assuming there is a reasonable place to put it). It would appear #1100 removed the previous tests around build order.
This PR has some open questions:
buildcommand when a cycle is detected during a topological sort?