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

Minimum deployment target #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Minimum deployment target #14

wants to merge 2 commits into from
+17 −10

Conversation

Vkt0r
Copy link
Contributor

@Vkt0r Vkt0r commented Nov 29, 2019

Hey there 👋. Thanks for the creation of this plugin it's really promising!

This is PR is focused on fixing an issue with the minimum deployment target in the *.podspec for the generated groups. In the plugin, this was hardcoded to s.ios.deployment_target = '8.0' causing that the inclusion of libraries with a higher deployment target would generate a compilation error. You can see the example library added in the PodMergeExample (Kingfisher).

To solve this I used the platform :ios, 'x.y' declared in the Podfile. This should reflect the minimum deployment target for the Pods project. It's worth to mention that the Podfile may have been parsed using the CocoaPods Core gem already available avoiding the manual parsing and regex, with something like this:

podfile = Pod::Podfile.from_file("Podfile")
sources = podfile.sources
    
podfile.root_target_definitions.each do |target|
    platforms.append(target.platform.to_s)
end

I leave as it was to avoid possibles issues in other parts of the plugin.

So this PR can be resumed in the following:

  • Update the ios.deployment_target from the Podfile.
  • Fix a misspelling in the README.

biocross
biocross suggested changes Dec 5, 2019
View reviewed changes
lib/cocoapods-pod-merge/Main.rb
@@ -490,6 +490,7 @@ def create_podspec(merged_framework_name, pods_to_merge, podspec_info, mixed_lan
resource_bundles = podspec_info.resource_bundles
vendored_libraries = podspec_info.vendored_libraries
swift_versions = podspec_info.swift_versions
ios_deployment_target = podfile_info.platforms.find { |platform| platform.include? "ios"}.split(',')[1]
Copy link

Choose a reason for hiding this comment

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

What happens if the Podfile does not define a platform? Will this be able to get a default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a very valid question! By default Cocoapods provide a default in case that one is not specified:

CocoaPods provides a default deployment target if one is not specified. The current default values are 4.3 for iOS, 10.6 for OS X, 9.0 for tvOS and 2.0 for watchOS.

Nevertheless, in this specific case, we should provide a default one in the case that is not specified in the Podfile. Let's do that of course!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhmm ran some experiments and one thing emerged. If I delete the MergedPods folder and run bundle exec pod install Cocoapods would raise an exception like the following in case the platform is not set in the Podfile:

It is necessary to specify the platform in the Podfile if not integrating.

If the project was integrated it already I think we're good if the platform is not specified.

The other case is when once it was integrated we comment it out the platform in the Podfile and run bundle exec pod install and Cocoapods would try to set a default platform target version for the Pods.xcodeproj like this:

[!] Automatically assigning platform `iOS` with version `11.0` on target `PodMergeExample` because no platform was specified. Please specify a platform for this target in your Podfile. See `https://guides.cocoapods.org/syntax/podfile.html#platform`.

Let me know your thoughts about it.

lib/cocoapods-pod-merge/Main.rb
@@ -334,7 +334,7 @@ def merge(merged_framework_name, group_contents, podfile_info)

# Create the local podspec
Pod::UI.puts "\tCreating Podspec for the merged framework".magenta
create_podspec(merged_framework_name, pods_to_merge, PodspecInfo.new(frameworks.uniq, prefix_header_contents.uniq, private_header_files.uniq, resources.uniq, script_phases.uniq, compiler_flags.uniq, libraries.uniq, prepare_command.uniq, resource_bundles, vendored_libraries.uniq, swift_version), mixed_language_group)
create_podspec(merged_framework_name, pods_to_merge, PodspecInfo.new(frameworks.uniq, prefix_header_contents.uniq, private_header_files.uniq, resources.uniq, script_phases.uniq, compiler_flags.uniq, libraries.uniq, prepare_command.uniq, resource_bundles, vendored_libraries.uniq, swift_version), mixed_language_group, podfile_info)
Copy link

Choose a reason for hiding this comment

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

Where is thispodfile_info being created? 🤔

Copy link
Contributor Author

@Vkt0r Vkt0r Dec 8, 2019 •
edited

Choose a reason for hiding this comment

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

Oh, this was created in this line

cocoapods-pod-merge/lib/cocoapods-pod-merge/Main.rb

Line 49 in c9830f8

podfile_info = read_podfile
I only passed it to be able to extract the deployment target.

PodMergeExample/Podfile.lock
@@ -13,7 +13,7 @@ DEPENDENCIES:
- UI (from `MergedPods/UI`)

SPEC REPOS:
https://cdn.cocoapods.org/:
trunk:
Copy link

Choose a reason for hiding this comment

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

Do we need this change? I think we can continue to use the new Cocoapods CDN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhmm weird, I've used the latest Cocoapods version so it should be using the CDN now. I'll check that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're good with trunk. Take a look at this closed issue CocoaPods/CocoaPods#9190.

@biocross
Copy link

biocross commented Dec 5, 2019

Thank you for your PR @Vkt0r!

I think reading the Podfile for the deployment target is a very nice! And thank you for the idea of reading the Podfile using Cocoapods Core, I should migrate some more code inside the plugin to use this and get rid my crazy regex 😂!

I've just left a few comments to better understand this PR :)

@Vkt0r
Fix a misspelling in the README
5092d1f
@Vkt0r
Update the ios.deployment_target from the Podfile
2ced8a4 
The `ios.deployment_target` was set to a default value causing that pods with a minimum deployment target greater than iOS 8 were throwing errors.
@Vkt0r
Copy link
Contributor Author

Vkt0r commented Nov 3, 2020

@biocross I rebased this PR for the latest changes. Can you please continue? Thanks

@M4kxjci M4kxjci linked an issue May 7, 2022 that may be closed by this pull request
int #29
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@biocross biocross biocross requested changes

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

Successfully merging this pull request may close these issues.

int
2 participants
@Vkt0r @biocross