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

watchOS Support #40

Open
SlaunchaMan opened this Issue Sep 8, 2015 · 13 comments

Comments

Projects
None yet
4 participants
@SlaunchaMan
Contributor

SlaunchaMan commented Sep 8, 2015

If it does not currently, it would be nice to use this library on watchOS 2 (and update the Podspec file to declare this support). I’m happy to help do this, just putting this here to see if anyone else is already working on this.

@dfed

This comment has been minimized.

Show comment
Hide comment
@dfed

dfed Sep 8, 2015

Collaborator

No one is currently working on Watch support (though I might get to it in the next couple months if no one else does). We'd welcome a PR, assuming that you have signed our CLA.

Here's what needs to be done to get watch support:

  • Create a shared Watch scheme in Xcode
  • Update the Podspec to include Watch
  • Create a sample app with a shared Xcode scheme that tests each Valet type on watch – ValetWatchTest. Use ValetTouchIDTest as a guide, though because there is no unit test target on watch ValetWatchTest will need significantly more features (for example, the ability to test migration)
  • Test to see if VALSecureEnclaveValet will work on the watch (my guess is it won't). If it won't, we'll need to compile it out on watch the same way we do on other unsupported OS versions
  • Test to see if VALSyncronizableValet will work on the watch. Is there a way to opt into iCloud keychain on the watch? If not, does VALSyncronizableValet sync anywhere? It'd be awesome if it syncs to the phone (and other iCloud keychain devices)
  • Revisit VAL_IOS_8_OR_LATER and VAL_IOS_9_OR_LATER to make sure they target iOS and not the Watch. We can use #if TARGET_OS_WATCH to compile things specifically into the Watch target. Unfortunately TARGET_OS_IPHONE is set to 1 on Watch. The only way to select iOS and not Watch is to select TARGET_OS_IOS, which of course only exists in the Xcode 7 SDK, which means we'll need to do some work to make any macros targeting iOS and not Watch backwards compatible

I know that's a lot, but it takes a lot to vet Valet for a new target – you wouldn't believe the fun bugs we found when getting Valet working on the Mac (there are wonderfully subtle differences between how Keychain works on the Mac and iOS), and at least we had unit tests to help us discover the issues. We're certainly around to help with this – happy to give comments or answer questions while you're developing, or even hop on your fork and help out a little. We just won't have time to tackle this project on our own in the short-term.

Collaborator

dfed commented Sep 8, 2015

No one is currently working on Watch support (though I might get to it in the next couple months if no one else does). We'd welcome a PR, assuming that you have signed our CLA.

Here's what needs to be done to get watch support:

  • Create a shared Watch scheme in Xcode
  • Update the Podspec to include Watch
  • Create a sample app with a shared Xcode scheme that tests each Valet type on watch – ValetWatchTest. Use ValetTouchIDTest as a guide, though because there is no unit test target on watch ValetWatchTest will need significantly more features (for example, the ability to test migration)
  • Test to see if VALSecureEnclaveValet will work on the watch (my guess is it won't). If it won't, we'll need to compile it out on watch the same way we do on other unsupported OS versions
  • Test to see if VALSyncronizableValet will work on the watch. Is there a way to opt into iCloud keychain on the watch? If not, does VALSyncronizableValet sync anywhere? It'd be awesome if it syncs to the phone (and other iCloud keychain devices)
  • Revisit VAL_IOS_8_OR_LATER and VAL_IOS_9_OR_LATER to make sure they target iOS and not the Watch. We can use #if TARGET_OS_WATCH to compile things specifically into the Watch target. Unfortunately TARGET_OS_IPHONE is set to 1 on Watch. The only way to select iOS and not Watch is to select TARGET_OS_IOS, which of course only exists in the Xcode 7 SDK, which means we'll need to do some work to make any macros targeting iOS and not Watch backwards compatible

I know that's a lot, but it takes a lot to vet Valet for a new target – you wouldn't believe the fun bugs we found when getting Valet working on the Mac (there are wonderfully subtle differences between how Keychain works on the Mac and iOS), and at least we had unit tests to help us discover the issues. We're certainly around to help with this – happy to give comments or answer questions while you're developing, or even hop on your fork and help out a little. We just won't have time to tackle this project on our own in the short-term.

@SlaunchaMan

This comment has been minimized.

Show comment
Hide comment
@SlaunchaMan

SlaunchaMan Sep 8, 2015

Contributor

That sounds like something I would like to help with. I’m a little busy with a watchOS book right now, but that should ship in mid-October, at which point I’ll have plenty of time to start tackling the list. I’m looking into the CLA. 👍

Contributor

SlaunchaMan commented Sep 8, 2015

That sounds like something I would like to help with. I’m a little busy with a watchOS book right now, but that should ship in mid-October, at which point I’ll have plenty of time to start tackling the list. I’m looking into the CLA. 👍

@dfed

This comment has been minimized.

Show comment
Hide comment
@dfed

dfed Sep 9, 2015

Collaborator

Sounds great @SlaunchaMan. Thanks for offering to help! We'll be sure to message you if we start working on it first.

Collaborator

dfed commented Sep 9, 2015

Sounds great @SlaunchaMan. Thanks for offering to help! We'll be sure to message you if we start working on it first.

@dfed

This comment has been minimized.

Show comment
Hide comment
@dfed

dfed Oct 26, 2015

Collaborator

According to @digoreis, VALSecureEnclaveValet works on the watch!

Collaborator

dfed commented Oct 26, 2015

According to @digoreis, VALSecureEnclaveValet works on the watch!

@nemesis

This comment has been minimized.

Show comment
Hide comment
@nemesis

nemesis Jan 15, 2016

Hey guys,

Since Valet is supported on watchos, maybe the .podspec should be modified to include watchos support?

nemesis commented Jan 15, 2016

Hey guys,

Since Valet is supported on watchos, maybe the .podspec should be modified to include watchos support?

@dfed

This comment has been minimized.

Show comment
Hide comment
@dfed

dfed Jan 15, 2016

Collaborator

@nemesis Valet isn't supported on watchOS. Some things may work on watchOS... but we don't have any tests (regression or otherwise) proving it yet. I'd prefer not to release Valet for a platform that we haven't thoroughly vetted/tested.

Collaborator

dfed commented Jan 15, 2016

@nemesis Valet isn't supported on watchOS. Some things may work on watchOS... but we don't have any tests (regression or otherwise) proving it yet. I'd prefer not to release Valet for a platform that we haven't thoroughly vetted/tested.

@nemesis

This comment has been minimized.

Show comment
Hide comment
@nemesis

nemesis Jan 18, 2016

@dfed I get that, but to build something on watchOS that's linked with Valet (not necessarily using it on watchOS, just linking against it) will be impossible. In my case, there are source files that I want to import on watchOS but I can't since they are linked with Valet in the main iOS target. Is there a resolution to that? I know it's not directly related to Valet, so I would appreciate any help on this one.

nemesis commented Jan 18, 2016

@dfed I get that, but to build something on watchOS that's linked with Valet (not necessarily using it on watchOS, just linking against it) will be impossible. In my case, there are source files that I want to import on watchOS but I can't since they are linked with Valet in the main iOS target. Is there a resolution to that? I know it's not directly related to Valet, so I would appreciate any help on this one.

@dfed

This comment has been minimized.

Show comment
Hide comment
@dfed

dfed Jan 19, 2016

Collaborator

@nemesis you shouldn't be linking Valet on watchOS, period. If you want to share a file between your iOS and watchOS targets, and you want the file on iOS to talk to Valet and on watchOS to do something else, you'll need to use compiler flags to compile out your reference to Valet on watchOS. That said, I really don't recommend that approach. Better to make your classes smaller and more single-purposed (use delegates to break up your secrets implementation from your secrets API), so that you don't need to share a class like that across library/target boundaries.

That said, this really isn't the right forum for this kind of help. StackOverflow is a better place for this kind of question. Let's keep this Issue tracking implementing a tested Valet on WatchOS. All other issues should be new Issues.

Collaborator

dfed commented Jan 19, 2016

@nemesis you shouldn't be linking Valet on watchOS, period. If you want to share a file between your iOS and watchOS targets, and you want the file on iOS to talk to Valet and on watchOS to do something else, you'll need to use compiler flags to compile out your reference to Valet on watchOS. That said, I really don't recommend that approach. Better to make your classes smaller and more single-purposed (use delegates to break up your secrets implementation from your secrets API), so that you don't need to share a class like that across library/target boundaries.

That said, this really isn't the right forum for this kind of help. StackOverflow is a better place for this kind of question. Let's keep this Issue tracking implementing a tested Valet on WatchOS. All other issues should be new Issues.

@dfed

This comment has been minimized.

Show comment
Hide comment
@dfed

dfed Mar 3, 2018

Collaborator

I'm actively working on this now. Since the migration to Swift the above may get a teensy bit easier.

Collaborator

dfed commented Mar 3, 2018

I'm actively working on this now. Since the migration to Swift the above may get a teensy bit easier.

@dfed dfed referenced a pull request that will close this issue Mar 4, 2018

Open

Add watchOS support #127

@dfed

This comment has been minimized.

Show comment
Hide comment
@dfed

dfed Mar 4, 2018

Collaborator

When #127 is merged, this will be completed.

Collaborator

dfed commented Mar 4, 2018

When #127 is merged, this will be completed.

@dfed dfed removed the help wanted label Mar 5, 2018

@jpv123

This comment has been minimized.

Show comment
Hide comment
@jpv123

jpv123 Mar 8, 2018

@dfed Any news on this? I'm really interested on this PR

jpv123 commented Mar 8, 2018

@dfed Any news on this? I'm really interested on this PR

@dfed

This comment has been minimized.

Show comment
Hide comment
@dfed

dfed Mar 8, 2018

Collaborator

@jpv123 I’m waiting for a code review or two 🙂. Glad you’re interested!

Collaborator

dfed commented Mar 8, 2018

@jpv123 I’m waiting for a code review or two 🙂. Glad you’re interested!

@jpv123

This comment has been minimized.

Show comment
Hide comment
@jpv123

jpv123 Mar 9, 2018

Thanks @dfed, I hope the code reviews come soon

jpv123 commented Mar 9, 2018

Thanks @dfed, I hope the code reviews come soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment