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

Prevent embedding static frameworks #490

Merged

Conversation

kwridan
Copy link
Collaborator

@kwridan kwridan commented Aug 29, 2019

Resolves #454

Short description πŸ“

Precompiled static frameworks were being embedded when generating projects.

Solution πŸ“¦

  • Ensure the linkage of precompiled frameworks is checked to determine if they need to be embedded

Notes πŸ“:

  • All FrameworkNodes (i.e. prebuilt frameworks) were being embedded regardless of their linkage
  • This check was redundant as there was a more general check for PrecompiledNode which happens to be the super class of FrameworkNode which took into account the framework linkage
  • To resolve this issue it was suffice to remove the redundant check

Implementation πŸ‘©β€πŸ’»πŸ‘¨β€πŸ’»

  • Add fixture
  • Update Graph embeddable references logic
  • Update changelog

Test Plan πŸ› :

  • Generate fixtures/ios_app_with_static_frameworks via running tuist generate
  • Verify the generated project doesn't embed PrebuiltStaticFramework
  • Verify the generated project builds successfully

Resolves tuist#454

- All `FrameworkNodes` (i.e. prebuilt frameworks) were being embedded regardless of their linkage
- This check was redundant as there was a more general check for `PrecompiledNode` which happens to be the super class of `FrameworkNode` which took into account the framework linkage
- To resolve this issue it was suffice to remove the redundant check

Test Plan:

- Generate `fixtures/ios_app_with_static_frameworks` via running `tuist generate`
- Verify the generated project doesn't embed `PrebuiltStaticFramework`
- Verify the generated project builds successfully
@kwridan kwridan requested a review from zdnk August 29, 2019 22:06
@@ -309,15 +309,6 @@ class Graph: Graphing {

references.append(contentsOf: otherTargetFrameworks)

/// Pre-built frameworks
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Line 299 takes care of embedding pre-compiled nodes (which include FrameworkNode) while checking their linkage (dynamic vs static)

@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

@codecov
Copy link

codecov bot commented Aug 30, 2019

Codecov Report

Merging #490 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #490      +/-   ##
=========================================
- Coverage    92.6%   92.6%   -0.01%     
=========================================
  Files         347     347              
  Lines       17799   17815      +16     
=========================================
+ Hits        16483   16497      +14     
- Misses       1316    1318       +2
Impacted Files Coverage Ξ”
Sources/TuistGenerator/Graph/Graph.swift 97.67% <100%> (-0.1%) ⬇️
Tests/TuistGeneratorTests/Graph/GraphTests.swift 100% <100%> (ΓΈ) ⬆️
Sources/TuistCore/Extensions/Sequence+Filter.swift 0% <0%> (-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 bc1fb58...d24c2ef. Read the comment docs.

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Good catch & fix @kwridan

@kwridan kwridan merged commit c778fed into tuist:master Aug 30, 2019
@kwridan kwridan deleted the fix/prevent-embedding-static-frameworks branch August 30, 2019 10:11
@sgrgrsn
Copy link
Collaborator

sgrgrsn commented Aug 30, 2019

Thanks a lot!

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.

Embedding frameworks wrapping static libraries
4 participants