Prevent generation of redundant file elements #515
Conversation
|
Is this ready for review @kwridan ? |
|
will rebase it ontop of master, there are a few additional changes needed for the swift packages dependencies too |
kwridan
changed the title
|
|
||
| references.append(contentsOf: otherTargetFrameworks) | ||
|
|
||
| return Set(references).sorted() | ||
| } | ||
|
|
||
| func copyProductDependencies(path: AbsolutePath, target: Target) -> [DependencyReference] { |
There was a problem hiding this comment.
I think copyProductDependencies is a bit ambiguous. What does it mean? What about including a note like this one explaining why this method is useful?
There was a problem hiding this comment.
π added a comment to https://github.com/tuist/tuist/pull/515/files#diff-12f3cd68bf1720345626c27c16f99056R108
| return Set(dependencies).sorted() | ||
| } | ||
|
|
||
| func allDependencyReferences(for project: Project, system: Systeming) throws -> [DependencyReference] { |
There was a problem hiding this comment.
We really need to get rid of the dependency injection for the System class π¬
|
The PR is looking good @kwridan π |
- File elements for all dependencies are products within a graph were always generated and included in projects without applying any filtering - This led to the inclusion of redundant file elements in top level projects (for products & dependencies that they didn't require) - The graph helper methods are now consulted to obtain a list of dependency references that are known to be required by the project - Additional helpers methods on graph were add to help unify / share some of the business logic regarding which dependencies are needed Test Plan: - Generate the fixture `fixtures/ios_app_with_frameworks` via `tuist generate` - Open the generated Workspace - Verify the `Framework3` project doesn't contain file references for `Framework5` - Verify the `Framework3` project doesn't contain file references for `ARKit.framework` - Verify the `Framework4` project doesn't contain file references for `ARKit.framework`
kwridan
force-pushed
the
fixes/redundant-file-elements
branch
from
98f5df6
to
ce93970
Compare
kwridan
changed the title
|
Thanks for taking a look @pepibumur! |
Generated by π« Danger |
Codecov Report
@@ Coverage Diff @@
## master #515 +/- ##
==========================================
- Coverage 91.6% 91.53% -0.08%
==========================================
Files 358 358
Lines 19343 19339 -4
==========================================
- Hits 17720 17701 -19
- Misses 1623 1638 +15
Continue to review full report at Codecov.
|
|
|
||
| references.append(contentsOf: otherTargetFrameworks) | ||
|
|
||
| return Set(references).sorted() | ||
| } | ||
|
|
||
| func copyProductDependencies(path: AbsolutePath, target: Target) -> [DependencyReference] { |
There was a problem hiding this comment.
π added a comment to https://github.com/tuist/tuist/pull/515/files#diff-12f3cd68bf1720345626c27c16f99056R108
| @@ -153,6 +147,20 @@ class ProjectFileElements { | |||
| isReference: $0.isReference) | |||
| }) | |||
|
|
|||
| // Local Packages | |||
There was a problem hiding this comment.
I need to think about this further ...
The inconsistency here is that all other dependencies are resolved via allDependencyReferences whereas this one isn't included there yet.
Resolves #514
Short description π
File elements for all dependencies and products within a graph were always generated and included in projects without applying any filtering.
This led to the inclusion of redundant file elements in top level projects (for products & dependencies that they didn't require)
In this example:
Dependencies:
ARKit.frameworkhoweverThe highlighted elements are redundant
Solution π¦
Leverage the business logic with the
Graphto determine which file references are needed.Implementation π©βπ»π¨βπ»
DependencyReference.productto include product nameGraphto allow listing all dependency references for a projectProjectFileElementsto leverage new helperTest Plan π :
fixtures/ios_app_with_frameworksviatuist generateFramework3project doesn't contain file references forFramework5Framework3project doesn't contain file references forARKit.frameworkFramework4project doesn't contain file references forARKit.framework