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

[Frameworks] Allow to customize the module name #205

Closed
mrackwitz opened this issue Nov 30, 2014 · 18 comments · Fixed by #209
Closed

[Frameworks] Allow to customize the module name #205

mrackwitz opened this issue Nov 30, 2014 · 18 comments · Fixed by #209
Milestone

Comments

@mrackwitz
Copy link
Member

Regarding Framework / Swift Support by CocoaPods/CocoaPods#2835:

By supporting Frameworks and Swift, we have also support Clang modules. Module names are C99-extended identifiers, which means [a-Z_][a-Z0-9_]*. Some podspecs use names beginning with a number or containing - or .. These characters are not possible in the module name and would be replaced by _. (That's the same what happens by e.g. ${PRODUCT_NAME:c99ext_identifier}.)
@orta proposed, we could solve that by allowing to define an alternative module name by the Podspec DSL. At the moment you can give your library another name for imports by specifying the header_dir in the podspec. E.g. it is solved in that way for Facebook-iOS-SDK. This option is currently not considered for frameworks on CocoaPods/CocoaPods#2835, so you would need to @import Facebook_iOS_SDK. We can derive it in the following way to keep maximum compatibility with user projects:

spec.module_name ?? c99ext_identifier(spec.header_dir) ?? c99ext_identifier(spec.name)

Where the attribute module_name is new to the Podsepc DSL and has to be introduced. It should be validated to allow only C99-extended identifiers.

@segiddins
Copy link
Member

@mrackwitz if we do this, then we'll have to handle the potential for duplicated module names in CocoaPods

@neonichu
Copy link
Member

neonichu commented Dec 2, 2014

I wonder if we really need to extend the DSL for this or if allowing it to be implicitly customisable via spec.header_dir does suffice.

@segiddins
Copy link
Member

or, we just recommend that people name their pods in a C99-compatible manner.

@orta
Copy link
Member

orta commented Dec 3, 2014

I think the example that spawned this issue is a good case for adding the DSL, "LibName.swift" -> import LibName. A library name could already be anything.

@mrackwitz
Copy link
Member Author

@segiddins: There are already a lot of podspecs out, which have incompatible names. While I can understand the point of duplicate names, I don't think that's necessarily a problem we have to solve. This can already occur now.

@neonichu: The implicit customization would suffice probably from a technical point of view regarding the integration / build outputs. But I would prefer to deliver our users a clean way, even when we spawn by this more potential combinations, which need more effort to cover by tests.

To have a distinct property module_name has some further advantages: if pod authors specify the module name, it raises their attention to the limited character set, which can be linted, so that they are able to come up with a good name. Furthermore this would ensure that the module name occurs at least once in the project, which increases the discoverability. We could even warn about implicit derived module names, which are different from the pod names and recommend to declare this name explicit. Another or further idea would be to serialize it explicit in the JSON representation.

@segiddins
Copy link
Member

@mrackwitz it can't happen now, because you #import <PodName/Header.h>, so the imports are scoped by the Pod name.

@neonichu
Copy link
Member

neonichu commented Dec 5, 2014

@segiddins not if one uses header_dir :)

@alloy
Copy link
Member

alloy commented Dec 5, 2014

The example that @orta mentions is a good one and so I think an attribute for it is warranted, but I think it should definitely default to header_dir || name, as @mrackwitz indicated.

We could even warn about implicit derived module names, which are different from the pod names and recommend to declare this name explicit.

@mrackwitz I don’t think we should add warnings for things that we can have sensible defaults for, but what you can do is show the inflected module name so that the author can at least see it and think about it. E.g. a ‘note’ in the linter.

Warnings tend to leave people with an annoyance if they in fact know what they’re doing, so it should be limited to things that really do need a warning.

@neonichu
Copy link
Member

neonichu commented Dec 5, 2014

I see the issue more in application code, an implicitly derived module name will be an unexpected breaking change for all current users once we eventually opt everyone into frameworks. If module_name and header_dir diverge, a pod could also be broken for either the static library or the framework case.

@alloy
Copy link
Member

alloy commented Dec 5, 2014

@neonichu I guess I’m missing a few pieces of the puzzle here as to why/how those breakages can occur. Can you elaborate?

@neonichu
Copy link
Member

neonichu commented Dec 5, 2014

@alloy Just in terms of header paths. Example: pod name or header_dir is "A-B" and we would convert it to "A_B". Any import of the form #import <A-B/A.h> would not work anymore.

@alloy
Copy link
Member

alloy commented Dec 5, 2014

@neonichu Gotcha. I don’t see how implicitly derived differs in that aspect, though?

@neonichu
Copy link
Member

neonichu commented Dec 5, 2014

@alloy nope, in fact using explicit names brings an additional can of worms for those Pods whose name is already a valid module name.

@alloy
Copy link
Member

alloy commented Dec 5, 2014

Correct me if I’m wrong, but can’t we have #import <A-B/A.h> still work for Objective-C and have @import Abba work for Objective-C and Swift? So when using static libs you use the former, and when you use modules/frameworks you use the latter form?

using explicit names brings an additional can of worms for those Pods whose name is already a valid module name

Yeah that would open up the possibility for even more complex scenarios. Hrmm.

@mrackwitz
Copy link
Member Author

@alloy:

I don’t think we should add warnings for things that we can have sensible defaults for, but what you can do is show the inflected module name so that the author can at least see it and think about it. E.g. a ‘note’ in the linter.

Warnings tend to leave people with an annoyance if they in fact know what they’re doing, so it should be limited to things that really do need a warning.

Good points! 👍

@neonichu
Copy link
Member

neonichu commented Dec 6, 2014

@alloy the import path and module name cannot differ as the former is derived from the name of the framework when using the -framework option to specify them.

I think the complexity of module_name vs. header_dir is best taken care of by building both framework and static library during lint if a spec author has defined an explicit module name. That ensures that the spec is at least working for both cases.

@alloy
Copy link
Member

alloy commented Dec 8, 2014

@neonichu Gotcha, thanks for the info 👍

akisute pushed a commit to akisute/YTPlayerView2 that referenced this issue Mar 22, 2016
I found that the name of the built framework is changed to `youtube_ios_player_helper` when framework (clang module) is enabled because of the clang module specification:
CocoaPods/Core#205

To make things worse this only happens when clang module is enabled, so make thing so much completecated. I can't keep using the name `youtube-ios-player-helper` anymore because of this, so I'm going to rename the entire module name to simple `YTPlayerView` in next commit.
akisute pushed a commit to akisute/YTPlayerView2 that referenced this issue Mar 22, 2016
CocoaPods/Core#205

Just learned from Facebook-iOS-SDK. Now cocoapods also supports module_name as well, so I added it too.
@hemangshah
Copy link

@orta , can you please give an example of using module_name with a Swift project's pod spec?

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 a pull request may close this issue.

6 participants