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

Swift Static Library support #6966

Merged
merged 42 commits into from Feb 20, 2018

Conversation

Projects
None yet
10 participants
@dantoml
Member

dantoml commented Aug 18, 2017

Closes #6899.

  • Examples that build static swift
  • New integration specs
  • Validator to ensure no swift pod targets depend upon built targets that don't define modules
  • CHANGELOG entry
  • Allow enabling/disabling module map generation for all pods in a target from the Podfile
  • Allow enabling/disabling module map generation for one pod in a target from the Podfile
  • Move the generated umbrella header to be in the public headers directory rather than private headers?
@keith

This comment has been minimized.

Member

keith commented Aug 18, 2017

This is fantastic! I'm testing it out now, the first big issue I've noticed is that Swift pods with Swift dependencies need their dependencies' build paths in their SWIFT_INCLUDE_PATHS.

@keith

This comment has been minimized.

Member

keith commented Aug 18, 2017

Here's a sample project for this TestSwiftNestedDependencies.zip (looks like Sonar doesn't actually compile in Swift 4, but it still shows the issue).

This can be fixed by adding ${PODS_CONFIGURATION_BUILD_DIR}/Alamofire to Sonar's SWIFT_INCLUDE_PATHS

@keith

This comment has been minimized.

Member

keith commented Aug 18, 2017

The next issue I've noticed is that if you have a Swift pod that depends on an Objective-C pod, it has trouble building the module:

image

Here's an example project TestObjectiveCDependency.zip

In this project I've added the SWIFT_INCLUDE_PATHS mentioned above. I've solved a similar problem like this before by making the path in the module map absolute, but that doesn't seem to work in this case. Either way I assume there's a better solution for that 🤔

@dantoml

This comment has been minimized.

Member

dantoml commented Aug 19, 2017

@keith Both of those projects build for me now 👍.

@keith

This comment has been minimized.

Member

keith commented Aug 19, 2017

Nice! Looks like that works on my end too. I noticed that specs using module_name don't seem to set the PRODUCT_NAME correctly. It looks like you explicitly commented that out though? If you want a test pod for that ModelMapper defines the module name as Mapper

@dantoml dantoml force-pushed the dani_static_swift branch from 73227d6 to c20ac94 Aug 19, 2017

@dantoml

This comment has been minimized.

Member

dantoml commented Aug 19, 2017

@keith I wasn't sure if it was going to be needed, but looks like it will be 👍

@keith

This comment has been minimized.

Member

keith commented Aug 19, 2017

Next thing I ran into is it seems that the Facebook SDK fails to build statically because of some non modular include warnings. I don't see this when using frameworks today. This obviously could 100% be on them, but figured it was worth mentioning. Here's a sample project: TestFacebookSDK.zip

@keith

This comment has been minimized.

Member

keith commented Aug 19, 2017

Hmm actually it looks like it's happening with other Objective-C pods we're using as well. I can give another project example if needed.

@dantoml

This comment has been minimized.

Member

dantoml commented Aug 19, 2017

@keith Good to know - a smaller example than FBSDK would be handy, but I think that's because I'm not preserving header paths properly yet.

@keith

This comment has been minimized.

Member

keith commented Aug 19, 2017

Here's a slightly smaller example. It appears to be an issue with imports referencing nested Objective-C dependencies, so this example also has 2 dependencies: TestLightstep.zip

@keith

This comment has been minimized.

Member

keith commented Aug 19, 2017

Ah the problem appears to be the imports in the umbrella header generated by CP. In both of these sample projects, commenting all the imports in that file out makes them compile.

@dnkoutso dnkoutso added this to the 1.4.0 milestone Sep 1, 2017

@fabb

This comment has been minimized.

fabb commented Sep 6, 2017

Is there a pre version of cocoapods where static swift libs can be tried out?

@chenxiao0228

This comment has been minimized.

chenxiao0228 commented Sep 6, 2017

@dantoml I tried your branch and applied it to our large scale project. In addition to your changes, I also made following changes for our project to build:

  • module_map.rb "framework module" -> "module" for static lib
  • pod_xcconfig.rb add dependent_targets' configuration_build_dir to HEADER_SEARCH_PATHS for Xcode to be able to find modulemap and umbrella headers
  • xcconfig_helper.rb add "-import-underlying-module" for swift/objc hybrid libs
  • tweaks in pod_target_installer.rb

There is however one issue left: make a swift class visible to ObjC class which depends on the lib. In theory this requires @import module -> modulemap -> umbrella -> module-Swift.h inclusion chain. I can copy it from derived-object folder but I am not sure if it's the best approach.

I attached my diff. It's just a quick hack and really messy. Let me know if you wanna collaborate.

@Pearapps

This comment has been minimized.

Pearapps commented Sep 25, 2017

Is this branch still being updated? Is there anything we can do to help move this along?

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Sep 25, 2017

@Pearapps yes! if you feel you can move this faster than the current pace feel free to pick it up and get it past the finish line.

As a reminder, most of the work in CocoaPods is done from people's free time. There is never a deadline or strict timeline to get things done.

@Pearapps

This comment has been minimized.

Pearapps commented Sep 25, 2017

I opened up a PR against this that uses that diff that @chenxiao0228 put up in a previous comment #7067

@dantoml

This comment has been minimized.

Member

dantoml commented Oct 3, 2017

Sorry for the silence on this, I've been super busy lately, and CocoaPods feature work has taken a back seat.

I'm taking another look at this today.

@dantoml

This comment has been minimized.

Member

dantoml commented Oct 3, 2017

@chenxiao0228 Thanks for that diff, coercing module maps into the right format was one of the things I was wondering about.

@dantoml dantoml force-pushed the dani_static_swift branch from c20ac94 to 9d9e561 Oct 3, 2017

@@ -88,6 +88,9 @@ def custom_build_settings
settings['PRIVATE_HEADERS_FOLDER_PATH'] = framework_name + '.framework' + '/PrivateHeaders'
end
else
# if target.uses_swift?

This comment has been minimized.

@segiddins

segiddins Oct 4, 2017

Member

commented code

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Oct 11, 2017

Punted to 1.5.0.

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Jan 20, 2018

Reviving this for 1.5.0 now that 1.4.0 shipped.

@segiddins segiddins force-pushed the dani_static_swift branch from 9d9e561 to 3e826eb Jan 24, 2018

segiddins added some commits Feb 1, 2018

[PodTargetInstaller] Set product / module name directly on the target
xcodebuild fails if a framework target has PRODUCT_MODULE_NAME
Revert target name changes
While well-intentioned, they broke cross-project target dependency detection
[PodTargetInstaller] Generate module maps that exclude other target’s…
… umbrella headers

This allows us to avoid incomplete umbrella header warnings
[PodTarget] Only create module maps for targets that define a module
By default, targets integrated as static libraries _will not_ get a module map, unless they opt-in by including `DEFINES_MODULE = YES` in their specification's `pod_target_xcconfig`
Ensure static library module maps are declared as [system]
This is to mirror our use of -isystem for header search paths, making it so that headers imported via <> have warnings supressed

@segiddins segiddins force-pushed the dani_static_swift branch from 139de10 to 70b5bb9 Feb 20, 2018

@segiddins

This comment has been minimized.

Member

segiddins commented Feb 20, 2018

Rebased, this is basically ready to go!

@segiddins segiddins force-pushed the dani_static_swift branch from 70b5bb9 to 11c9226 Feb 20, 2018

@segiddins segiddins merged commit d7c0b2b into master Feb 20, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arithma

This comment has been minimized.

arithma commented Mar 11, 2018

Question: Will this resolve the issue with transitive dependency stuff?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment