Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[CocoaPods/CocoaPods#904] Separate header paths for different targets in .xcconfig files. #1590

Closed
wants to merge 2 commits into from

4 participants

Michael Melanson Coveralls Fabio Pelosin sstorkel
Michael Melanson

This patch changes how header paths are created in .xcconfig files such that if the podfile has two targets for different platforms (e.g. iOS and OS X), then it won't end up causing the iOS target to reference headers from pods in the OS X target.

We encountered this problem when using Chameleon, because it replaces UIKit headers -- but the iOS target ended up linking against the (OS X only!) Chameleon headers.

This is my first contribution to CocoaPods and I haven't fully verified that it is correct, but it does work for our purposes. Please double-check I haven't broken anything before merging.

Coveralls

Coverage Status

Changes Unknown when pulling 771b31c on toushay:issue/904 into ** on CocoaPods:master**.

Fabio Pelosin

This patch looks ace. Unfortunately there is an important issue that we need to resolve before merging it, provide support for test targets in the Podfile DSL. You can read more about this issue here.

Also a patch like this before being merged would require adequate tests and an entry in the Changelog.

Michael Melanson

Sorry about the delay there. Had a deadline to deal with on my end and just got a chance to deal with this today.

Those commits fix the test failures from before and add a note in the CHANGELOG. The Travis build seems to be failing for an unrelated issue (can't find the correct Ruby version?).

@irrationalfab, I'm not sure what you mean by changes to the Podfile DSL. Could you explain what changes you mean?

Fabio Pelosin

I can confirm that the Travis issue is in fact unrelated.

Regarding the Podfile DSL the issue is:

  1. Users might want to access headers of a Pod without linking against it (test bundles where the target already includes the Pod).
  2. They have been instructed to use the :exclude option to do so (not sure why now that I think about it) which leverages the fact that the all the headers are visible.
  3. They should still be supported so an upgrade path should be provided for them with a DSL option.
Michael Melanson

@irrationalfab:

How would you recommend we fix this issue? I'm afraid I've never used that capability, so I'm not sure what the requirements are. What sort of an upgrade path would you recommend?

Perhaps someone else would be better equipped to do that work?

sstorkel

@michaelmelanson: It occurs to me that a more generic solution to this problem, and perhaps one that meshes better with @irrationalfab's ideas of where Podfile syntax is headed, would be to allow target to specify whether headers for included pods are published globally.

Toward that end, adding a :publish_headers option to target might be the way to go. If set to true, the default, headers specified in the target's pods would be made available globally just like they currently are. If set to false, the headers would only be available to the current target (and sub-targets?). If we use this idea with the example from issue #1619, we might end up with a Podfile that looks like this:

workspace 'Foo'
pod 'AFNetworking'

target :Foo_Mac, :publish_headers => false do
    xcodeproj 'Mac/Foo_Mac'
    platform :osx
    pod 'HockeySDK-Mac'
end

target :Foo_iOS, :publish_headers => false do
    xcodeproj 'Foo_iOS'
    platform :ios, '7.0'
    pod 'Parse-iOS-SDK'
end

And the contents of Pods-Foo_iOS.xcconfig would be:

HEADER_SEARCH_PATHS = "${PODS_ROOT}/Headers/AFNetworking" "${PODS_ROOT}/Headers/Parse-iOS-SDK" 

While Pods-Foo_Mac.xcconfig would contain:

HEADER_SEARCH_PATHS = "${PODS_ROOT}/Headers/AFNetworking" "${PODS_ROOT}/Headers/HockeySDK-Mac" 

Unfortunately, I don't grok the long-term plan well enough to know whether this would help or hinder long-term progress. Maybe @irrationalfab will weigh-in with a more authoritative suggestion...

Michael Melanson

I think we should be careful not to let this issue get out of scope. I worry we're straying from the purpose of this bug fix: to prevent the header search paths from being cross-linked in multi-platform workspaces.

sstorkel

We also need to be cognizant of the bigger picture: we don't want to implement something that's going to lead to future compatibility or migration issues as CocoaPods continues to evolve...

Michael Melanson

Of course not, but there should be no migration required. And as for compatibility, I would be highly surprised if anyone depended on the old behaviour in their Podfile even accidentally.

Do you have any examples of cases where these changes could break a person's setup?

sstorkel

I think you misunderstood: I'm not suggesting your change will be a problem today, but rather might become one when @irrationalfab starts making the Podfile syntax changes he's described elsewhere (ex: issue #840).

Fabio Pelosin

And as for compatibility, I would be highly surprised if anyone depended on the old behavior in their Podfile even accidentally.

@michaelmelanson

That was my point and the reason why I didn't merge such an important patch straight away. If you are using a test bundle to test a target integrated with CocoaPods you are relying on this feature to access the headers of the Pods of the tested target.

Fo this reason I think that we should come up with a DSL solution (not in this issue) before merging it.

@sstorkel

[...] would be to allow target to specify whether headers for included pods are published globally.

I can't think of a scenario where you might want to expose your headers to all targets. I think that only certain targets might be interested in the headers of other specific ones.

sstorkel

I can't think of a scenario where you might want to expose your headers to all targets. I think that only certain targets might be interested in the headers of other specific ones.

@irrationalfab Perhaps, but exposing headers to all targets is currently the default behavior for CocoaPods and that seems to work for the vast majority of people and projects!

By attaching a slightly more generic version of @michaelmelanson's fix to the DSL I think we'd fix the current problem without preventing progress on other fronts (ex: #840). Formalizing the existing behavior should make it easier to deprecate or replace once other more powerful mechanisms are available.

Fabio Pelosin

@irrationalfab Perhaps, but exposing headers to all targets is currently the default behavior for CocoaPods and that seems to work for the vast majority of people and projects!

It works most of the times but it can lead to subtle issues.

By attaching a slightly more generic version of @michaelmelanson's fix to the DSL I think we'd fix the current problem without preventing progress on other fronts (ex: #840).

The changes to the DSL should be minimized as much as possible so this proposal would not help #840 which is likely to introduce a different option and would create confusion to the users.

Formalizing the existing behavior should make it easier to deprecate or replace once other more powerful mechanisms are available.

That is a good point but this proposal requires to make a change to the Podfile for the old behavior. At that point is worth to ask the affected users to perform the change only once and use the syntax of #840.

To be clear #840 is blocked only by semantics and not by technical reasons (as it would be reasonably straightforward to implement).

sstorkel

@irrationalfab

It works most of the times but it can lead to subtle issues.

Which we've already got and need to deal with, as evidenced by the fact that this issue exists...

The changes to the DSL should be minimized as much as possible so this proposal would not help #840 which is likely to introduce a different option and would create confusion to the users.

Since a design for fixing #840 hasn't been finalized, how can you be sure that it won't incorporate this mechanism or at least be compatible with it?

That is a good point but this proposal requires to make a change to the Podfile for the old behavior.

This is incorrect. My proposal ensures that existing Podfiles will continue to work exactly as they have always worked. Those Podfiles won't include the new publish_headers attribute as part of the target definition, so they will continue to use the current/default behavior of making all headers available to all targets.

The only way to get the new behavior is to modify the Podfile to use :publish_headers => false. Again: I think it's possible that my proposal could be folded into #840 or at the very least continue to work alongside it. If not, the change will only affect the small number of users who need the new behavior in order to get their Podfiles to work.

Fabio Pelosin

@sstorkel I agree with most of your points.

This is incorrect. My proposal ensures that existing Podfiles will continue to work exactly as they have always worked. Those Podfiles won't include the new publish_headers attribute as part of the target definition, so they will continue to use the current/default behavior of making all headers available to all targets.

Sorry I missed that the default was true.

Since a design for fixing #840 hasn't been finalized, how can you be sure that it won't incorporate this mechanism or at least be compatible with it?

I have a rough idea of how the syntax should look like, so although I'm not sure I have good expect ions that there is no need to publish the headers globally. About compatibility, one of the most important principles of the CocoaPods DSLs is to stay as concise as possible. Options create confusions among users (I know something about it thanks to this issue list) and impose complexity in the implementation affecting maintainability and easiness of implementing new features (because is state that needs to be carried around in the installation process).

Michael Melanson michaelmelanson deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
5 CHANGELOG.md
View
@@ -19,6 +19,11 @@ To install or update CocoaPods see this [guide](http://docs.cocoapods.org/guides
###### Bug Fixes
+* Fixed a bug where header search paths for targets for different platforms
+ would get cross-linked.
+ [Michael Melanson](https://github.com/michaelmelanson)
+ [#904](https://github.com/CocoaPods/CocoaPods/issues/904)
+
* Fixed a bug which resulted in `pod lib lint` not being able to find the
headers.
[Fabio Pelosin](https://github.com/irrationalfab)
6 lib/cocoapods/generator/xcconfig/aggregate_xcconfig.rb
View
@@ -44,10 +44,12 @@ def save_as(path)
# @return [Xcodeproj::Config]
#
def generate
- header_search_path_flags = target.sandbox.public_headers.search_paths.map {|path| '-isystem'+path}
+ search_paths = target.sandbox.public_headers.search_paths(target.platform).uniq
+ header_search_path_flags = search_paths.map {|path| '-isystem'+path}
+
@xcconfig = Xcodeproj::Config.new({
'OTHER_LDFLAGS' => XCConfigHelper.default_ld_flags(target),
- 'HEADER_SEARCH_PATHS' => XCConfigHelper.quote(target.sandbox.public_headers.search_paths),
+ 'HEADER_SEARCH_PATHS' => XCConfigHelper.quote(search_paths),
'PODS_ROOT' => target.relative_pods_root,
'GCC_PREPROCESSOR_DEFINITIONS' => '$(inherited) COCOAPODS=1',
'OTHER_CFLAGS' => '$(inherited) ' + XCConfigHelper.quote(header_search_path_flags)
5 lib/cocoapods/generator/xcconfig/private_pod_xcconfig.rb
View
@@ -47,7 +47,10 @@ def save_as(path)
# @return [Xcodeproj::Config]
#
def generate
- search_paths = target.build_headers.search_paths.concat(target.sandbox.public_headers.search_paths)
+ target_build_headers = target.build_headers.search_paths(target.platform)
+ sandbox_public_headers = target.sandbox.public_headers.search_paths(target.platform)
+
+ search_paths = target_build_headers.concat(sandbox_public_headers).uniq
config = {
'OTHER_LDFLAGS' => XCConfigHelper.default_ld_flags(target),
'PODS_ROOT' => '${SRCROOT}',
8 lib/cocoapods/installer/file_references_installer.rb
View
@@ -116,15 +116,15 @@ def link_headers
libraries.each do |library|
library.file_accessors.each do |file_accessor|
headers_sandbox = Pathname.new(file_accessor.spec.root.name)
- library.build_headers.add_search_path(headers_sandbox)
- sandbox.public_headers.add_search_path(headers_sandbox)
+ library.build_headers.add_search_path(headers_sandbox, library.platform)
+ sandbox.public_headers.add_search_path(headers_sandbox, library.platform)
header_mappings(headers_sandbox, file_accessor, file_accessor.headers).each do |namespaced_path, files|
- library.build_headers.add_files(namespaced_path, files)
+ library.build_headers.add_files(namespaced_path, files, library.platform)
end
header_mappings(headers_sandbox, file_accessor, file_accessor.public_headers).each do |namespaced_path, files|
- sandbox.public_headers.add_files(namespaced_path, files)
+ sandbox.public_headers.add_files(namespaced_path, files, library.platform)
end
end
end
2  lib/cocoapods/installer/target_installer/pod_target_installer.rb
View
@@ -93,7 +93,7 @@ def create_xcconfig_file
path = library.xcconfig_private_path
private_gen = Generator::XCConfig::PrivatePodXCConfig.new(library, public_gen.xcconfig)
- UI.message "- Generating private xcconfig file at #{UI.path(path)}" do
+ UI.message "- Generating private xcconfig file for #{library.label} at #{UI.path(path)}" do
private_gen.save_as(path)
xcconfig_file_ref = add_file_to_support_group(path)
19 lib/cocoapods/sandbox/headers_store.rb
View
@@ -25,15 +25,17 @@ def root
def initialize(sandbox, relative_path)
@sandbox = sandbox
@relative_path = relative_path
- @search_paths = [relative_path]
+ @search_paths = []
end
# @return [Array<String>] All the search paths of the header directory in
# xcconfig format. The paths are specified relative to the pods
# root with the `${PODS_ROOT}` variable.
#
- def search_paths
- @search_paths.uniq.map { |path| "${PODS_ROOT}/#{path}" }
+ def search_paths(platform)
+ platform_path_entries = @search_paths.select {|entry| entry[:platform] == platform }
+
+ ["${PODS_ROOT}/#{@relative_path}"] + platform_path_entries.uniq.map { |entry| "${PODS_ROOT}/#{entry[:path]}" }
end
# Removes the directory as it is regenerated from scratch during each
@@ -64,8 +66,8 @@ def implode!
#
# @return [Pathname]
#
- def add_files(namespace, relative_header_paths)
- add_search_path(namespace)
+ def add_files(namespace, relative_header_paths, platform)
+ add_search_path(namespace, platform)
namespaced_path = root + namespace
namespaced_path.mkpath unless File.exist?(namespaced_path)
@@ -84,10 +86,13 @@ def add_files(namespace, relative_header_paths)
# @param [Pathname] path
# the path tho add.
#
+ # @param [String] platform
+ # the platform the search path applies to
+ #
# @return [void]
#
- def add_search_path(path)
- @search_paths << Pathname.new(@relative_path) + path
+ def add_search_path(path, platform)
+ @search_paths << {:platform => platform, :path => (Pathname.new(@relative_path) + path) }
end
#-----------------------------------------------------------------------#
4 spec/unit/generator/xcconfig/aggregate_xcconfig_spec.rb
View
@@ -56,12 +56,12 @@ module XCConfig
end
it 'adds the sandbox public headers search paths to the xcconfig, with quotes, as header search paths' do
- expected = "\"#{config.sandbox.public_headers.search_paths.join('" "')}\""
+ expected = "\"#{config.sandbox.public_headers.search_paths(:ios).join('" "')}\""
@xcconfig.to_hash['HEADER_SEARCH_PATHS'].should == expected
end
it 'adds the sandbox public headers search paths to the xcconfig, with quotes, as system headers' do
- expected = "$(inherited) \"-isystem#{config.sandbox.public_headers.search_paths.join('" -isystem"')}\""
+ expected = "$(inherited) \"-isystem#{config.sandbox.public_headers.search_paths(:ios).join('" -isystem"')}\""
@xcconfig.to_hash['OTHER_CFLAGS'].should == expected
end
4 spec/unit/generator/xcconfig/private_pod_xcconfig_spec.rb
View
@@ -44,8 +44,8 @@ module XCConfig
end
it 'adds the library build headers and public headers search paths to the xcconfig, with quotes' do
- private_headers = "\"#{@pod_target.build_headers.search_paths.join('" "')}\""
- public_headers = "\"#{config.sandbox.public_headers.search_paths.join('" "')}\""
+ private_headers = "\"#{@pod_target.build_headers.search_paths(:ios).join('" "')}\""
+ public_headers = "\"#{config.sandbox.public_headers.search_paths(:ios).join('" "')}\""
@xcconfig.to_hash['HEADER_SEARCH_PATHS'].should.include private_headers
@xcconfig.to_hash['HEADER_SEARCH_PATHS'].should.include public_headers
end
1  spec/unit/installer/file_references_installer_spec.rb
View
@@ -6,6 +6,7 @@ module Pod
before do
@file_accessor = fixture_file_accessor('banana-lib/BananaLib.podspec')
@pod_target = PodTarget.new([], nil, config.sandbox)
+ @pod_target.stubs(:platform).returns(Platform.new(:ios, '6.0'))
@pod_target.file_accessors = [@file_accessor]
@project = Project.new(config.sandbox.project_path)
@project.add_pod_group('BananaLib', fixture('banana-lib'))
8 spec/unit/sandbox/headers_store_spec.rb
View
@@ -22,7 +22,7 @@ module Pod
relative_header_paths.each do |path|
File.open(@sandbox.root + path, "w") { |file| file.write('hello') }
end
- symlink_paths = @header_dir.add_files(namespace_path, relative_header_paths)
+ symlink_paths = @header_dir.add_files(namespace_path, relative_header_paths, :fake_platform)
symlink_paths.each do |path|
path.should.be.symlink
File.read(path).should == "hello"
@@ -39,12 +39,12 @@ module Pod
relative_header_paths.each do |path|
File.open(@sandbox.root + path, "w") { |file| file.write('hello') }
end
- @header_dir.add_files(namespace_path, relative_header_paths)
- @header_dir.search_paths.should.include("${PODS_ROOT}/Headers/ExampleLib")
+ @header_dir.add_files(namespace_path, relative_header_paths, :fake_platform)
+ @header_dir.search_paths(:fake_platform).should.include("${PODS_ROOT}/Headers/ExampleLib")
end
it 'always adds the Headers root to the header search paths' do
- @header_dir.search_paths.should.include("${PODS_ROOT}/Headers")
+ @header_dir.search_paths(:fake_platform).should.include("${PODS_ROOT}/Headers")
end
end
end
Something went wrong with that request. Please try again.