Skip to content

Change Triple.isiOS to no longer match tvOS #1955

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

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

Conversation

jakepetroules
Copy link
Contributor

This behavior is confusing and leads to wrong answers in a number of places. All usage sites of isiOS have been audited to ensure they're now correct.

  • unsupportedTargetVariant was showing an error message including -target-variant, which should have only matched iOS and not tvOS
  • getDefaultDwarfVersion checked isiOS in two places, but just so happened to always get the right answer anyways (tvOS's first version was 9, so checking for tvOS < 9 was nonsensical and didn't matter, and iOS and tvOS version numbers have always been aligned, so a < 18 check was right for both)

This behavior is confusing and leads to wrong answers in a number of places. All usage sites of isiOS have been audited to ensure they're now correct.

- unsupportedTargetVariant was showing an error message including -target-variant, which should have only matched iOS and not tvOS
- getDefaultDwarfVersion checked isiOS in two places, but just so happened to always get the right answer anyways (tvOS's first version was 9, so checking for tvOS < 9 was nonsensical and didn't matter, and iOS and tvOS version numbers have always been aligned, so a < 18 check was right for both)
@jakepetroules jakepetroules requested a review from artemcm July 8, 2025 21:21
///
/// This accessor is semi-private; it's typically better to use `version(for:)` or
/// `Triple.FeatureAvailability`.
public var _tvOSVersion: Version {
Copy link
Contributor Author

@jakepetroules jakepetroules Jul 8, 2025

Choose a reason for hiding this comment

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

This part (and changes to _iOSVersion) could use extra scrutiny, I'm not super clear what the true intent is here in terms of what's supposed to be able to map to what.

@jakepetroules
Copy link
Contributor Author

@swift-ci test

@jakepetroules
Copy link
Contributor Author

@swift-ci test windows

var version = self.osVersion
// Default to 5.0 (or 7.0 for arm64).
if version.major == 0 {
version.major = arch == .aarch64 ? 7 : 5
}
return version
case .tvos:
return osVersion
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this become fatalError("conflicting triple info") like .watchos?

case .ios, .tvos:
var version = self.osVersion
if version.major == 0 {
version.major = 9
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment for why we default to 9?

@artemcm
Copy link
Contributor

artemcm commented Jul 10, 2025

I believe we must also update getDefaultDwarfVersion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants