Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transitively Link Static Dependency's Dynamic Dependencies Correctly #484

Merged
merged 5 commits into from Aug 21, 2019

Conversation

adamkhazi
Copy link
Contributor

Resolves #455

Short description 📝

When a static dependency has a dynamic dependencies, those dynamic dependencies need to be linked at the point the static dependency is linked.

This PR fixes the issue where a static dependency's dynamic libraries were linking incorrectly.

Solution 📦

As we already traverse the graph for static dependencies, we can add a step to this process to check each static dependency's targetDependencies for dynamic libraries and add them as a reference adjacent to where the original static dependency is referenced.

Implementation 👩‍💻👨‍💻

  • Update linkableDependencies in Graph.swift to reference a static dependency's dynamic libraries
  • Update GraphTests to check the test case:
  • A (App) -> B (Static) -> C (Dynamic)
  • Expected: A -> C
  • Update GraphTests to check the test case:
  • A (App) -> B (Dynamic) -> C (Static) -> D (Static) -> E (Dynamic)
  • Expected: A -> B
  • Expected: B -> (C, D, E)
  • Update GraphTests to check the test case:
  • A (App) -> B (Dynamic) -> C (Dynamic) -> D (Static) -> E (Static) -> F (Dynamic)
  • Expected: B -> C

Test Plan

  • Run tuist generate inside fixtures/ios_app_with_static_frameworks
  • Check that the App target has D inside Build Phases -> Link Binary With Libraries

@adamkhazi adamkhazi changed the title Transitively Link Static Dependency's Dynamic Dependencies Transitively Link Static Dependency's Dynamic Dependencies Correctly Aug 20, 2019
@tuistbot
Copy link
Contributor

1 Warning
⚠️ Have you introduced any user-facing changes? If so, please take some time to update the documentation. Keeping the documentation up to date makes it easier for users to learn how to use Tuist.

Generated by 🚫 Danger

@adamkhazi
Copy link
Contributor Author

The failing test passes locally I think

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice work! You can push an empty commit to re-trigger CI

.map { DependencyReference.product(target: $0.target.name) }
var staticLibraries = [DependencyReference]()

findAll(targetNode: targetNode, test: isStaticLibrary, skip: isFramework).forEach {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could keep the static libraries related code local to this scope via some minor tweaks:

e.g.

let staticLibraryTargetNodes = findAll(targetNode: targetNode, test: isStaticLibrary, skip: isFramework)
let staticLibraries = staticLibraryTargetNodes.map {
     DependencyReference.product(target: $0.target.name)
}

let staticDependenciesDynamicLibraries = staticLibraryTargetNodes.flatMap {
    $0.targetDependencies
           .filter(or(isFramework, isDynamicLibrary))
           .map { DependencyReference.product(target: $0.target.name) }
 }

references = references.union(staticLibraries + staticDependenciesDynamicLibraries)
...

I guess that sorta goes against the sections marked out in comments - though those could change too if that helps

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had something similar before but changed it due to the comments in the code. I'll change the comments and go by your suggestion.

@codecov
Copy link

codecov bot commented Aug 21, 2019

Codecov Report

Merging #484 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #484      +/-   ##
==========================================
+ Coverage    92.4%   92.45%   +0.04%     
==========================================
  Files         348      348              
  Lines       17809    17914     +105     
==========================================
+ Hits        16457    16562     +105     
  Misses       1352     1352
Impacted Files Coverage Δ
Sources/TuistGenerator/Graph/Graph.swift 97.76% <100%> (+0.04%) ⬆️
Tests/TuistGeneratorTests/Graph/GraphTests.swift 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71a3ec0...cb19a4f. Read the comment docs.

@adamkhazi adamkhazi merged commit cbc4780 into tuist:master Aug 21, 2019
@adamkhazi adamkhazi deleted the bugfix/transitive-dyn-libs branch August 21, 2019 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transitively link static dependency's dynamic dependencies
4 participants