[Fix] Focus Resources on Tests Targets #3571
Conversation
|
LGTM, probably better if @adellibovi and @kwridan have a look as well 👀 |
| @@ -457,7 +457,7 @@ final class BuildPhaseGenerator: BuildPhaseGenerating { | |||
| .sorted() | |||
| var refs = bundles.compactMap { fileElements.product(target: $0.target.name) } | |||
|
|
|||
| if target.product.runnable { | |||
| if target.product.runnable || target.product.testsBundle { | |||
There was a problem hiding this comment.
This may not be 100% correct for example command line executable targets are runnable but do not support resources, the determining factor is target.supportsResources.
Another example is dynamic frameworks, they do support embedding resource bundles however they are not runnable, nor test targets.
Looks like this regressed somewhere along the way, here's the original implementation in a past commit for reference:
tuist/Sources/TuistCore/Graph/Graph.swift
Line 181 in 4a8bfd8
There was a problem hiding this comment.
Actually let me double check, this may not be the right place for this fix, the GraphTraverser should be able to provide the correct results (comparing with the original implementation) without further filtering.
There was a problem hiding this comment.
Looks like GraphTraverser already has the right logic in place
https://github.com/tuist/tuist/blob/main/Sources/TuistCore/Graph/GraphTraverser.swift#L128
@mstfy I spotted those extra bundles being added here
Is this needed? it was my understanding GraphTraverser already returned all the resources bundles applicable for the specified target, perhaps I am missing something here?
There was a problem hiding this comment.
I am not %100 sure but I think resourceBundleDependencies in GraphTraverser checks the actual targets that can produce resource bundles. They are represented as GraphDependency.target . Precompiled bundles are represented as GraphDependency.bundle . So this check produces false for precompiled bundles:
let isBundle: (GraphDependency) -> Bool = {
self.target(from: $0)?.target.product == .bundle
}
self.target(from: $0) returns nil for GraphDependency.bundle
That’s why BuildPhaseGenerator adds compiled bundles to copy files build phase
There was a problem hiding this comment.
Thanks for clarifying @mstfy 👍
Indeed it does, I believe we can update GraphTraverser cater for these cases.
Am I correct in my understanding in the cache scenario we have the following setup?
There was a problem hiding this comment.
Yes this should what graph look like after CacheGraphMutator replaces targets with cached artifacts. I think we should further modify graph to add that Resource bundle as direct dependency of the Target.
| @@ -457,7 +457,7 @@ final class BuildPhaseGenerator: BuildPhaseGenerating { | |||
| .sorted() | |||
| var refs = bundles.compactMap { fileElements.product(target: $0.target.name) } | |||
|
|
|||
| if target.product.runnable { | |||
| if target.product.runnable || target.product.testsBundle { | |||
There was a problem hiding this comment.
Looks like GraphTraverser already has the right logic in place
https://github.com/tuist/tuist/blob/main/Sources/TuistCore/Graph/GraphTraverser.swift#L128
@mstfy I spotted those extra bundles being added here
Is this needed? it was my understanding GraphTraverser already returned all the resources bundles applicable for the specified target, perhaps I am missing something here?
- The `resourceBundleDependencies` method on `GraphTraverser` now accounts for precompiled (cached) resource bundles - The logic was previously partly split between the build phase generator and the graph traverser, now it's consolidated within the traverser - Tests have been extended to account for the precompiled bundle cases - To support this change, the signature for the `resourceBundleDependencies` needed to be updated to return a `GraphDependencyReference` instead of a `GraphTarget` - Tests / Mocks updated accordingly Test Plan: - Verify tests pass - Re-generate the fixture `projects/tuist/fixtures/ios_app_with_framework_and_resources` and verify all the resource bundles match the ones from before these changes - Note: due to the type changes, the sort of the resource bundles in the build phase has changed slightly, but it's expected to remain stable
Resolves #3523
Short description 📝
As per title this PR fixes a crash when focusing on a test framework with resources.
Checklist ✅
CHANGELOG.mdhas been updated to reflect the changes. In case of a breaking change, it's been flagged as such.