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

[Analyzer] Set BUILD_LIBRARY_FOR_DISTRIBUTION for all dependencies if it's set in the user target #9693

Merged
merged 5 commits into from
Apr 8, 2020

Conversation

juanjonol
Copy link
Contributor

As discussed in #9232, this PR checks if the BUILD_LIBRARY_FOR_DISTRIBUTION Build Setting evaluates to YES in an user target, and in that case it applies this setting to all the dependencies of this target. This is needed to avoid issues with XCFrameworks.

This also checks if BUILD_LIBRARY_FOR_DISTRIBUTION is set in a .xcconfig, as proposed for APPLICATION_EXTENSION_API_ONLY in #9233.

This is both my first PR and my first time working with Ruby, so I heavily based this PR in the previous work with the APPLICATION_EXTENSION_API_ONLY Build Setting. I hope I didn't miss anything critical.

BTW, thank you for your amazing work in CocoaPods!

If BUILD_LIBRARY_FOR_DISTRIBUTION is present in a target (directly in its Build Settings or in an .xcconfig), this setting is also set for all its dependencies. Closes CocoaPods#9232.

This commit is based in the following commits:

- “Search in users xcconfig’s for figuring out when to set `APPLICATION_EXTENSION_API_ONLY`.WIP” (375c2e1)
- “make `Target#application_extension_api_only` a readonly prop” (814cd03)
- “make APPLICATION_EXTENSION_API_ONLY not break when performing a cached install” (c319a90)
- “[Installer] Add spec for setting APPLICATION_EXTENSION_API_ONLY when the user target has it set” (7e7c3d7, Pull Request CocoaPods#4321)
This commit is based in the following commits:

- “add specs for :application_extension_api_only in Analyzer” (89c2798)
- “[Installer] Add spec for setting APPLICATION_EXTENSION_API_ONLY when the user target has it set” (7e7c3d7, Pull Request CocoaPods#4321)
CHANGELOG.md Outdated Show resolved Hide resolved
lib/cocoapods/target.rb Outdated Show resolved Hide resolved
@@ -98,6 +98,7 @@ def merge_embedded_pod_targets(embedded_pod_targets_for_build_configuration)
target_definition, client_root, user_project, user_target_uuids, merged).tap do |aggregate_target|
aggregate_target.search_paths_aggregate_targets.concat(search_paths_aggregate_targets).freeze
aggregate_target.mark_application_extension_api_only if application_extension_api_only
aggregate_target.mark_build_library_for_distribution if build_library_for_distribution
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a test covering this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there isn't. I copied this from #9142 and there isn't a test for mark_application_extension_api_only neither (or I haven't found it), so I don't know how to test this.

@dnkoutso dnkoutso added this to the 1.10.0 milestone Apr 6, 2020
@dnkoutso
Copy link
Contributor

dnkoutso commented Apr 6, 2020

cc @amorde

@juanjonol
Copy link
Contributor Author

I've fixed all the CI errors except this one: spec/unit/installer/analyzer_spec.rb:5:5: C: Block has too many lines. [2034/1968] describe 'Analysis' do .... I don't know what's the best way to solve it...

@dnkoutso
Copy link
Contributor

dnkoutso commented Apr 6, 2020

Also run be rubocop -a

Copy link
Member

@amorde amorde left a comment

Choose a reason for hiding this comment

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

LGTM besides other comments made already

Thank you for working on this!

@amorde
Copy link
Member

amorde commented Apr 7, 2020

You can disable the Rubocop for that specific location by adding # rubocop:disable Metrics/BlockLength at the end of the describe 'Analysis' line. I think we're ok with this being long since these tests are a bit verbose

@dnkoutso dnkoutso merged commit 2050bfa into CocoaPods:master Apr 8, 2020
@vg-identance
Copy link

When this will be released?

@amorde
Copy link
Member

amorde commented Jun 10, 2020

We have 1-2 more things to wrap up before 1.10. Don't have any timelines to give you unfortunately

@vg-identance
Copy link

@amorde Thanks!

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.

4 participants