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

Revert creation of module maps for static libraries #3898

Merged
merged 8 commits into from
Jul 25, 2015

Conversation

neonichu
Copy link
Member

Reverts #3878

Fixes #3879, #3888, #3884, #3886, #3889

Most importantly, we actually cannot support this feature in CocoaPods, because of how ENABLE_CLANG_MODULES works. This option is enabled by default, but acts a little bit different from its documentation in that it will automatically convert all imports of headers outside of the given target to module imports if a module exists. So the generated modules wouldn't be opt-in, but automatically used. Since not all Pods are working as modules, this makes this change unsuitable.

Integration spec PR: CocoaPods/cocoapods-integration-specs#42

@@ -71,40 +71,18 @@ def implode!
# @return [Array<Pathname>]
#
def add_files(namespace, relative_header_paths, platform)
relative_header_paths.map do |relative_header_path|
Copy link
Member

Choose a reason for hiding this comment

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

I think these changes were fine, though?

neonichu added a commit that referenced this pull request Jul 24, 2015
@neonichu neonichu changed the title WIP: Revert creation of module maps for static libraries Revert creation of module maps for static libraries Jul 24, 2015
create_umbrella_header do |generator|
generator.imports += target.file_accessors.flat_map(&:public_headers).map(&:basename)
end
link_module_map
Copy link
Contributor

Choose a reason for hiding this comment

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

What about Pods that provide custom module maps? If the pod provides the module map, then CocoaPods should still be able to link it safely in the sandbox. It's the generated ones that are the problem.

Copy link
Member

Choose a reason for hiding this comment

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

Realm has a custom module map, and we got reports of issues as well

Copy link
Member Author

Choose a reason for hiding this comment

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

The ENABLE_CLANG_MODULES issue is just the ultimate reason why this cannot work at all for generated module maps, there were apparently other practical issues with this change in some situations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Yep, I can see at least one reason why the Realm pod broke: it finds both the original modulemap and the module map in Headers/Public. :(

Copy link
Member

Choose a reason for hiding this comment

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

Looked into trying to get this to work tonight, and no dice.

@segiddins
Copy link
Member

I'm good with this 👍

neonichu added a commit that referenced this pull request Jul 25, 2015
@neonichu neonichu force-pushed the revert-module-support-for-static-libraries branch from b2d1e78 to 73c779d Compare July 25, 2015 08:37
@@ -12,6 +12,17 @@ To install release candidates run `[sudo] gem install cocoapods --pre`
headers.
[Russ Bishop](https://github.com/russbishop)
[#3893](https://github.com/CocoaPods/CocoaPods/issues/3893)
##### Fixed
Copy link
Member

Choose a reason for hiding this comment

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

belongs in the bug fixes section?

@segiddins
Copy link
Member

👍

segiddins added a commit that referenced this pull request Jul 25, 2015
…tic-libraries

Revert creation of module maps for static libraries
@segiddins segiddins merged commit 99394b2 into master Jul 25, 2015
@segiddins segiddins deleted the revert-module-support-for-static-libraries branch July 25, 2015 18:36
@coveralls
Copy link

coveralls commented Apr 18, 2017

Coverage Status

Changes Unknown when pulling c17f843 on revert-module-support-for-static-libraries into ** on master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants