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.

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

Include dependent vendored frameworks in linker flags #9045

Merged
merged 1 commit into from Sep 13, 2019
Merged

Include dependent vendored frameworks in linker flags #9045

merged 1 commit into from Sep 13, 2019

Conversation

drcapulet
Copy link
Contributor

@drcapulet drcapulet mentioned this pull request Jul 25, 2019
Merged
@dnkoutso
Copy link
Contributor

I'd like @segiddins to review this since he made the change in #8314

@dnkoutso dnkoutso added this to the 1.8.0 milestone Jul 25, 2019
@dnkoutso dnkoutso requested a review from segiddins July 25, 2019 02:03
@amorde
Copy link
Member

amorde commented Jul 25, 2019

+1 on @dnkoutso's comment, we had a bunch of regressions when changing this so would like to be 100% sure we get this right

@drcapulet
Copy link
Contributor Author

Of course! Based on some cursory glancing, it should fix #8519.

@dnkoutso
Copy link
Contributor

Does it break #8204 which is what #8314 fixed?

@drcapulet
Copy link
Contributor Author

No, I don't think so - that was specifically related to libraries instead of frameworks. This only targets frameworks that wouldn't otherwise be compiled since those will get included in the "Link Binary With Libraries" phase, but I'm definitely no expert on all this stuff.

segiddins
segiddins reviewed Jul 30, 2019
View reviewed changes
.gitmodules Outdated
@@ -6,7 +6,7 @@
url = https://github.com/tonymillion/Reachability.git
[submodule "spec/cocoapods-integration-specs"]
path = spec/cocoapods-integration-specs
url = https://github.com/CocoaPods/cocoapods-integration-specs.git
Copy link
Member

Choose a reason for hiding this comment

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

will need to point back to master

Copy link
Contributor

Choose a reason for hiding this comment

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

yes @drcapulet will need to revert this

lib/cocoapods/target/build_settings.rb Outdated
if non_library_xcconfig?
frameworks.concat dependent_targets_to_link.flat_map { |pt| pt.build_settings.frameworks_to_import }
end
targets_to_link = library_xcconfig? ? dependent_targets.reject(&:should_build?) : dependent_targets_to_link
Copy link
Member

Choose a reason for hiding this comment

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

for unbbuilt dependent targets, should we only be linking dynamic frameworks here, instead of dynamic and static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, updated.

@dnkoutso
Copy link
Contributor

dnkoutso commented Aug 9, 2019

@drcapulet needs a rebase to fix CHANGELOG conflict.

@dnkoutso
Copy link
Contributor

dnkoutso commented Aug 9, 2019

Also would need to rebase off of 1-8-stable and point to that branch instead to have this change released for 1.8.0.beta.2.

@drcapulet drcapulet changed the base branch from master to 1-8-stable August 9, 2019 19:54
@dnkoutso dnkoutso mentioned this pull request Aug 11, 2019
Closed
dnkoutso
dnkoutso reviewed Aug 20, 2019
View reviewed changes
lib/cocoapods/target/build_settings.rb
if library_xcconfig?
# We know that this library target is being built dynamically based
# on the guard above, so include any vendored static frameworks.
frameworks.concat vendored_static_frameworks.map { |l| File.basename(l, '.framework') } if target.should_build?
Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean that a vendored static framework will be linked twice? Once for this dynamic framework and once on the app target itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is just moved around, but no - since this is a dynamic library target, when it gets compiled it needs to include any vendored static frameworks, and then the whole dynamic library will get linked into it's parent (either app or another library).

Copy link
Contributor

Choose a reason for hiding this comment

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

If I recall correctly CocoaPods will link that vendored static framework to the app target as well not just the dynamic library target.

From its early days CocoaPods had a very (kinda annoying) naive approach to just link everything to the app level target.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can verify my own question this week.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you are right this was already happening before. I will still verify though

@dnkoutso
Copy link
Contributor

@drcapulet needs a rebase. I will try to land it for a beta.2.

@drcapulet drcapulet force-pushed the alexc-link-vendor branch 2 times, most recently from 44d31a2 to f00074d Compare August 23, 2019 16:17
@dnkoutso
Copy link
Contributor

dnkoutso commented Sep 9, 2019

@drcapulet can you rebase to fix the CHANGELOG conflict and we can merge?

@dnkoutso dnkoutso merged commit 1e86e46 into CocoaPods:1-8-stable Sep 13, 2019
@drcapulet drcapulet deleted the alexc-link-vendor branch September 17, 2019 15:20
@ryang1428
Copy link

Still having issues installing vendors frameworks in 1.8.0 :/

@ryang1428
Copy link

ryang1428 commented Oct 2, 2019 •
edited

@dnkoutso should I open a new issue? I am still having issues installing vendored frameworks via 1.8.1.. We are still locked down to using 1.5.3. Getting the same compile time errors:

Undefined symbols for architecture x86_64:
  "_OBJC_CLASS_$_CardIO", referenced from:
      objc-class-ref in AppCoordinator.o
      objc-class-ref in VoiceController.o
      objc-class-ref in CheckController.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

@ryang1428 ryang1428 mentioned this pull request Oct 9, 2019
Closed
1 task
@icecrystal23
Copy link
Contributor

I think this may be causing us to have duplicate symbol errors, as most of our vendored frameworks are static and therefore we do NOT want them included in linker flags outside the project vendoring them.

I will investigate and create an issue (and maybe a PR to fix) if I can repro for you.

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

@dnkoutso dnkoutso dnkoutso left review comments

@segiddins segiddins segiddins left review comments

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

Successfully merging this pull request may close these issues.

None yet
6 participants
@drcapulet @dnkoutso @amorde @ryang1428 @icecrystal23 @segiddins