-
Notifications
You must be signed in to change notification settings - Fork 822
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
Use carthage xcframeworks #1020
base: master
Are you sure you want to change the base?
Conversation
a36092c
to
e7ee5f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Ongoing
@@ -677,7 +681,7 @@ public class PBXProjGenerator { | |||
|
|||
let targetSupportsDirectEmbed = !(target.platform.requiresSimulatorStripping && | |||
(target.type.isApp || target.type == .watch2Extension)) | |||
let directlyEmbedCarthage = target.directlyEmbedCarthageDependencies ?? targetSupportsDirectEmbed | |||
let directlyEmbedCarthage = target.directlyEmbedCarthageDependencies ?? project.options.carthageXCFrameworks || targetSupportsDirectEmbed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
↓ may be correct?
let directlyEmbedCarthage = target.directlyEmbedCarthageDependencies ?? project.options.carthageXCFrameworks || targetSupportsDirectEmbed | |
let directlyEmbedCarthage = target.directlyEmbedCarthageDependencies ?? (project.options.carthageXCFrameworks || targetSupportsDirectEmbed) |
This makes one failed test passing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO we don't need to handle carthage dependency specially any more because XCFramework removed carthage copy-framework
step. We can add carthage dependency as a normal framework dependency like: framework: Carthage/Build/Alamofire.xcframework
.
Yeah, but making things simple often leads some redundancy, so I understand the demand of shorthand.
But even if we provide a shorthand for framework: Carthage/Build/Alamofire.xcframework
, we should separate API completely in the aspect of backward compatibility, I think.
IIUC, carthage build
will produce XCFramework by default in the near future. So we may need to set carthageXCFrameworks
true by default.
However, in that case, it will not be compatible with the old project.yml which uses carthage: Alamofire
without explicit carthageXCFrameworks
and doesn't use xcframeworks. If that project will update XcodeGen to the future version which enables carthageXCFrameworks
by default, xcodeproj can't be generated without explicit carthageXCFrameworks: false
.
On the other hand, if we won't make carthageXCFrameworks
true by default, we have to write carthageXCFrameworks: true
for the rest of our life.
So we need to re-think we should provide a shorthand or not, and the shorthand API should be separated from the current API or not.
I tried this out with our project and an issue I'm noticing is that since carthage supports getting precompiled frameworks, it's possible to get a mix of xcframeworks and frameworks. This flag seems to assume everything will be available as a xcframework. As far as I can tell, there isn't a great way to know whether a dependency will be a xcframework when the project is generated. The only ways I can think of would be:
|
Hi there, is there any progress here? |
This enables XcodeGen to look up the xcframeworks that Carthage 0.37 builds.
This PR is still a work in progress, but put up for reference.
Resolves #1006
For now it requires adding
carthageXCFrameworks: true
to the spec options