-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Xcframework prebuilt binaries #3123
Xcframework prebuilt binaries #3123
Conversation
I've tested this against the latest https://github.com/aws-amplify/aws-sdk-ios, which provides prebuilt XCFramework binaries and this works as expected! Using Carthage 0.37.0 we see:
Using this branch we see:
And the prebuilt frameworks are in the expected folder. |
Hi @igstewart3 , I just ran into this today with a similar hiccup with AWS (wowza HUGE install times). Thanks for kicking off the effort. I'm not a repo owner/maintainer, but wanted to give some notes as I just started dabbling with this too: Reading through I noticed this used string literals and bool logic to concatenate framework download behaviors. Skimming the implementation, I noticed:
And was wondering if perhaps xcframework should be included as a sibling here? Allowing for the use of this definition for path? Allowing for a model to drive potential differentiation rather than bool checks?
|
Good shout, I'll take a look at tidying that up 👍 |
I can confirm as well, the fix in this branch works, please merge and release this ASAP, this blocks some frameworks from being properly distributed. Thanks |
Source/CarthageKit/Project.swift
Outdated
@@ -1447,11 +1457,13 @@ func platformForFramework(_ frameworkURL: URL) -> SignalProducer<SDK, CarthageEr | |||
/// Sends the URL to each framework bundle found in the given directory. | |||
internal func frameworksInDirectory(_ directoryURL: URL) -> SignalProducer<URL, CarthageError> { | |||
return filesInDirectory(directoryURL, kUTTypeFramework as String) | |||
.concat(filesInDirectory(directoryURL, "com.apple.xcframework")) |
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.
indentation fix needed.
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.
Done 👍
Source/CarthageKit/Xcode.swift
Outdated
@@ -1413,6 +1419,9 @@ public func nonDestructivelyStripArchitectures(_ frameworkURL: URL, _ architectu | |||
|
|||
/// Strips the given architectures from a framework. | |||
private func stripArchitectures(_ packageURL: URL, _ architectures: Set<String>) -> SignalProducer<(), CarthageError> { | |||
guard isNotXCFramework(packageURL) else { |
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.
indentation fix needed. Please check the indentation of return
statement below it.
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.
Done 👍
Source/CarthageKit/Xcode.swift
Outdated
@@ -1431,6 +1440,9 @@ private func stripArchitectures(_ packageURL: URL, _ architectures: Set<String>) | |||
|
|||
// Returns a signal of all architectures present in a given package. | |||
public func architecturesInPackage(_ packageURL: URL, xcrunQuery: [String] = ["lipo", "-info"]) -> SignalProducer<[String], CarthageError> { | |||
guard isNotXCFramework(packageURL) else { |
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.
indentation fix needed. Please check the indentation of return statement below it.
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.
Done for all changes made in the PR 👍
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.
First off, thanks so much for your contribution! I think this makes a lot of sense and is obviously something we need to support.
Aside from the suggestions below, we need to handle checkFrameworkCompatibility
correctly. Since it's possible to build an xcframework with -allow-internal-distribution
, it's possible to have swiftmodules in the xcframework that aren't compatible with the machine they're downloaded to. This patch should fix it, LMK if there's anything I can clarify:
carthage-3123-additions.patch.txt
Source/CarthageKit/Xcode.swift
Outdated
|
||
private func isNotXCFramework(_ frameworkUrl: URL) -> Bool { | ||
return FrameworkSuffix.from(string: frameworkUrl.pathExtension).value != .xcframework | ||
} |
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.
What is this (and all its call sites) doing? AFAICT this is related to copy-frameworks, not downloading binaries?
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.
Several of these methods are called when installing the prebuilt XCFrameworks and fail due to not being able to find the executableURL in the bundle in binaryURL(packageUrl:)
. We could look into adding a custom case for XCFrameworks like is already in place for .dSYM and (and changing to return an array of URLs), but many of these processes (stripping architectures and debug symbols) don't necessarily make sense for an XCFramework anyway.
@elliottwilliams Thanks very much for the feedback! I've implemented most of it, including the patch you provided. I think only the XCFramework guard cases in |
@igstewart3 I think I got it working – we just need to avoid calling into the dSYM/bcsymbolmap logic higher up. This is similar to what we do on the I confirmed this works with a |
@elliottwilliams Nice! Good fix, that looks a lot tidier. Everything seems to be working in my tests as well 👍 |
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.
Sounds good! Going to give the other maintainers a moment to review if they'd like.
… update --use-xcframeworks` when installing `dd-sdk-ios` in other app. We need this as `arm64` is disabled in PLCrashReporter simulator builds, so we cannot produce `DatadogCrashReporting.xcframework` as it is not able to link `CrashReporter` to our `ios-arm64_x86_64-simulator` slice. This change makes our SDK produce smaller slice, which is enough for running on simulator. This was discovered as Carthage doesn’t yet support pre-build xcframeworks: Carthage/Carthage#3123
… update --use-xcframeworks` when installing `dd-sdk-ios` in other app. We need this as `arm64` is disabled in PLCrashReporter simulator builds, so we cannot produce `DatadogCrashReporting.xcframework` as it is not able to link `CrashReporter` to our `ios-arm64_x86_64-simulator` slice. This change makes our SDK produce smaller slice, which is enough for running on simulator. This was discovered as Carthage doesn’t yet support pre-build xcframeworks: Carthage/Carthage#3123
This PR adds the ability to retrieve XCFramework prebuilt binaries. It installs them into the Carthage/Build folder and skips any Xcode.swift steps that involve checking the architecture.
It could probably use a wee bit of guidance from someone more familiar with the project to ensure I haven't missed something important.
I have tested this on a local project and was successfully able to install XCFramework binaries, as well as regular Framework binaries from a previous release.
Note: I have not been able to run the unit tests yet due to some Swift compile errors I haven't been able to resolve.