Fix aggregate target dependencies with transitivelyLinkDependencies
#383
Conversation
|
Could you add a changelog entry |
| @@ -981,6 +981,9 @@ public class PBXProjGenerator { | |||
| queue.append(dependencyTarget) | |||
| } | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
Would be good if the code showed what was happening as well as the comment. For completion sake could we add a Project.getAggregateTarget(_ name: String) -> AggregateTarget function and use it here
} else if project.getAggregateTarget(dependency.reference) != nil {
|
Good catch 👍 |
brentleyjones
force-pushed
the
transitive-aggragate-targets
branch
from
6fc7f42
to
1aa3957
Compare
|
Done and done 😄. |
| private var targetsMap: [String: Target] | ||
| private var aggregateTargetsMap: [String: AggregateTarget] |
There was a problem hiding this comment.
One missing map creation in the aggregateTargets setter
There was a problem hiding this comment.
Oops. Fixed.
brentleyjones
force-pushed
the
transitive-aggragate-targets
branch
from
79cd59f
to
7140f0d
Compare
|
Actually, looks like |
|
I hadn't considered that. I'll fix that quick. Related, in my Static Framework work, I'll be consolidating those and doing the caching work we talked about. |
|
Awesome. Yeah those 2 could be combined and optimised by only running over the graph once 👍 |
|
Updated for Carthage dependencies as well. |
| queue.append(target) | ||
| } else if let aggregateTarget = projectTarget as? AggregateTarget { | ||
| for dependencyName in aggregateTarget.targets { | ||
| if let projectTarget = project.getProjectTarget(dependencyName) { |
There was a problem hiding this comment.
This highlights a question. Should getAllDependenciesPlusTransitiveNeedingEmbedding be checking aggregateTarget.targets as well?
There was a problem hiding this comment.
I went down that route at first. It wouldn't since it's the same as shouldEmbedDependencies in my mind. An aggregate target will be merging it's targets, they shouldn't propagate up, at least automatically. If that's the desire, it can still be done manually.
In the new algorithm this will be expressible explicitly.
|
Released 👍 |
Fixes #381 to work with the
transitivelyLinkDependenciesflag.