Add directlyEmbedCarthageDependencies to Target #361
Conversation
brentleyjones
force-pushed
the
disable-copy-frameworks
branch
from
6a404ff to
57d7605
Compare
Copies the pattern from `getAllDependenciesPlusTransitiveNeedingEmbedding`. They should be combined into one solution eventually.
Allows for choice on if `copy-frameworks` or an `Embed Frameworks` build phase should be used to embed Carthage framworks. Defaults to `true` for macOS targets to match current behavior.
…nies If using `transitivelyLinkDependencies` this already worked. In the future `getAllDependenciesPlusTransitiveNeedingEmbedding` and `getAllCarthageDependencies` should really be the same mechanism.
brentleyjones
force-pushed
the
disable-copy-frameworks
branch
from
57d7605 to
0ec3b1d
Compare
| } | ||
|
|
||
| for dependency in carthageDependencies { | ||
| guard target.type != .staticLibrary else { break } |
There was a problem hiding this comment.
nits: I prefer if than guard in this case.
There was a problem hiding this comment.
If the target type is non dependant on the dependencies, could the if go outside this loop?
| var frameworkPath = platformPath + dependency.reference | ||
| if frameworkPath.extension == nil { | ||
| frameworkPath = Path(frameworkPath.string + ".framework") | ||
| } |
There was a problem hiding this comment.
Maybe we can always add the .framework extension, without if statement.
There was a problem hiding this comment.
You can list extension as part of a carthage dependency. Also, both of these are just copying what was there in the above for loop. I would like to keep both of them the same for this PR, and if they should change they can in a different one. 😁
|
@yonaskolb instead of a configurable value I could just change the default. Thinking more about it I think the default should only use the script for iOS/tvOS/watchOS Application targets. I’m going to be updating Carthage’s docs to say the same thing based on the conversation over there. Let me know if you want the value or not (I’ll update the default in a little bit). The only reason I can think of why someone would want a configurable value, to override that default, is if they wanted to create umbrella frameworks on iOS (which isn't really "supported" by Apple). Personally I feel safer having this as an configurable value though, since someone out there might want to change the default. |
`directlyEmbedCarthageDependencies` is only `false` now for iOS/tvOS/watchOS applications.
|
Carthage/Carthage#2537 for update to their docs. Re-reading them they do say to only use it in iOS/tvOS/watchOS applications. |
|
I'm always hesitant to add too many options, as they will need to be supported going forward, and the name will have to stick around. Especially if the outcome can be achieved with build settings. In this case though it's something you can't change with a manual build setting. In terms of the default, what does that mean for tests that run on a device? Are there also no other cases where things need stripping? |
|
|
||
| let targetDependencies = (target.transitivelyLinkDependencies ?? project.options.transitivelyLinkDependencies) ? | ||
| getAllDependenciesPlusTransitiveNeedingEmbedding(target: target) : target.dependencies | ||
|
|
||
| let directlyEmbedCarthage = target.directlyEmbedCarthageDependencies ?? !(target.platform.requiresSimulatorStripping && target.type.isApp) |
There was a problem hiding this comment.
type.isApp actually includes tests as well. Is that on purpose?
There was a problem hiding this comment.
No. Tests have an extension of .xctests. isApp checks for .app correct?
There was a problem hiding this comment.
Check the getter, it includes test targets 👍
isApp is used in other places. It's ok if you want to create new property and rename the usages to be more explicit about what they are checking
There was a problem hiding this comment.
Sorry I take all that back. I was thinking of isExecutable
|
You only need to strip when submitting to the App Store, so tests run on device are fine. Sent with GitHawk |
|
I agree on this being an option since you can't change it with a build setting. |
Resolves #359.