Skip to content
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

Add support to watchOS from 4.0 #182

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rin-senpai
Copy link

Description of the change

I changed the files to also run if on watchOS 7.0+

Motivation

So this can be used on watchOS.

Type of change

Not sure what it counts as?

Checklist

  • I have performed a self-review of my code.
  • I have added detailed comments to my code where applicable.
  • I have verified that my change does not break existing code.
  • My PR is based on the latest changes of the main branch (if unsure, please run git pull --rebase upstream main).
  • I am familiar with the Google Style Guide for the language I have coded in.
  • I have read through the Contributing Guide and signed the Contributor License Agreement.

@github-actions github-actions bot added component:swift sdk Issue/PR related to Swift SDK status:awaiting review PR awaiting review from a maintainer labels Jun 28, 2024
@andrewheard
Copy link
Collaborator

Thanks for the PR, @rin-senpai! Before you proceed any further though, I'll need to discuss with my team about whether we want to support an additional platform going forward. I'd also need to verify that our networking code works on a real Apple Watch, rather than the watchOS simulator.

@rin-senpai
Copy link
Author

rin-senpai commented Jun 29, 2024

I've added the platform in the package.swift file (it's v8 because seems like that's needed for some things).

I could probably also lower that v8 to v7 or lower and require 8.0 where needed, and also remove all the watchOS 7.0s that aren't needed to clean things up, so I'll do that later when I have time.

@rin-senpai
Copy link
Author

rin-senpai commented Jun 30, 2024

@andrewheard I've done the changes I mentioned above and there are no issues with building other than this thing which I is confusing me a little:
image
The functions are supported from watchOS 2.0 but I also tried requiring watchOS 8.0 and also higher versions on this and the ThrowingPartsRepresentable function so I'm not sure why it's not in scope assuming it does usually work? Maybe I'm missing something simple...

@andrewheard
Copy link
Collaborator

@andrewheard I've done the changes I mentioned above and there are no issues with building other than this thing which I is confusing me a little: image The functions are supported from watchOS 2.0 but I also tried requiring watchOS 8.0 and also higher versions on this and the ThrowingPartsRepresentable function so I'm not sure why it's not in scope assuming it does usually work? Maybe I'm missing something simple...

Hi @rin-senpai, despite the documentation for CGImageDestinationCreateWithData(_:_:_:_:) stating watchOS 2+, I also saw the same errors as you. If I recall correctly, I was able to resolve the error 'nil' requires a contextual type by changing nil to nil as CFDictionary? but it didn't resolve the other functions not being in scope. I think it would be reasonable to remove this extension from watchOS builds using #if !os(watchOS).

That said, I just want to reiterate from before:

I'll need to discuss with my team about whether we want to support an additional platform going forward. I'd also need to verify that our networking code works on a real Apple Watch, rather than the watchOS simulator.

This issue is one example of the maintenance burden of an additional platform. cc: @ryanwilson

@rin-senpai
Copy link
Author

Yes of course I do understand the maintenance burden, and no rush on making a decision there. I'm just making sure it actually works if the team decides to go through with this, and even if not I'll be maintaining my own fork anyway. I'll look into the aforementioned issue more later, as I do recall watchOS supporting CGImage, but if things don't work out I'll just remove the extension as you have said. Thanks!

@rin-senpai
Copy link
Author

Hey @andrewheard! Finally got around to fixing this and in my latest commit the CGImage build error is fixed and I can confirm it works. I've tested basic things such as image and text generation, and there doesn't seem to be any issues so in the interest of my time I'll probably leave it at that for the moment. If there are any style issues for me to address I'll do so sometime in the next few weeks. I know the team may decide the maintenance burden is too high, but just wanted to update you.

@rin-senpai rin-senpai changed the title Add support to watchOS from 7.0 (or at least fixing build errors) Add support to watchOS from 4.0 Jul 15, 2024
@rin-senpai
Copy link
Author

hey is there an update on this?

@andrewheard
Copy link
Collaborator

Hi @rin-senpai, unfortunately we don't currently have plans to support watchOS in this SDK in the near term. As a potential alternative, I've added basic watchOS support in the Vertex AI in Firebase SDK. The API surface is quite similar and we have a migration guide here: https://firebase.google.com/docs/vertex-ai/migrate-to-vertex-ai?platform=ios

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:swift sdk Issue/PR related to Swift SDK status:awaiting review PR awaiting review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants