Fix wrong build order with nested dependencies #702
Conversation
|
Oh, latest master aed3810 produces the after result without the fix, but I don't know why. 😲 |
|
I confirmed that the behavior is changed at 9d1a67f. |
|
That was really affected by the evaluation order of the set, which is changed in 9d1a67f by the change of |
| } | ||
|
|
||
| for node in edge { | ||
| if let nodeDependencies = self.edges[node] where edgeDependsOn(nodeDependencies, dependsOn: dependsOn) { |
There was a problem hiding this comment.
I think it might be better to handle the nesting in addNode. What do you think?
There was a problem hiding this comment.
It would be better and simpler, I'll try it.
I don't understand what you're saying here. Can you elaborate?
|
|
Thank you for the feedback,
what I wanted to say is that the initial order of the collection affects a final result before:
This is more correct explanation about the situation. 👍 |
|
I've updated with the way as you proposed. 🙇 |
| itsDependencies.insert(node) | ||
| edges[ancestor] = itsDependencies | ||
| } | ||
| } |
There was a problem hiding this comment.
Should this also check to see if the nodes in nodeSet are in edges?
What happens if you call:
addNode(Box, dependencyOf: ReactiveCocoa)
addNode(ReactiveCocoa, dependencyOf: ReactiveTask)
?
Or do we know that these will always be ordered in the other direction?
There was a problem hiding this comment.
I'm sorry, I don't understand correctly what you mean and your concern. Could you give me more details?
There was a problem hiding this comment.
let resolver1 = Resolver()
resolver1.addNode(Box, dependencyOf: ReactiveCocoa)
resolver1.addNode(ReactiveCocoa, dependencyOf: ReactiveTask)
let resolver2 = Resolver()
resolver2.addNode(ReactiveCocoa, dependencyOf: ReactiveTask)
resolver2.addNode(Box, dependencyOf: ReactiveCocoa)
resolver1.edges == resolver2.edges // is this true?There was a problem hiding this comment.
I understand, appreciate your input. 🙏
resolver1.edges == resolver2.edges // is this true?
That was false, I added a fix for the case you pointed out here.
|
Thanks for fixing this! ✨ |
Fix wrong build order with nested dependencies
|
Thanks for your review! 🎉 |
|
I don't think this has been fixed yet. Here is top level cartfile: here is the cartfile.resolved: And here is the cartfile for some/privateRepo: Note that some/privateRepo has a dependency on Argo and JVFloatLabeledTextField, which is why they appear before that repo in the top level cartfile. Yet some/privateRepo is the second item that gets built after Argo, which obviously fails since the dependencies haven't been built yet. Also note that unless I include Argo and JVFloatLabeledTextField in the top level cart file, they will be completely disregarded, even though some/privateRepo does specify them in its cart file. Maybe I'm misunderstanding what the fix was supposed to address? I'm currently using carthage version 0.10.0. |
|
I'm running carthage 0.16.2 on OS X 10.11.4 and am seeing this behavior. my cart file looks like this: The cartfile.resolved looks like this: It tried the trick of editing cartfile.resolved, to this: and then running the command "carthage build --platform iOS" but that made no difference, the build ordering stayed the same, and failed: Dons-MBP:Project$ carthage build --platform iOS The following build commands failed: The following build commands failed: |
|
@dfclark Can you please open a new issue with the details of what you're seeing? Commenting on an old issue or pull request makes it very difficult to track issues. |
Should fix #415.
The results in
Cartfile.resolvedwithgithub "Carthage/Carthage" "master":