8000 Fix static_framework Swift pod deps and pod access to dep vendored modules by paulb777 · Pull Request #7135 · CocoaPods/CocoaPods · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix static_framework Swift pod deps and pod access to dep vendored modules #7135

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 13, 2017
Merged

Fix static_framework Swift pod deps and pod access to dep vendored modules #7135

merged 1 commit into from
Oct 13, 2017

Conversation

paulb777
Copy link
Member

Fixes #7117

This PR fixes/implements two issues:

  1. When I implemented static frameworks, I made the incorrect assumption that static_frameworks don't need to worry about build dependencies because they don't link. That's not the case when a static_framework depends upon a Swift CocoaPod where it needs its bridging header to be built. This is fixed by the conditional change in lib/cocoapods/installer/xcode/pods_project_generator.rb.

  2. CocoaPods did not support importing modules from vendored frameworks. While this is annoying for Objective C, it's a show stopper for a Swift CocoaPod depending on a vendored framework. The vendored framework modules were not being added to FRAMEWORK_SEARCH_PATHS for pod builds that depended upon them. I added a call in lib/cocoapods/generator/xcconfig/pod_xcconfig.rb to generate_vendored_build_settings. Most of the changes to lib/cocoapods/generator/xcconfig/xcconfig_helper.rb are renaming and filtering so that FRAMEWORK_SEARCH_PATHS can be updated with or without updating OTHER_LD_FLAGS.

@dnkoutso dnkoutso added this to the 1.4.0 milestone Oct 13, 2017
dnkoutso
dnkoutso reviewed Oct 13, 2017
View reviewed changes
lib/cocoapods/generator/xcconfig/pod_xcconfig.rb Outdated
@@ -71,6 +71,7 @@ def generate
XCConfigHelper.add_target_specific_settings(target, @xcconfig)
recursive_dependent_targets = target.recursive_dependent_targets
@xcconfig.merge! XCConfigHelper.search_paths_for_dependent_targets(target, recursive_dependent_targets, @test_xcconfig)
XCConfigHelper.generate_vendored_build_settings(target, recursive_dependent_targets, @xcconfig, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this always happen for all pod targets? For example what if someone does not use use_frameworks! and instead use static libs? Does the same apply?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I added the check.

It also changed the back the different order on the adds settings for test dependent targets excluding the parents targets unit test to being unchanged from master

dnkoutso
dnkoutso reviewed Oct 13, 2017
View reviewed changes
lib/cocoapods/installer/xcode/pods_project_generator.rb
@@ -229,10 +229,11 @@ def set_target_dependencies
aggregate_target.native_target.add_dependency(pod_target.native_target)
configure_app_extension_api_only_for_target(pod_target) if is_app_extension

add_dependent_targets_to_native_target(pod_target.dependent_targets,
Copy link
Contributor
@dnkoutso dnkoutso Oct 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

is there a test that covers this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't been able to figure out how the tests in pods_project_generator_spec.rb do validation to test Xcode project contents.

Do you have any guidance?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes I remember now this is a bit difficult to write but doable.

Look into:

          describe '#set_test_target_dependencies' do
            ...
          end

Most of it is mocked and ensures all mocks receive the correct method calls with parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! That helped.

@dnkoutso
Copy link
Contributor

The xcconfig_helper.rb is getting convoluted. I've been thinking of redesigning a lot of it, perhaps by providing an "IntegrationStrategy" class in which different pod targets are integrated differently (static lib, static framework, dynamic framework etc.)

Not for now just vending on the complexity of xcconfig_helper.rb

@paulb777
Copy link
Member Author

Agreed on the need to rework xcconfig_helper.rb and the defining of FRAMEWORK_SEARCH_PATHS and 'OTHER_LD_FLAGS`

dnkoutso
dnkoutso reviewed Oct 13, 2017
View reviewed changes
spec/unit/generator/xcconfig/pod_xcconfig_spec.rb Outdated
@xcconfig.to_hash['FRAMEWORK_SEARCH_PATHS'].should.not.include('${PODS_ROOT}/DDD')
end

it 'it vendored frameworks dont get added to frameworks paths if use_frameworks! isnt set' do
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove second it

dnkoutso
dnkoutso reviewed Oct 13, 2017
View reviewed changes
CHANGELOG.md Outdated
@@ -8,6 +8,10 @@ To install release candidates run `[sudo] gem install cocoapods --pre`

##### Enhancements

* Fix static_framework Swift pod dependencies and implement pod access to dependent vendored_framework modules
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: probably place this in the Bug Fixes section can include #7117 link instead

dnkoutso
dnkoutso reviewed Oct 13, 2017
View reviewed changes
lib/cocoapods/generator/xcconfig/xcconfig_helper.rb
file_accessor.vendored_static_frameworks.each do |vendored_static_framework|
adds_other_ldflags = XCConfigHelper.links_dependency?(aggregate_target, pod_target)
Copy link
Contributor

Choose a reason for hiding this comment

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

ha good catch, no need to call this twice

@dnkoutso
Copy link
Contributor

ah sorry @paulb777 I merged the other PR first and this one needs rebase.

@paulb777
Copy link
Member Author

np @dnkoutso I needed to squash anyways.

It's now squashed and rebased and should be good to go on green.

Thanks for the prompt, helpful reviews today! 👍

@dnkoutso dnkoutso merged commit 1b834cf into CocoaPods:master Oct 13, 2017
@paulb777 paulb777 mentioned this pull request Oct 14, 2017
Closed
@mukeshthawani
Copy link

Thanks @paulb777 for fixing this, I will give it a try and let you know.

@dnkoutso dnkoutso mentioned this pull request Oct 15, 2017
Closed
1 task
@paulb777 paulb777 deleted the sf-swift branch October 18, 2017 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnkoutso dnkoutso dnkoutso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.4.0
Development

Successfully merging this pull request may close these issues.

Using static framework gives error
3 participants
@paulb777 @dnkoutso @mukeshthawani
0