Use swift-driver triple #6627

rauhul · 2023-05-30T17:45:46Z

Removes and replaces the custom LLVM Triple type from the Basics module with the implementation from SwiftDriver. This change allows SwiftPM to understand all triples that swift-driver already does and resolves some issues surrounding incorrect triple parsing. For example the Basics.Triple was not able to parse 'armv7em-apple-none-eabihf-macho' where as SwiftDriver.Triple can.

rauhul · 2023-05-30T17:46:05Z

@swift-ci please smoke test

rauhul · 2023-05-30T17:46:10Z

@swift-ci please smoke test windows

compnerd · 2023-05-30T17:51:57Z

Sources/PackageModel/UserToolchain.swift

@@ -382,11 +382,11 @@ public final class UserToolchain: Toolchain {
                            // the merged swiftmodules.  XCTest followed suit.
                            "-I",
                            AbsolutePath(
-                                validating: "usr/lib/swift/windows/\(triple.arch)",
+                                validating: "usr/lib/swift/windows/\(triple.arch?.rawValue ?? "unknown")",


I'm somewhat uncomfortable with this. The unknown here is confusing. Would we be able to replace this with something like <<NULL>> or something that is explicitly bad and obviously so?

should this throw in case it is null?

I think that having it throw would be a much safer option. We need to know the architecture to be able to build.

Resolved this by using archName which is non-optional

compnerd · 2023-05-30T17:52:31Z

Sources/PackageModel/UserToolchain.swift

@@ -716,32 +716,32 @@ public final class UserToolchain: Toolchain {
                // (~5.7), we always had a singular installed SDK.  Prefer the
                // new variant which has an architecture subdirectory in `bin`
                // if available.
-                switch triple.arch {
-                case .x86_64, .x86_64h:
+                switch triple.archName {


Can we ensure that this is in lower case? I'm worried about the stringly matching here.

switching on the string feels like a regression from a type safety pov. is it necessary?

Changed this back to switch on arch, the coverage of the included cases is increased as a result though.

Tests/BasicsTests/TripleTests.swift

MaxDesiatov · 2023-05-30T19:45:40Z

Is there a reason we're not using Driver's Triple type directly instead of copying it here?

rauhul · 2023-05-30T20:00:27Z

Is there a reason we're not using Driver's Triple type directly instead of copying it here?

That's a @neonichu question

rauhul · 2023-05-30T20:18:25Z

@swift-ci please smoke test

MaxDesiatov · 2023-05-30T20:21:18Z

I've previously copied this Triple implementation from TSC since we wanted to deprecate the TSC version, but it seems like the duplication in both TSC and Driver is historic. As SwiftPM already uses Driver as a dependency, I don't see why we can't use Driver's Triple type directly and just remove one in SwiftPM if one from Driver does everything we need.

neonichu · 2023-05-30T20:56:11Z

We don't want to introduce a dependency on Driver in the "data model" portion of SwiftPM since that will cause issues for the Xcode integration.

MaxDesiatov · 2023-05-30T20:57:11Z

@swift-ci test windows

Tests/BasicsTests/TripleTests.swift

rauhul · 2023-05-30T22:24:37Z

@swift-ci please smoke test

rauhul · 2023-05-30T22:24:43Z

@swift-ci please smoke test windows

Sources/Basics/Triple+Basics.swift

tomerd · 2023-06-06T23:06:42Z

Sources/SPMBuildCore/BinaryTarget+Extensions.swift

-            $0.platform == triple.os.asXCFrameworkPlatformString &&
-                $0.architectures.contains(triple.arch.rawValue)
+            $0.platform == triple.os?.asXCFrameworkPlatformString &&
+                $0.architectures.contains(triple.archName)


can architectures be made more type safe instead of string array?

I have absolutely no idea how this XCFramework code works and I'd like to avoid touching it if possible in this PR.

struct XCFrameworkMetadata.Library { let libraryIdentifier: String let libraryPath: String let headersPath: String? let platform: String let architectures: [String] }

tomerd · 2023-06-06T23:09:15Z

Sources/Basics/Triple.swift

-        return nil
-    }
+extension Triple {
+    public static let macOS = try! Self("x86_64-apple-macosx")


does Triple need a non-throwing constructer that takes a static string / ExpressibleByStringLiteral?

Probably, but I want to avoid additional changes API changes in this PR. (The existing triple uses the above API shape)

tomerd · 2023-06-06T23:11:05Z

Sources/Basics/Triple.swift

-        candidates.append((name: "macos", value: .macOS))
-        return candidates.first(where: { string.hasPrefix($0.name) })?.value
+extension Triple {
+    public init(_ description: String) throws {


where/how is this used? worried about the public extension

I added this to match the current Basics.Triple API

tomerd · 2023-06-06T23:11:45Z

Sources/Basics/Triple.swift

 import enum TSCBasic.JSON
 import class TSCBasic.Process
+import struct SwiftDriver.Triple


This file should probably be renamed to Triple+Extension to match style of repo

Sources/Basics/Triple.swift

tomerd · 2023-06-06T23:13:55Z

Package.swift

@@ -152,6 +152,7 @@ let package = Package(
                .product(name: "DequeModule", package: "swift-collections"),
                .product(name: "OrderedCollections", package: "swift-collections"),
                .product(name: "SwiftToolsSupport-auto", package: "swift-tools-support-core"),
+                .product(name: "SwiftDriver", package: "swift-driver"),


hmm I dont love adding this to Basics. I think we are going that only for the extension which are a bit questionable imho. Maybe we can limit the dependency to the modules that actually use it and find a better home (closer to call site) for the extensions

tomerd · 2023-06-12T21:50:53Z

@swift-ci test Windows

WowbaggersLiquidLunch · 2023-06-13T14:33:35Z

Is there a reason we're not using Driver's Triple type directly instead of copying it here?

We don't want to introduce a dependency on Driver in the "data model" portion of SwiftPM since that will cause issues for the Xcode integration.

What if Triple is extracted into its own module?

tomerd · 2023-06-13T16:23:32Z

I think creating a new library that has some of these very basic models (eg semantic version, triple) is called for. TSC was suppose to be that but became a grab bag of "stuff" that is impossible to version properly and as such lost utility, and will be deprecated. while we are working to deprecate TSC, this specific solution seems like an okay compromise

neonichu · 2023-06-13T16:57:00Z

Yah, I would see this as a temporary compromise, but in the future we should have a library with shared types like this one between driver and SwiftPM.

Removes and replaces the custom LLVM Triple type from the Basics module with the implementation from SwiftDrvier. This change allows spm to understand all triples that swift-driver already does and resolves some issues surrounding incorrect triple parsing. For example the Basics.Triple was not able to parse 'armv7em-apple-none-eabihf-macho' where as SwiftDriver.Triple can.

tomerd · 2023-06-20T19:57:17Z

@rauhul is this ready to be merged?

rauhul · 2023-06-20T19:58:11Z

@tomerd I just pushed fixes to address the review feedback, do you mind taking another look?

rauhul · 2023-06-20T20:01:47Z

Likely in a follow on PR Sources/Basics/Triple+Basics.swift should be slimmed down as much as possible, upstreaming necessary content to swift-driver

rauhul · 2023-06-20T20:08:10Z

@swift-ci please smoke test

rauhul · 2023-06-20T20:08:16Z

@swift-ci please test windows

rauhul · 2023-06-20T23:25:36Z

@swift-ci test Windows

rauhul · 2023-06-21T16:09:06Z

@swift-ci test windows

Cherry-pick of #6627 Removes and replaces the custom LLVM Triple type from the Basics module with the implementation from the Swift Driver. This change allows SwiftPM to understand all triples that the Swift Driver already does and resolves some issues surrounding incorrect triple parsing. For example the Basics.Triple was not able to parse 'armv7em-apple-none-eabihf-macho' where as SwiftDriver.Triple can. (cherry picked from commit 4e6501b) ``` # Conflicts: # Sources/Basics/CMakeLists.txt # Sources/Basics/Triple.swift ```

Cherry-pick of #6627 Removes and replaces the custom LLVM Triple type from the Basics module with the implementation from the Swift Driver. This change allows SwiftPM to understand all triples that the Swift Driver already does and resolves some issues surrounding incorrect triple parsing. For example the Basics.Triple was not able to parse 'armv7em-apple-none-eabihf-macho' where as SwiftDriver.Triple can. (cherry picked from commit 4e6501b) ``` # Conflicts: # Sources/Basics/CMakeLists.txt # Sources/Basics/Triple.swift ``` * Fix missing `import struct TSCBasic.AbsolutePath` --------- Co-authored-by: Rauhul Varma <[email protected]>

rauhul requested review from abertelrud, neonichu, tomerd and MaxDesiatov as code owners May 30, 2023 17:45

rauhul requested a review from compnerd May 30, 2023 17:47

compnerd reviewed May 30, 2023

View reviewed changes

MaxDesiatov reviewed May 30, 2023

View reviewed changes

Tests/BasicsTests/TripleTests.swift Show resolved Hide resolved

MaxDesiatov reviewed May 30, 2023

View reviewed changes

Sources/Basics/Triple+Basics.swift Show resolved Hide resolved

tomerd reviewed Jun 6, 2023

View reviewed changes

Sources/Basics/Triple.swift Outdated Show resolved Hide resolved

tomerd reviewed Jun 6, 2023

View reviewed changes

tomerd enabled auto-merge (squash) June 12, 2023 21:51

rauhul added 4 commits June 20, 2023 12:17

Address PR comments, vendor swift-driver code

aa107af

lingering whitespace fixes

2e5f6e8

additional test converage

39372f4

Address review feedback

459d652

rauhul force-pushed the one-triple-to-rule-them-all branch from eb3a1f9 to 459d652 Compare June 20, 2023 19:57

slim down redudant paragraph in vendor readme

93ab6a4

rauhul disabled auto-merge June 20, 2023 20:00

Remove lingering comment

b1932f3

tomerd enabled auto-merge (squash) June 20, 2023 21:44

neonichu approved these changes Jun 21, 2023

View reviewed changes

tomerd merged commit 4e6501b into main Jun 21, 2023

tomerd deleted the one-triple-to-rule-them-all branch June 21, 2023 17:56

aleksproger mentioned this pull request Jul 18, 2023

Add support for iOS triples in swift build #6572

Closed

MaxDesiatov mentioned this pull request Jul 21, 2023

[5.9] Fix triples parsed by Driver not accepted by SwiftPM #6741

Merged

Use swift-driver triple #6627

Use swift-driver triple #6627

Uh oh!

Conversation

rauhul commented May 30, 2023 • edited by MaxDesiatov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rauhul commented May 30, 2023

Uh oh!

rauhul commented May 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaxDesiatov commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rauhul commented May 30, 2023

Uh oh!

rauhul commented May 30, 2023

Uh oh!

MaxDesiatov commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neonichu commented May 30, 2023

Uh oh!

MaxDesiatov commented May 30, 2023

Uh oh!

Uh oh!

rauhul commented May 30, 2023

Uh oh!

rauhul commented May 30, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd commented Jun 12, 2023

Uh oh!

WowbaggersLiquidLunch commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomerd commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neonichu commented Jun 13, 2023

Uh oh!

tomerd commented Jun 20, 2023

rauhul commented May 30, 2023 •

edited by MaxDesiatov

Loading

MaxDesiatov commented May 30, 2023 •

edited

Loading

MaxDesiatov commented May 30, 2023 •

edited

Loading

tomerd Jun 6, 2023 •

edited

Loading

tomerd Jun 6, 2023 •

edited

Loading

WowbaggersLiquidLunch commented Jun 13, 2023 •

edited

Loading

tomerd commented Jun 13, 2023 •

edited

Loading

rauhul commented Jun 20, 2023 •

edited

Loading