Allow declaring modulemap file in Target #1274
Conversation
In order to be able to include a Copy Headers build phase including the modulemap and other public headers, which will support .a libraries. Thanks to @pepibumur for guiding me!
There was a problem hiding this comment.
Nice work @natanrolnik - thanks for tackling this 👍
Left some initial thoughts
| @@ -104,7 +109,8 @@ public struct Target: Codable, Equatable { | |||
| dependencies: [TargetDependency] = [], | |||
| settings: Settings? = nil, | |||
| coreDataModels: [CoreDataModel] = [], | |||
| environment: [String: String] = [:]) { | |||
| environment: [String: String] = [:], | |||
| modulemap: Path? = nil) { | |||
There was a problem hiding this comment.
minor:
What is everyones thoughts on moving it to somewhere in the middle (e.g. after entitlements)?
Thinking of how it appears in usage?
let target = Target(
name: "MyTarget",
dependencies: [
// ..
],
modulemap: "path/to/module.modulemap")let target = Target(
name: "MyTarget",
modulemap: "path/to/module.modulemap",
dependencies: [
// ..
])| if let headers = target.headers { | ||
| try generateHeadersBuildPhase(headers: headers, | ||
| includePublicFiles: target.modulemap == nil, |
There was a problem hiding this comment.
If I'm not mistaken the criteria here would be to omit the entire build phase if product == .staticLibrary,
we can introduce a helper in TuistCore/Target along side some of the others there to achieve something along those lines:
if let headers = target.headers, target.supportsHeadersPhase {
try generateHeadersBuildPhase( ... )
}| pbxTarget: pbxTarget, | ||
| fileElements: fileElements, | ||
| pbxproj: pbxproj) | ||
| } | ||
|
|
||
| if let modulemap = target.modulemap { |
There was a problem hiding this comment.
I believe the criteria for including the generateCopyHeadersBuildPhase is when !target.supportsHeadersPhase.
e.g. A normal .framework allows specifying a modulemap and does not require any special copy, Xcode automatically deals with that.
| if let definesModules = config.buildSettings["DEFINES_MODULE"] { | ||
| let message = "The target \(target.name) has DEFINES_MODULE build setting set to " | ||
| + "\"\(definesModules)\" in \(config.name). " | ||
| + "It will be set to \"YES\" because `modulemap` was set in the target to \"\(relativeModulemap)\"." | ||
| logger.log(level: .warning, Logger.Message(stringLiteral: message)) |
There was a problem hiding this comment.
For consistency - we can add this as a lint warning within SettingsLinter for example
| config.buildSettings["DEFINES_MODULE"] = "YES" | ||
| config.buildSettings["MODULEMAP_FILE"] = relativeModulemap |
There was a problem hiding this comment.
Target settings are usually updated in ConfigGenerator.generalTargetDerivedSettings - we can move that there for consistency :)
| @@ -128,6 +128,10 @@ class ProjectFileElements { | |||
| files.insert(entitlements) | |||
| } | |||
|
|
|||
| if let modulemap = target.modulemap { | |||
There was a problem hiding this comment.
👍 - awesome this means modulemaps will automatically appear in Xcode navigator - no longer need to manually add them to additionalFiles in the manifest! 🎉
|
I would love to continue this PR. But unfortunately, at the moment, I don't have the capacity to do all the work needed. |
In order to be able to include a Copy Headers build phase including the modulemap and other public headers, which will support .a libraries.
Resolves #1254
Thanks to @pepibumur for guiding me, over the step described in the issue above by @kwridan.
This PR still requires adding tests and in a later stage (or commit) we will add the suggested linting as well.