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

Expose all build configuration for SPM #1426

Merged
merged 2 commits into from
Apr 8, 2020
Merged

Expose all build configuration for SPM #1426

merged 2 commits into from
Apr 8, 2020

Conversation

onevcat
Copy link
Owner

@onevcat onevcat commented Apr 8, 2020

This is a PR related to #1418 and #1423. Xcode 11.4 is not doing well on determining which type of linking (dynamic or static) should be used.

Xcode 11.4 added a new diagnostic on detecting duplicated libraries (as static ones) for Swift Package libraries. The compiler will not be happy if Kingfisher is linked to two targets, with the error:

Swift package product 'Kingfisher' is linked as a static library by {APP_TARGET} and {OTHER_TARGET}. This will result in duplication of library code.

This is only correct when you want to link OTHER_TARGET as static library, and finally, the symbols in OTHER_TARGET will be copied into the final binary. It will cause duplicated code in the final binary.

However, it is not likely a common case in iOS development. Usually, we use OTHER_TARGET as a dynamic framework or it is even an extension target (such as Today Extension or Notification Content Extension).

In theory, we can simply change the Kingfisher as a dynamic library (#1420) to solve it. But Xcode does not allow only linking an SPM library without embedding it now (By changing to "Do Not Embed", the library is also removed from the "Frameworks and Libraries" section). So it would cause another problem that the frameworks are embedded to extension targets (#1423), which prevents you submit your app to App Store. It looks like something like this:

)

In this PR, I exposed all build configurations in the Package.swift file, so the library users can choose freely what they need.

I personally suggest the best approach below:

Best approach

Before Xcode can have a fix for the problems above, the best approach for linking Kingfisher (SPM), and any other libraries created by SPM, should be:

Create a wrapper dynamic framework, then add the plain Kingfisher to it, expose necessary methods from Kingfisher there, then import the framework into other targets like your main app or extensions. (In other words, wrap the SPM Kingfisher library in your own dynamic framework, and import the dynamic framework everywhere else).

Depends on your usage of Kingfisher, sometimes the best approach would require a lot of work. I personally believe Xcode can fix the issue above in a short time (maybe?), so you can also consider these non-ideal "workaround" below. There are two scenarios:

Importing Kingfisher to App & Dynamic Framework

Importing KingfisherDynamic to both. This will make Xcode happy since the dynamic framework does not trigger the newly added "linked as a static library" diagnostic. However, this is not ideal because it increases the size of your final binary (due to you have to embed Kingfisher to both the app target and the framework target. Unfortunately Xcode does not allow us to only linking it yet).

Importing Kingfisher to App & Extension

You have to prevent using KingfisherDynamic for your extension target since Apple does not allow to embed a library there. Instead, you can choose to use KingfisherDynamic for the main app and KingfisherStatic for the extension.

This will allow you to copy the symbols into the extension targets so App Store will be happy with it.

If you have multiple extensions that will use Kingfisher, you also need to set DISABLE_DIAMOND_PROBLEM_DIAGNOSTIC to YES in related targets to prevent the wrong diagnostic. Please check comments in this post for more.

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.

1 participant