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

[RFC] Podspec Info.plist DSL #8753

Closed
amorde opened this issue May 1, 2019 · 7 comments

Comments

@amorde
Copy link
Member

commented May 1, 2019

Podspec Info.plist DSL

Introduction

This change would provide a new DSL for modifying the contents of the Info.plist that is generated by CocoaPods, depending on the integration type.

Motivation

The Info.plist file is the customization point for things such as the bundle identifier, development language, version number, and build number, and also allows for setting custom key value pairs that are readable at runtime. The bundle identifier is especially important as it can be used to initialize Bundle instances and is also used by Interface Builder in things such as Storyboard References and IBDesignable views.

With the introduction of app specs in CocoaPods 1.7.0, the ability to modify generated Info.plist contents becomes more important. The proposed solution includes support for modifying the Info.plist of the generated app host.

Proposed solution

Introduce a new attribute to the Specification DSL, info_plist, that allows specifying a Hash that will be merged with the values that CocoaPods generates. This allows Pod authors to override any default values or add custom key-value pairs to what is generated by default.

Example:

s.info_plist = {
  'CFBundleIdentifier' => 'com.myorg.MyLib',
  'CFBundleDevelopmentRegion' => 'ko_KR'
}

Test and App Specs can provide values which will apply to unit test bundles and app hosts, respectively.

s.info_plist = {
  'CFBundleIdentifier' => 'com.myorg.MyLib'
}

s.test_spec 'Test' do |ss|
  ss.info_plist = {
    'CFBundleIdentifier' => 'com.myorg.MyLibTests'
  }
end

s.app_spec 'HostApp' do |app|
  app.info_plist = {
    'CFBundleIdentifier' => 'com.myorg.MyApplication'
  }
end

Detailed design

The Specification DSL will be updated to include a new attribute.

  # @!method info_plist=(info_plist)
  #
  #   Key-Value pairs to add to the generated `Info.plist`.
  #
  #   The values will be merged with the default values that
  #   CocoaPods generates, overridding any duplicates.
  #
  #   For library specs, the values will be merged into the generated Info.plist
  #   for libraries that are integrated using frameworks. It will have no effect
  #   for static libraries.
  #
  #   Subspecs (other than app and test specs) are not supported.
  #
  #   For app specs, the values will be merged into the application host's `Info.plist`.
  #
  #   For test specs, the values will be merged into the test bundle's `Info.plist`.
  #
  #   @example
  #
  #     spec.info_plist = {
  #       'CFBundleIdentifier' => 'com.myorg.MyLib',
  #       'MY_VAR' => 'SOME_VALUE'
  #     }
  #
  #   @param  [Hash] info_plist
  #           The Info.plist values for the Pod.
  #
  attribute :info_plist,
            :container => Hash,
            :inherited => false

The InfoPlistFile class already supports passing a Hash of additional values to merge into the default values generated by CocoaPods.

It is important to note that this will only affect Pods integrated as dynamic frameworks, test specs, and app specs, since other integration types do not have an Info.plist bundled into the resulting application. For example, static libraries do not require Info.plist files.

Subspecs will not be supported initially. This is because multiple subspecs can be included in 1 host, and there is not a clear way to include the values of both subspecs into one hash. Support can be added easily if there is an agreement on how this would work in practice.

Backwards compatibility

Podspecs that utilize this feature will require the version of CocoaPods
that introduces it. It is recommended for Pod authors to use the cocoapods_version attribute
to specify which version the Podspec requires.

This feature also requires dynamic frameworks which were introduced in iOS 8.

Alternatives considered

Nothing, leaving things as is

Currently it is possible to specify an Info.plist file using a combination of pod_target_xcconfig and either preserve_paths or resources, with something such as the following:

s.resources = 'Resources/Info.plist'
s.pod_target_xcconfig = {
    'INFOPLIST_FILE' => '$(POD_TARGET_SRCROOT)/Resources/Info.plist'
}

