Add requiresObjCLinking to Target
#353
Conversation
| if let string = buildSettings[otherLinkingFlags] as? String { | ||
| buildSettings["OTHER_LDFLAGS"] = "\(string) -ObjC" | ||
| } else { | ||
| buildSettings["OTHER_LDFLAGS"] = "$(inherited) -ObjC" |
There was a problem hiding this comment.
This is the new code in here. Lots of red and green since this section had to be moved below targetDependencies generation
|
I'm not convinced all this complexity and performance hit is worth it. Is there a big downside to either just adding |
Sources/ProjectSpec/Target.swift
Outdated
| @@ -159,6 +162,63 @@ extension Target { | |||
| merged["targets"] = crossPlatformTargets | |||
| return merged | |||
| } | |||
|
|
|||
| public var shouldEmbedDependencies: Bool { | |||
| return type.isApp || type.isTest | |||
There was a problem hiding this comment.
This could be an extension on PBXProjectType like the getters it's using
Sources/ProjectSpec/Target.swift
Outdated
| } | ||
| } | ||
|
|
||
| private func getAllDependenciesPlusTransitiveNeedingEmbedding(project: Project) -> [Dependency] { |
There was a problem hiding this comment.
I'd rather keep all this code out of the ProjectSpec module. Some people use ProjectSpec as a seperate library without XcodeGen's logic.
But mostly this also removes the ability to add caching across the project as there is no controller for this logic
There was a problem hiding this comment.
Removed that commit altogether, it's back in XcodeGenKit.
| } | ||
|
|
||
| // objc linkage | ||
| let anyDependencyRequiresObjCLinking = targetDependencies.contains { dependency in |
There was a problem hiding this comment.
This could be quite performance intensive
There was a problem hiding this comment.
All searching of sources was removed, and it now just sets a flag once as soon as anyDependencyRequiresObjCLinking would be true. Performance shouldn't be an issue anymore.
| } | ||
| if anyDependencyRequiresObjCLinking { | ||
| let otherLinkingFlags = "OTHER_LDFLAGS" | ||
| if let string = buildSettings[otherLinkingFlags] as? String { |
There was a problem hiding this comment.
The setting may also be a [String]. Perhaps a convenience somewhere that appends an item to a list style setting using $(inherited) would be good
There was a problem hiding this comment.
I changed it to follow the pattern. I didn't add a convenience though.
|
|
||
| func dependencySourceFiles() -> [SourceFile] { | ||
| // This is duplicative and should be cached | ||
| return (try? sourceGenerator.getAllSourceFiles(sources: dependencyTarget.sources)) ?? [] |
There was a problem hiding this comment.
This would generate duplicates wouldn't it?
Docs/ProjectSpec.md
Outdated
| - [ ] **dependencies**: **[[Dependency](#dependency)]** - Dependencies for the target | ||
| - [ ] **transitivelyLinkDependencies**: **Bool** - If this is not specified the value from the project set in [Options](#options)`.transitivelyLinkDependencies` will be used. | ||
| - [ ] **requiresObjCLinking**: **Bool** - If this is `true` any targets that link to this target will have `-ObjC` added to their `OTHER_LDFLAGS`. Defaults to `true` if `type` is `library.static` and [Sources](#sources) contain any `.swift`, `.m`, or `.mm` files. |
There was a problem hiding this comment.
People would really have to have a deep understanding of what this means
|
Great feedback. I’ll adjust it to not look at the sources and just default the value to I want to keep the property though, since I see it being added to the Ohh, and to answer your question about just putting this on targets: there is a very real binary size hit you take if you use this option without he need for it. I’ll make sure the documentation reflects this. |
brentleyjones
force-pushed
the
objcLinking
branch
from
8b0b65c
to
a115fa2
Compare
|
@yonaskolb Addressed your feedback. Per the other PR's comments, I'll add new commits instead of amending next time. |
Allows config to reference targer dependencies.
Allows a Target to indicate that any target that links to it needs to set `-ObjC` in `OTHER_LDFLAGS`.
brentleyjones
force-pushed
the
objcLinking
branch
from
a115fa2
to
15af4fd
Compare
Child of #350. Addresses 2.ii.