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

Fixed issue when linking and embeding static frameworks #820

Merged

Conversation

acecilia
Copy link
Collaborator

@acecilia acecilia commented Apr 2, 2020

From what I understand, static frameworks are:

  1. Linked as usual (to be able to use them)
  2. Never embed, as their binary becomes part of the binary that depends on them

For example, given this (SwiftModule1, SwiftModule2 and SwiftModule3 are static frameworks; SnapKit is a dynamic one):

options:
  transitivelyLinkDependencies: true

targets:
  SwiftModule1:
    type: framework.static
    ...
    dependencies:
    - framework: SwiftModule2.framework
      implicit: true
      embed: false
    - framework: ../../Carthage/Build/iOS/SnapKit.framework
  SwiftModule1Tests:
    type: bundle.unit-test
    ...
    dependencies:
    - target: SwiftModule1
    - framework: SwiftModule3.framework
      implicit: true
      embed: false
      

In the target SwiftModule1Tests, when generating the xcode project you find that:

  • Link binary with libraries: SnapKit, SwiftModule1, SwiftModule3
  • Embed frameworks: SnapKit, SwiftModule1

I believe that is wrong, and instead it should be:

  • Link binary with libraries: SnapKit, SwiftModule1, SwiftModule2, SwiftModule3
  • Embed frameworks: SnapKit

@yonaskolb
Copy link
Owner

Thanks @acecilia!
@brentleyjones does this look right to you? You did a lot of the static library work

@acecilia acecilia force-pushed the static_framework_link_and_embed branch from a03083f to 00bc4f3 Compare April 2, 2020 19:40
@acecilia acecilia force-pushed the static_framework_link_and_embed branch from 00bc4f3 to 11bfcdd Compare April 2, 2020 20:08
@acecilia
Copy link
Collaborator Author

acecilia commented Apr 2, 2020

Build not working due to tuist/XcodeProj#536

@acecilia acecilia force-pushed the static_framework_link_and_embed branch from 11bfcdd to 42699db Compare April 2, 2020 23:51
Sources/XcodeGenKit/PBXProjGenerator.swift Outdated Show resolved Hide resolved
Sources/XcodeGenKit/PBXProjGenerator.swift Show resolved Hide resolved
Sources/XcodeGenKit/PBXProjGenerator.swift Show resolved Hide resolved
Sources/XcodeGenKit/PBXProjGenerator.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/XCProjExtensions.swift Show resolved Hide resolved
Sources/ProjectSpec/XCProjExtensions.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/XCProjExtensions.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/XCProjExtensions.swift Outdated Show resolved Hide resolved
@acecilia
Copy link
Collaborator Author

acecilia commented Apr 3, 2020

Oh sorry, I am not familiar with Github PR reviews, and I had comments explaining the code (which I made yesterday) but did not press in "Submit review" :/

Copy link
Collaborator

@brentleyjones brentleyjones left a comment

Choose a reason for hiding this comment

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

After the dependency.embed != true is propagated more (and more tests are written if needed to ensure nothing breaks from that), this is good to good from my PoV. Thanks @acecilia!

@acecilia acecilia force-pushed the static_framework_link_and_embed branch from 42699db to f1b6d66 Compare April 3, 2020 13:53
@acecilia
Copy link
Collaborator Author

acecilia commented Apr 7, 2020

This is ready to go :)

@brentleyjones
Copy link
Collaborator

@acecilia Please rebase to resolve conflicts.

@acecilia acecilia force-pushed the static_framework_link_and_embed branch from f1b6d66 to c17a831 Compare April 7, 2020 15:00
@acecilia
Copy link
Collaborator Author

acecilia commented Apr 7, 2020

@brentleyjones done :)

@brentleyjones brentleyjones merged commit 7afcf1f into yonaskolb:master Apr 7, 2020
@brentleyjones
Copy link
Collaborator

Thanks @acecilia for this fix!

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.

None yet

3 participants