Support swift build --experimental-xcframeworks-on-linux#1310
Conversation
owenv
left a comment
There was a problem hiding this comment.
As I said in the linked issue, I don't think it makes sense to take this change - we shouldn't attempt to process malformed XCFrameworks that don't contain MachO binaries, and we should ensure the compiler is prepared to enforce the appropriate stability guarantees before allowing packages to vend binary shared libraries via artifact bundle on non-Apple platforms.
| /// Given a platform and the variant, attempt to find an library within the XCFramework that can be used. | ||
| public func findLibrary(platform: String, platformVariant: String = "") -> XCFramework.Library? { | ||
| public func findLibrary(platform: String, platformVariant: String = "", architectures: [String] = []) -> XCFramework.Library? { | ||
| #if canImport(Darwin) |
There was a problem hiding this comment.
You can't use ifdefs here because this will not work for cross-compilation.
I think the actual condition here is whether the platform uses per-arch slices? You can extract the logic in the usesPerArchSlices property you added to LibraryKey and re-use that here.
There was a problem hiding this comment.
OK! ifdef removed, originally I was trying to minimise the changes required and not thread through this argument.
| // that platform default archs to the host and allow a no-variant fallback. | ||
| let platformMatches = libraries.filter { $0.supportedPlatform == platform } | ||
| var effectiveArchs = architectures | ||
| if platform == "linux", effectiveArchs.isEmpty, let hostArch = Architecture.hostStringValue { |
There was a problem hiding this comment.
You can't use Architecture.hostStringValue here because this will not work for cross-compilation. architectures should never be an empty list - for the Linux case you can check that it contains exactly one value, and look up that value.
jakepetroules
left a comment
There was a problem hiding this comment.
Looking better! Commented on a few more small things...
| @_spi(Testing) public static let mergeableMetadataVersion = Version(1, 1) | ||
|
|
||
| static func hasPerArchSlices(_ platform: String) -> Bool { | ||
| return platform == "linux" |
There was a problem hiding this comment.
Please change this to:
| return platform == "linux" | |
| return !["macosx", "iphoneos", "iphonesimulator", "appletvos", "appletvsimulator", "watchos", "watchsimulator", "xros", "xrsimulator", "driverkit"].contains(platform) |
It's a bit verbose, but this is more correct and protects against edge cases if we end up splitting linux into multiple platforms, which @owenv and I have talked about doing to solve some cross compilation issues.
There was a problem hiding this comment.
Adjusted, even more verbose as an existing test was failing.
| lib.supportedPlatform == platform && (lib.platformVariant ?? "") == platformVariant | ||
| }.first | ||
| } | ||
| // Linux ships per-arch XCFramework slices and typically omits SupportedPlatformVariant, so for |
There was a problem hiding this comment.
| // Linux ships per-arch XCFramework slices and typically omits SupportedPlatformVariant, so for | |
| // Non-Apple platforms ship per-arch XCFramework slices and typically omit SupportedPlatformVariant, so for |
| }.first | ||
| } | ||
| // Linux ships per-arch XCFramework slices and typically omits SupportedPlatformVariant, so for | ||
| // that platform require exactly one target arch and allow a no-variant fallback. |
There was a problem hiding this comment.
| // that platform require exactly one target arch and allow a no-variant fallback. | |
| // those platforms, require exactly one target arch and allow a no-variant fallback. |
| // identifier is only used for tracking the offending library | ||
| hasher.combine(platform) | ||
| hasher.combine(platformVariant) | ||
| if usesPerArchSlices { |
There was a problem hiding this comment.
This needs to be unconditionally added to the hash; Hashable and Equatable conformances are not the right place for this sort of business logic.
There was a problem hiding this comment.
Business logic moved to validate().
| guard lhs.platform == rhs.platform, lhs.platformVariant == rhs.platformVariant else { | ||
| return false | ||
| } | ||
| return lhs.usesPerArchSlices ? (lhs.architectures == rhs.architectures) : true |
There was a problem hiding this comment.
This needs to be unconditionally compared; Hashable and Equatable conformances are not the right place for this sort of business logic.
| // Linux ships per-arch XCFramework slices and typically omits SupportedPlatformVariant, so for | ||
| // that platform require exactly one target arch and allow a no-variant fallback. | ||
| guard architectures.count == 1 else { return nil } | ||
| let targetArch = architectures[0] |
There was a problem hiding this comment.
You can combine the .count==1 check with this:
| let targetArch = architectures[0] | |
| guard let targetArch = architectures.only else { return nil } |
3c2fee8 to
18dff13
Compare
| return false | ||
| } | ||
| // SDK names from Info.plist. | ||
| let sdkCanonicalNames = [ |
There was a problem hiding this comment.
Sorry, I forgot these platform names are in terms of the triples, not the Xcode platform names. You shouldn't check both -- just the BuildVersion.Platform code path is sufficient.
|
@swift-ci test |
|
Ubuntu failure https://github.com/swiftlang/swift-build/actions/runs/24925300005/job/73036104365?pr=1310#step:14:43080 doesn't seem to be related to the changes: |
owenv
left a comment
There was a problem hiding this comment.
I guess this isn't doing too much harm
Allow use of
--experimental-xcframeworks-on-linuxflag as per #1250These changes allows us to compile our code base on Linux using
--build-system swiftbuild.