Ensure private headers are declared as such in a framework's generated module map.

[segiddins]

Closes #2974.

\c @mrackwitz @kylef

[neonichu]

Do we have a good reason to generate our module map differently from Xcode defaults (see #2974 (comment))?

[kylef]

I'm not entirely sure if this is correct.

Shouldn't the output be something like:

module Foo {
  header "Foo.h"

  explicit module Private {
    header "Foo_Private.h"
  }
}

Ref: http://clang.llvm.org/docs/Modules.html#private-module-map-files

[kylef]

There doesn't seem to be any tests to ensure generate works, can we add some? (I understand if you feel this is outside of this PR, can be done separately, but I think it makes sense to add them in this PR to make sure this works correctly).

[segiddins]

Sure, can add some tests later.

[segiddins]

A header with the private specifier may not be included from outside the module itself.

[segiddins]

There doesn't seem to be any tests to ensure generate works

That's the integration specs

[mrackwitz]

@neonichu: In which way different?
Generally, we need module maps to inject our auto-generated umbrella header.
Do you mean different from only the export * declaration?

[neonichu]

@mrackwitz I was referring to including the private headers in them, Xcode does not do that, it will merely include the actual files in a PrivateHeader directory.

[segiddins]

note to self: add unit test for spec with private headers

[segiddins]

@kylef rebased + added spec

[neonichu]

Hm, this only works if one specifies private_header_files manually, is that intended? I would have thought this would include all private headers, regardless of definition. Moreover, the private headers are not copied to the framework by CP, so this wouldn't even do anything rn, would it?

[kylef]

@neonichu A header wouldn't be private unless it has presence in the pod spec's private_header_files?

[neonichu]

@kylef seems unintuitive for me, especially since we introduced Pods/Headers/{Private,Public} - that suggest that a header is either considered private or public by CP and cannot be neither.

This is also what http://guides.cocoapods.org/syntax/podspec.html#private_header_files suggests, so at the very least, we have to change that.

[neonichu]

To clarify what I mean, the current implementation in this PR would add b.h to the module map in this scenario:

source_files = a.h b.h
private_header_files = b.h

but not in this one:

source_files = a.h b.h
public_header_files = a.h

I don't think that is intended.

[segiddins]

Boris, I'm pretty sure it would in the latter. Check out the file accessor.

-Samuel E. Giddins

On Jan 19, 2015, at 4:02 AM, Boris Bügling notifications@github.com wrote:

To clarify what I mean, the current implementation in this PR would add b.h to the module map in this scenario:

source_files = a.h b.h
private_header_files = b.h
but not in this one:

source_files = a.h b.h
public_header_files = a.h
—
Reply to this email directly or view it on GitHub.

[neonichu]

@segiddins didn't work for me in manual testing - will have a look at why that is the case, first wanted to confirm that I am not missing something and that behaviour was intended.

[neonichu]

@segiddins So the file accessor is actually the issue.

https://github.com/CocoaPods/CocoaPods/pull/3004/files#diff-a1bd48eb509ce3365557319fdda59f25R130 contains only the contents of the private_header_files attribute of a spec. I think headers - public_headers should actually be the implementation of that method.

But fixing that breaks the build of frameworks anyway, possibly because the private headers are not actually copied into the framework. Unfortunately, the error message is only the ever elusive "Could not build Objective-C module".

[neonichu]

Not copying the headers is indeed the issue, we need to add handling for private headers here https://github.com/CocoaPods/CocoaPods/blob/seg-module-map-private-headers/lib/cocoapods/installer/target_installer/pod_target_installer.rb#L68, similar to the public ones.

As this copies the headers into the PrivateHeaders directory, this also clears up my earlier confusion here #3004 (comment)

[segiddins]

I think #3145 will make this work?

[segiddins]

@mrackwitz please see that this will work in conjunction with the custom modulemap PR?

[mrackwitz]

We could add -Wincomplete-umbrella to all generated Pod targets:

Any headers not included by the umbrella header should have explicit header declarations. Use the -Wincomplete-umbrella warning option to ask Clang to complain about headers not covered by the umbrella header or the module map.

This could help to find misconfigurations of the header visibility in the podspec with pod [spec|lib] lint. This could also be helpful in combination with custom modulemaps or we disable it in that case and don't add this additional flag for warnings. But the documentation doesn't cover how clang handles project headers, which are since #3145 possible with CocoaPods, by adding headers in the source_files of a podspec, which are neither matched by public_headers nor by private_headers.

[segiddins]

@mrackwitz if we add -Wincomplete-umbrella, will this be mergable?

[mrackwitz]

@segiddins: That should go in a separate issue.

[segiddins]

In that case, what needs to be done for this to be merged?

[mrackwitz]

Rebuilding the integration spec. ✅