Add support for modular header search paths, include "legacy" support #7412

dnkoutso · 2018-02-06T18:32:04Z

Assuming PodA, PodB and PodC in a project.
Assuming PodA depends only on PodB but not on PodC

Header search paths for PodA:

before (<= 1.4, a.k.a today)

${PODS_ROOT}/Headers/Private
${PODS_ROOT}/Headers/Public
${PODS_ROOT}/Headers/Private/PodA
${PODS_ROOT}/Headers/Public/PodA
${PODS_ROOT}/Headers/Public/PodB
${PODS_ROOT}/Headers/Public/PodC

Notice that root Public folder but also that PodC is included in the pod's header search paths despite the fact that PodA does not depend on PodC.

after (> 1.5)

${PODS_ROOT}/Headers/Private/PodA
${PODS_ROOT}/Headers/Public/PodA
${PODS_ROOT}/Headers/Public/PodA/PodA
${PODS_ROOT}/Headers/Public/PodB
${PODS_ROOT}/Headers/Public/PodB/PodB

Notice that PodC is correctly not included (due to #7116) but also that the root folder Public is no longer present which would have otherwise allowed consumers to include headers of PodC without directly depending on it in their podspec.

Instead, the root Public folder is "expanded" so users can continue to use <> and "" header imports. Functionally, this is still the same and it shouldn't break anyone upgrading to 1.5.0 but it also makes it more clear by removing the root Public folder.

When using/enabling modular headers there is no "before" state because we never had modular header support in CocoaPods, but PodA header search paths look like this (using static libraries):

${PODS_ROOT}/Headers/Private/PodA
${PODS_ROOT}/Headers/Public/PodA
${PODS_ROOT}/Headers/Public/PodB

As for the file system all public headers are now nested an additional folder further so it looks like this:

Public header symlinks on the file system (notice the extra nested folder):

Pods/Headers/Public/PodA/PodA/PodA.h
Pods/Headers/Public/PodB/PodB/PodB.h
Pods/Headers/Public/PodC/PodC/PodC.h

Private header symlinks on the file system (notice no extra folder as all imports can work with ""):

Pods/Headers/Private/PodA/PodA_Private.h
Pods/Headers/Private/PodB/PodB_Private.h
Pods/Headers/Private/PodC/PodC_Private.h

This means that PodA can now use <> imports of PodB but not use "" imports for PodB private headers (unless maybe USE_HEADERMAPS is set to YES?).

The above only applies HEADER_SEARCH_PATHS which get populated when static libraries are used. When frameworks are used then FRAMEWORK_SEARCH_PATHS are used which continue to work as they are today.

Note that CocoaPods does not also change or interfere with the USE_HEADERMAPS flag, which can cause plenty of dark magic to find headers of targets that are otherwise not part of the header search paths.

closes #7011

paulb777 · 2018-02-06T18:49:44Z

This looks great! How will a podspec choose to "legacy" or "new" functionality?

dnkoutso · 2018-02-06T19:33:06Z

@paulb777 see this comment https://github.com/CocoaPods/CocoaPods/pull/6966/files#r165800460

paulb777 · 2018-02-06T19:43:12Z

Isn't that comment only about Swift static libraries? My understanding is that this PR making the imports more restrictive will break Pods that were implicitly depending on the incorrectly exposed headers - like using quotes instead of brackets or importing a private header.

dnkoutso · 2018-02-06T19:49:59Z

lib/cocoapods/installer/xcode/pods_project_generator/file_references_installer.rb

          # @param  [Array<Pathname>] headers
          #         The absolute paths of the headers which need to be mapped.
          #
          # @return [Hash{Pathname => Array<Pathname>}] A hash containing the
          #         headers folders as the keys and the absolute paths of the
          #         header files as the values.
          #
-          def header_mappings(headers_sandbox, file_accessor, headers)
+          def header_mappings(headers_sandbox, file_accessor, headers, visibility_scope = :private)


I am thinking on removing visibilty_scope from this method and instead pass in the initializer. Some header stores are explicitly created as private stores but yet this method talks about a public header scope.

actually this comment is on the wrong file, I meant header_store.rb and yes I changed it there. This is fine to keep here.

dnkoutso · 2018-02-06T19:54:27Z

@paulb777 it is mostly tied to Swift static libs but not 100%, the defines_module? method has three rules:

all pods with Swift must use a module (static lib or not when Swift Static Library support #6966 lands)
If requires frameworks is on then true (in which case FRAMEWORKS_SEARCH_PATHS are used instead)
If the pod author had specified DEFINES_MODULE => 'YES' in their config.

I think "turning this on by default" might break the world. Any additional thoughts or ideas around this?

We could potentially offer a Podfile flag to enable modules everywhere

paulb777 · 2018-02-06T20:01:02Z

I'm worried about (2). I suspect we have pure Objective C podspecs that are sloppy with #import's that would break. I would be fine to fix them, but can't speak for the world.

segiddins · 2018-02-06T21:45:19Z

@paulb777 pods built as frameworks already dont get the header search paths, since they find headers via the FRAMEWORK_SEARCH_PATHS

segiddins · 2018-02-06T18:39:58Z

lib/cocoapods/installer/xcode/pods_project_generator/file_references_installer.rb

@@ -147,11 +147,15 @@ def link_headers
                    pod_target.build_headers.add_search_path(headers_sandbox, pod_target.platform)
                    sandbox.public_headers.add_search_path(headers_sandbox, pod_target.platform)

+                    # Private headers will always end up in <Pods/Headers/Private/PodA/*.h


why the <?

segiddins · 2018-02-06T18:40:06Z

lib/cocoapods/installer/xcode/pods_project_generator/file_references_installer.rb

                    header_mappings(headers_sandbox, file_accessor, file_accessor.headers).each do |namespaced_path, files|
                      pod_target.build_headers.add_files(namespaced_path, files.reject { |f| f.to_path =~ framework_exp })
                    end

-                    header_mappings(headers_sandbox, file_accessor, file_accessor.public_headers).each do |namespaced_path, files|
+                    # Public headers on the other hand will be added in <Pods/Headers/Public/<PodA/PodA/*.h


segiddins · 2018-02-06T18:40:45Z

lib/cocoapods/installer/xcode/pods_project_generator/file_references_installer.rb

-                    header_mappings(headers_sandbox, file_accessor, file_accessor.public_headers).each do |namespaced_path, files|
+                    # Public headers on the other hand will be added in <Pods/Headers/Public/<PodA/PodA/*.h
+                    # The extra folder is intentional in order for `<>` imports to work.
+                    header_mappings(headers_sandbox, file_accessor, file_accessor.public_headers, true).each do |namespaced_path, files|


looks like the 4th parameter should be a symbol rather than a bool?

segiddins · 2018-02-06T18:42:09Z

lib/cocoapods/target/pod_target.rb

@@ -571,13 +584,19 @@ def version
    # @return [Array<String>] The set of header search paths this target uses.
    #
    def header_search_paths(include_test_dependent_targets = false)
+      defines_module = defines_module?


no need to extract this into a variable, it's cached

segiddins · 2018-02-06T21:47:30Z

lib/cocoapods/sandbox/headers_store.rb

+        @search_paths_cache[key] = search_paths.uniq.flat_map do |entry|
+          path = "${PODS_ROOT}/#{headers_dir}/#{entry[:path]}"
+          paths = [path]
+          paths.push("#{path}/#{entry[:path].basename}") if !use_modular_headers && @visibility_scope == :public


we may also need to keep doing this for podspecs that define a header mappings dir, since they might be using something like j2objc that will expect things like com/company/foo/bar/header.h to be directly importable, without the module name prefix

dnkoutso · 2018-02-07T01:20:05Z

/cc @efirestone please take a look at the top of this PR for all header description.

segiddins · 2018-02-12T19:31:30Z

Updated integration specs, also fixed compatibility with pods that define a header_mappings_dir

segiddins · 2018-02-12T19:32:31Z

lib/cocoapods/target/pod_target.rb

+      return @defines_module if defined?(@defines_module)
+      return @defines_module = true if uses_swift? || requires_frameworks?
+
+      @defines_module = non_test_specs.any? { |s| s.consumer(platform).pod_target_xcconfig['DEFINES_MODULE'] == 'YES' }


we need to decide if we want to hoist this setting up into the Podfile, instead of leaving it in the podspec

dnkoutso · 2018-02-12T23:08:29Z

LGTM, thanks for the fix on header mappings dir, do we want to merge this and continue discussion of defines_module? in #6966?

paulb777 · 2018-02-12T23:35:46Z

lib/cocoapods/generator/xcconfig/aggregate_xcconfig.rb

+        # For >= 1.5.0 we use modular (stricter) header search paths this means that the integrated target will only be
+        # able to import public headers using `<>` or `@import` notation, but never import any private headers.
+        #
+        # For < 1.5.0 legacy header search paths the same rules apply: Its the wild west.


Its -> It's

paulb777 · 2018-02-12T23:38:03Z

spec/unit/generator/xcconfig/pod_xcconfig_spec.rb

@@ -133,13 +137,6 @@ module XCConfig
            @xcconfig.to_hash['PODS_TARGET_SRCROOT'].should == '${PODS_ROOT}/../../spec/fixtures/banana-lib'
          end

-          it 'adds the library build headers and public headers search paths to the xcconfig, with quotes' do


Can/should this be converted to a negative test?

I like it, I may have toyed with the idea but ultimately decided to remove. Will add back.

dnkoutso · 2018-02-15T01:41:03Z

lib/cocoapods/target/pod_target.rb

+      return @defines_module = true if uses_swift? || requires_frameworks?
+      return @defines_module = true if target_definitions.any? { |td| td.build_pod_as_module?(pod_name) }
+
+      @defines_module = non_test_specs.any? { |s| s.consumer(platform).pod_target_xcconfig['DEFINES_MODULE'] == 'YES' }


we decided to keep this for now

…appings dirs

segiddins · 2018-02-16T21:50:16Z

Updated, ready for final review!

dnkoutso · 2018-02-16T21:51:45Z

👍 👍

efirestone · 2018-02-16T22:51:27Z

A few possible gotchas I see:

By removing ${PODS_ROOT}/Headers/Public/ it means that PodA can no longer import PodC. This is correct, but has the potential to break existing pods, unless legacy support also brings back that import.
You make mention of using "" to import files like "PodA.h", but I don't believe this will work unless Pods is also populating the USER_HEADER_SEARCH_PATHS along with the standard HEADER_SEARCH_PATHS. By including the nested path (like ${PODS_ROOT}/Headers/Public/PodA/PodA) you'll keep the ability to import things like <PodA.h>, but I don't believe "PodA.h" will work. This isn't a change from existing behavior, but I wanted to clarify based on your comments.
We will eventually want to remove the nested folders (like ${PODS_ROOT}/Headers/Public/PodA/PodA) as they're a bad practice and can easily lead to conflicts. For example, if two dependencies have a NSString+Additions.h file, then which one gets imported with <NSString+Additions.h> is dependent on the order of the search paths. Instead we should require an explicit <PodA/NSString+Additions.h>.

segiddins · 2018-02-16T22:55:53Z

By removing ${PODS_ROOT}/Headers/Public/ it means that PodA can no longer import PodC. This is correct, but has the potential to break existing pods, unless legacy support also brings back that import.

Legacy support retains the ability to import everything

You make mention of using "" to import files like "PodA.h", but I don't believe this will work unless Pods is also populating the USER_HEADER_SEARCH_PATHS along with the standard HEADER_SEARCH_PATHS. By including the nested path (like ${PODS_ROOT}/Headers/Public/PodA/PodA) you'll keep the ability to import things like <PodA.h>, but I don't believe "PodA.h" will work. This isn't a change from existing behavior, but I wanted to clarify based on your comments.

Correct

We will eventually want to remove the nested folders (like ${PODS_ROOT}/Headers/Public/PodA/PodA) as they're a bad practice and can easily lead to conflicts. For example, if two dependencies have a NSString+Additions.h file, then which one gets imported with <NSString+Additions.h> is dependent on the order of the search paths. Instead we should require an explicit <PodA/NSString+Additions.h>.

The fully namespaced import is required when "modular_headers" are enabled for a pod

…ocoaPods#7412

dnkoutso requested review from segiddins and justinseanmartin February 6, 2018 18:32

dnkoutso force-pushed the strict_header_search_paths branch from ec2cbff to 6a2c009 Compare February 6, 2018 18:33

dnkoutso mentioned this pull request Feb 6, 2018

Update integration specs for modular header search paths CocoaPods/cocoapods-integration-specs#138

Closed

dnkoutso force-pushed the strict_header_search_paths branch from 6a2c009 to 6e27d9c Compare February 6, 2018 18:45

dnkoutso force-pushed the strict_header_search_paths branch from 6e27d9c to ea6c5c0 Compare February 6, 2018 19:44

dnkoutso commented Feb 6, 2018

View reviewed changes

dnkoutso force-pushed the strict_header_search_paths branch 2 times, most recently from 68c76b6 to cac5952 Compare February 6, 2018 20:58

dnkoutso added this to the 1.5.0 milestone Feb 6, 2018

dnkoutso force-pushed the strict_header_search_paths branch from cac5952 to 2dff7a8 Compare February 6, 2018 21:26

segiddins reviewed Feb 6, 2018

View reviewed changes

dnkoutso force-pushed the strict_header_search_paths branch 2 times, most recently from 4da688a to 953f391 Compare February 6, 2018 23:01

dnkoutso mentioned this pull request Feb 7, 2018

Option to set HEADER_SEARCH_PATHS to just "${PODS_ROOT}/Headers/Public"? #7011

Closed

segiddins mentioned this pull request Feb 12, 2018

Update for restructuring headers dir to allow stricter search paths CocoaPods/cocoapods-integration-specs#140

Merged

segiddins reviewed Feb 12, 2018

View reviewed changes

segiddins force-pushed the strict_header_search_paths branch from 31c0267 to 2867fac Compare February 12, 2018 19:43

segiddins requested a review from paulb777 February 12, 2018 23:33

paulb777 approved these changes Feb 12, 2018

View reviewed changes

dnkoutso force-pushed the strict_header_search_paths branch from 2867fac to 29160de Compare February 15, 2018 01:20

dnkoutso force-pushed the strict_header_search_paths branch from 29160de to 1fe2eab Compare February 15, 2018 01:32

dnkoutso commented Feb 15, 2018

View reviewed changes

dnkoutso and others added 3 commits February 16, 2018 13:48

Add support for modular header search paths, include "legacy" support

fb622fc

[PodTarget] Add compatible header search paths for pods with header m…

e2d3754

…appings dirs

Update integration specs

be48c58

segiddins force-pushed the strict_header_search_paths branch from c0e1bc2 to c6d6d5d Compare February 16, 2018 21:50

Allow enabling modular headers in the podfile

95d59b9

segiddins force-pushed the strict_header_search_paths branch from c6d6d5d to 95d59b9 Compare February 16, 2018 22:12

dnkoutso merged commit 8307a68 into CocoaPods:master Feb 16, 2018

dnkoutso deleted the strict_header_search_paths branch February 16, 2018 23:23

dnkoutso mentioned this pull request Feb 21, 2018

Fix legacy header search paths #7445

Merged

dnkoutso added a commit to dnkoutso/CocoaPods that referenced this pull request Feb 21, 2018

Fix legacy header search paths that broke due to CocoaPods#7116 and C…

d07b986

…ocoaPods#7412

dnkoutso added a commit to dnkoutso/CocoaPods that referenced this pull request Feb 22, 2018

Fix legacy header search paths that broke due to CocoaPods#7116 and C…

1bcbadc

…ocoaPods#7412

dnkoutso added a commit to dnkoutso/CocoaPods that referenced this pull request Feb 22, 2018

Fix legacy header search paths that broke due to CocoaPods#7116 and C…

b1035d4

…ocoaPods#7412

This was referenced May 1, 2018

Application fails to be archived as iOS if installing SDK with cocoapods 1.5.0 forcedotcom/SalesforceMobileSDK-iOS#2504

Closed

Unable to build iOS native app with Xcode 9.3 and Salesforce sdk 6.1 forcedotcom/SalesforceMobileSDK-iOS#2512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for modular header search paths, include "legacy" support #7412

Add support for modular header search paths, include "legacy" support #7412

dnkoutso commented Feb 6, 2018 •

edited

paulb777 commented Feb 6, 2018

dnkoutso commented Feb 6, 2018

paulb777 commented Feb 6, 2018

dnkoutso Feb 6, 2018

dnkoutso Feb 6, 2018

dnkoutso Feb 7, 2018

dnkoutso commented Feb 6, 2018 •

edited

paulb777 commented Feb 6, 2018

segiddins commented Feb 6, 2018

segiddins Feb 6, 2018

segiddins Feb 6, 2018

segiddins Feb 6, 2018

segiddins Feb 6, 2018

segiddins Feb 6, 2018

dnkoutso commented Feb 7, 2018

segiddins commented Feb 12, 2018

segiddins Feb 12, 2018

dnkoutso commented Feb 12, 2018 •

edited

paulb777 Feb 12, 2018

paulb777 Feb 12, 2018

dnkoutso Feb 13, 2018

dnkoutso Feb 15, 2018

dnkoutso Feb 15, 2018 •

edited

segiddins commented Feb 16, 2018

dnkoutso commented Feb 16, 2018

efirestone commented Feb 16, 2018

segiddins commented Feb 16, 2018

Add support for modular header search paths, include "legacy" support #7412

Add support for modular header search paths, include "legacy" support #7412

Conversation

dnkoutso commented Feb 6, 2018 • edited

paulb777 commented Feb 6, 2018

dnkoutso commented Feb 6, 2018

paulb777 commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnkoutso commented Feb 6, 2018 • edited

paulb777 commented Feb 6, 2018

segiddins commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnkoutso commented Feb 7, 2018

segiddins commented Feb 12, 2018

Choose a reason for hiding this comment

dnkoutso commented Feb 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnkoutso Feb 15, 2018 • edited

Choose a reason for hiding this comment

segiddins commented Feb 16, 2018

dnkoutso commented Feb 16, 2018

efirestone commented Feb 16, 2018

segiddins commented Feb 16, 2018

dnkoutso commented Feb 6, 2018 •

edited

dnkoutso commented Feb 6, 2018 •

edited

dnkoutso commented Feb 12, 2018 •

edited

dnkoutso Feb 15, 2018 •

edited