-
Notifications
You must be signed in to change notification settings - Fork 11
Deployment targets supported by Xcode 16 #25
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
|
You should be able to use @available and #available instead of moving things to another package. For the methods that use concurrency things that ar not available yet. And the tests: |
|
@naftaly updated. Example is also protected by |
| .macOS(.v10_13), | ||
| .iOS(.v12), | ||
| .tvOS(.v12), | ||
| .watchOS(.v4), |
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.
| .watchOS(.v4), | |
| .watchOS(.v6), |
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 checked Xcode 16 support and it supports watchOS 4+. And seems as everything builds fine for watchOS 4. Do we want to support 6+ only?
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.
That's just what was in the proposal we all agreed upon. I think we should stick to the proposal unless there's a reason to diverge.
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 simply checked what Xcode really supports. And seems as if we don't have any dependencies on watchOS 6. I can change to 6 if needed
|
|
||
| let tracer = OpenTelemetry.instance.tracerProvider.get(instrumentationName: "ConcurrencyContext", instrumentationVersion: "semver:0.1.0") | ||
|
|
||
| @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) |
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.
should you move this function into #available?
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.
This is an extension. It can be only on the top level in the file, so I had to move it outside and add @available to it
|
This looks great, thanks @ypopovych . Do you think the added |
|
TaskLocalContextManager is public. I think both of them should be public. I will test the main repo |
|
Tested in the opentelemetry-swift and some example app for watchOS 4 - works fine in iOS 12 and watchOS 4, main repo also builds fine (but needs Xcode 16.3+, grpc bumped swift version to 6.1). |
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.
Hey @ypopovych ! Thanks for the PR! I've been testing it out and everything seems to be working just fine.
Would you be able to update the .podspec files as well? That way, we stay consistent in the platforms and versions we support across the different package managers
Done. Also added macOS deployment target to them, it was missing |
Updated project to support all of the deployment targets supported by Xcode 16.
Had to move example to own swift package, because it can't be built for old targets.
Resolves #23