Skip to content

Wire up sdk configure to compilation and fix bug #9229

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Wire up sdk configure to compilation and fix bug #9229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions Sources/PackageModel/SwiftSDKs/SwiftSDK.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,50 @@ public struct SwiftSDK: Equatable {
} else if let swiftSDKSelector {
do {
swiftSDK = try store.selectBundle(matching: swiftSDKSelector, hostTriple: hostTriple)

// Override with user's manual config
do {
let configurationStore = try SwiftSDKConfigurationStore(
hostTimeTriple: hostTriple,
swiftSDKBundleStore: store
)
if swiftSDKSelector != swiftSDK.targetTriple?.tripleString,
let configSDK = try configurationStore.readConfiguration(
sdkID: swiftSDKSelector,
targetTriple: swiftSDK.targetTriple!)
{
swiftSDK = configSDK
} else if let targetTriple = swiftSDK.targetTriple, swiftSDKSelector == targetTriple.tripleString {
var matchedBundles: [String] = []
var multipleConfigMatches: Bool = false
var matchSDK: SwiftSDK?

let bundles = try store.allValidBundles
for artifactID in bundles.sortedArtifactIDs {
bundles.selectSwiftSDK(id: artifactID, hostTriple: hostTriple, targetTriple: targetTriple).map { print($0.targetTriple); matchedBundles.append(artifactID) }
}
for match in matchedBundles {
if let matchSDK {
multipleConfigMatches = true
} else if let readSDK = try configurationStore.readConfiguration(
sdkID: match,
targetTriple: targetTriple)
{
matchSDK = readSDK
}
}
if multipleConfigMatches {
observabilityScope.emit(
warning: """
Multiple SDKs have been configured with this triple \(swiftSDKSelector), so
dropping back to values from original bundle.
Copy link
Member Author

@finagolfin finagolfin Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I had to add all this logic is because one can specify --swift-sdk triple instead of a Swift bundle name, what is termed an artifact ID in the code, and since that triple may be supplied by multiple installed Swift SDKs, the existing logic simply picks the first SDK it finds and warns that it found multiple matching SDKs. Since this local config override then doesn't know which SDK bundle was chosen, as the bundle artifact ID picked is not passed back by these internal methods, this logic has to again search for the bundle chosen and can't use the local config override if multiple bundles matched.

Instead, I would like to make two independent changes to these SDK selection methods:

  1. Only return a result if there is a unique matching SDK bundle ID or triple, error if not. If there is a unique match, pass back both the bundle artifact ID and the SDK, so that this local config override can then be uniquely checked.
  2. Do not allow passing in triples to --swift-sdk, use the --triple flag instead. The use of that flag is currently broken with SDK bundles, as reported in --triple option doesn't take installed Swift SDKs into account #7330 and SwiftPM selects wrong architecture from multi-arch Swift SDK when using --swift-sdk and --triple #9220, so this change would also fix those issues.

@MaxDesiatov, let me know what you think of such proposed fixes, which this pull does not implement, as this pull only uses existing SwiftPM methods.

Copy link
Member Author

@finagolfin finagolfin Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dschaefer2, not getting any response from Max, who implemented this subcommand, doesn't seem like he's active in this repo anymore. Is there anybody else I can discuss these proposed changes with or should I just go ahead and make them?

Copy link
Contributor

@MaxDesiatov MaxDesiatov Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can have a look when this is ready for review and not marked as a work in progress draft.

Copy link
Contributor

@MaxDesiatov MaxDesiatov Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(modulo any other work I and other people are doing, sometimes lead time for a review can be much longer than 2 days because of that)

Copy link
Member Author

@finagolfin finagolfin Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaxDesiatov, as I explained in detail starting two weeks ago, it is only marked as a draft because I think this overall approach isn't great and that we should change more of the underlying and user-facing behavior.

Let me know when you get a chance to think about those deeper changes I've proposed.

Copy link
Contributor

@MaxDesiatov MaxDesiatov Oct 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In accordance with semantic versioning, these changes must be made in a backward-compatible manner with an appropriate deprecation period. I'm fine with these changes in principle, as long as we don't break anything for existing users, notify about the desired migration path, and give them time to migrate before the next major release.

Copy link
Member Author

@finagolfin finagolfin Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I will make the changes, let's see. One key question I asked a couple weeks ago still stands, can I substantially alter a seemingly non-public extension method like selectSwiftSDK(selector:hostTriple:observabilityScope) for both its behavior and declaration, or would I have to create a new method and deprecate the old one?

As for backwards compatibility and migration paths, the current behavior is buggy, non-deterministic, and incomplete for these two features, so pretty sure that making it strict and work right won't be "compatible" 😉, but we can try for the parts that were working.

Copy link
Contributor

@MaxDesiatov MaxDesiatov Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC none of the Swift SDK code in SwiftPM is public, so that's not an API and hence not something that has to follow SemVer. I'm referring to CLI commands, and their arguments, options, and flags.

"""
)
} else if let matchSDK {
swiftSDK = matchSDK
}
}
} catch { /* do nothing to override if a custom SDK config is not found */ }
} catch {
// If a user-installed bundle for the selector doesn't exist, check if the
// selector is recognized as a default SDK.
Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftSDKCommand/Configuration/SetConfiguration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ struct SetConfiguration: ConfigurationSubcommand {
}

if let swiftStaticResourcesPath {
configuration.swiftResourcesPath =
configuration.swiftStaticResourcesPath =
try AbsolutePath(validating: swiftStaticResourcesPath, relativeTo: currentWorkingDirectory)
updatedProperties.append(CodingKeys.swiftStaticResourcesPath.stringValue)
}
Expand Down