This is subpar for a few reasons:

  • It relies on CocoaPods checking for this and not overriding the value of INFOPLIST_FILE (see #7530)
  • It is not inherently clear that this is possible or supported
  • Pod authors utilizing this must maintain a list of required and/or default key-value pairs to include in their file. This is likely the biggest drawback.

Support specifying a file instead of a Hash

The attribute could be changed to accept a String path to a file which contains the entire contents of the Info.plist. This would be simpler to implement for CocoaPods as we can use existing file pattern logic similar to other attributes such as resources.

However, the default values generated by CocoaPods are useful and it would be cumbersome for Pod authors to research what values need to be specified for things to work correctly. This solution would shift burden from CocoaPods to Pod authors, and is likely to cause bugs when done incorrectly. This also differs from the existing override paradigm used in pod_target_xcconfig and user_target_xcconfig.

Support both specifying a file or the overridding behavior using Hash

It is possible to support both use cases, and allow for specifying either a file or a Hash of overrides. This is the most flexible, but is much more complex. I had originally planned for this to be the proposed solution, but decided it was not worth the extra complexity.

Given that pod_target_xcconfig only accepts overrides, it seems natural for the info_plist attribute to behave the same way. It is also much easier to add support for specifying a file later if it becomes necessary, but much more difficult to remove support for it if it's decided it is not worth the extra complexity.

Add DSL atrributes for the specific keys that can be overridden

Another option is to make different attributes for the most commonly used keys in the Info.plist, such as the bundle identifier. This would support the most common use cases without the additional complexity of supporting any possible key-value pair in the final Info.plist.

s.bundle_identifier = 'com.myorg.MyLib`

This is suboptimal in that it requires a new attribute for each key, but it does provide an easy entry point for providing validation on each attribute during lint (ie. pod spec lint could produce a warning if bundle_identifier was a number instead of a string). This is also a much smaller initial change.

In the scenario that the info_plist DSL was added at a later point in time, there would be ambiguity as to how the attributes combine.

s.bundle_identifier = 'com.myorg.MyLibA'
s.info_plist = {
    'CFBundleIdentifier' => 'com.myorg.MyLibB'
}
# Which one ends up in the generated file?
@paulb777

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Are generated app's for the test_spec attribute requires_app_host included? It would provide a more scalable solution than PR's like #8747.

@dnkoutso

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

It is important to note that this will only affect Pods integrated as dynamic frameworks, test specs, and app specs, since other integration types do not have an Info.plist bundled into the resulting application. For example, static libraries do not require Info.plist files.

I would add that to the documentation above the DSL declaration. it seems it only mentions dynamic frameworks for now.

@amorde

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

@dnkoutso I'll update the docs, it's mentioned below but the first sentence could be improved a bit

@paulb777 yes I think the ideal scenario would be to use the functionality introduced in CocoaPods/Core#520 to customize the app host, which might look something like this:

s.test_spec 'Tests' do |test_spec|
    test_spec.app_host_name = 'TestAppHost'
end

s.app_spec 'TestAppHost' do |app_spec|
    app_spec.info_plist = {
        'NSAppTransportSecurity' => {
	    'NSAllowsArbitraryLoads' => true,
        }
    }
end

@amorde amorde pinned this issue May 9, 2019

@sebastianv1 sebastianv1 unpinned this issue Jun 3, 2019

@sebastianv1 sebastianv1 pinned this issue Jun 3, 2019

@dnkoutso dnkoutso added this to the 1.8.0 milestone Jun 14, 2019

@dnkoutso

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

Marked for 1.8.0.

amorde added a commit that referenced this issue Jun 15, 2019

@amorde amorde referenced this issue Jun 15, 2019

Merged

Add support for new Info.plist DSL #8907

2 of 2 tasks complete
@Coeur

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

The Hash solution (as described in "Proposed solution") sounds the best to me. The support for specifying a file can indeed be added later. And we don't want to clutter with specific keys for each attribute, as it would require to update the DSL often, only to add ambiguity to something that can be done from the Hash directly.

@amorde

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2019

I've run into an issue during implementation that may prevent subspecs from being supported.

Suppose a Podfile such as the following:

target 'MyApp' do
  pod 'SomePod/A'
  pod 'SomePod/B'
end

A Pod variant with the label SomePod-A-B will be created, and its Info.plist will be SomePod-A-B-Info.plist. But if both subspecs specified info_plist hashes, they will both need to be merged. I don't think any merging would make sense here, and I'm considering dropping support for subspecs but leaving support for app and test specs since those create separate targets.

Another option would be to merge them in the other they are specified, but historically the ordering of pod declarations hasn't mattered and I don't think we should introduce something that is dependent on the order of calls to pod.

@Coeur

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2019

Then, for now, let's forbid plist Hash in subspecs (with an error triggered during pod lib lint)

amorde added a commit that referenced this issue Jul 2, 2019

amorde added a commit that referenced this issue Jul 7, 2019

amorde added a commit that referenced this issue Jul 7, 2019

amorde added a commit that referenced this issue Jul 8, 2019

@amorde amorde closed this in #8907 Jul 8, 2019

@dnkoutso dnkoutso unpinned this issue Jul 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.