-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
base: main
Are you sure you want to change the base?
Conversation
| observabilityScope.emit( | ||
| warning: """ | ||
| Multiple SDKs have been configured with this triple \(swiftSDKSelector), so | ||
| dropping back to values from original bundle. |
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.
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:
- 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.
- Do not allow passing in triples to
--swift-sdk, use the--tripleflag instead. The use of that flag is currently broken with SDK bundles, as reported in--tripleoption 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.
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.
@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?
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.
I can have a look when this is ready for review and not marked as a work in progress draft.
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.
(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)
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.
@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.
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.
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.
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.
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.
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.
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.
This will require more discussion, but this attempts to fix #8958, will have more info soon.