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 tvOS and watchOS support, identical to iOS support #317

Merged
merged 2 commits into from
Oct 25, 2022

Conversation

thomcc
Copy link
Contributor

@thomcc thomcc commented Oct 24, 2022

This adds tvOS and watchOS support. Support in the Rust stdlib in watchOS is recent, and tvOS is in progress.

These OSes all can use the same interface from Security.framework. Ideally they'd use getrandom, but sadly they currently cannot. I've renamed ios.rs to apple-other.rs as a result of this, but there are no other changes.

@josephlr
Copy link
Member

Given that these targets are not super well supported currently, it's fine that we don't run the tests in CI. However, we should do something similar to our build and link tests for Apple Arm64. You should just be able to add the targets to the tests.yml

@thomcc
Copy link
Contributor Author

thomcc commented Oct 25, 2022

Given that these targets are not super well supported currently, it's fine that we don't run the tests in CI. However, we should do something similar to our build and link tests for Apple Arm64. You should just be able to add the targets to the tests.yml

Hm, cargo test --no-run --target=arm64_32-apple-watchos -Zbuild-std --features=std fails with a pretty obscure linker error. I guess we don't pass things to the linker correctly for arm64_32 (I mostly am making this PR for tvOS, but figured I'd add watchOS while I was here).

For tvos it's not currently supported in libstd, so the best we could do is cargo check. I'll follow up here later.

@josephlr
Copy link
Member

josephlr commented Oct 25, 2022

Hm, cargo test --no-run --target=arm64_32-apple-watchos -Zbuild-std --features=std fails with a pretty obscure linker error. I guess we don't pass things to the linker correctly for arm64_32.

Does this issue occur of the other watchOS targets (i.e. {aarch64,x86_64}-apple-watchos-sim or armv7k-apple-watchos)? We really only need to check one of them in our CI. I would check myself, but I don't have a mac.

If you can get one of them working, add it here.

(I mostly am making this PR for tvOS, but figured I'd add watchOS while I was here).

Makes sense. If linking doesn't work, just add a comment and put the watchOS target with the tvOS target for now.

For tvos it's not currently supported in libstd, so the best we could do is cargo check. I'll follow up here later.

This makes sense, can you add aarch64-apple-tvos to the list of no_std targets here.

@thomcc
Copy link
Contributor Author

thomcc commented Oct 25, 2022

Does this issue occur of the other watchOS targets (i.e. {aarch64,x86_64}-apple-watchos-sim or armv7k-apple-watchos)? We really only need to check one of them in our CI. I would check myself, but I don't have a mac.

Good call, the simulator targets work, which actually helps point out what this issue probably is. Thanks.

Some notes:

  • The watchos-sim target is not distributed via rustup, so I had to just -Zbuild-std it. Let me know if you want me to move it elsewhere or something.
  • I did have to bump your libc version to 0.2.136 for tvOS, to get Add support for tvOS rust-lang/libc#2958.

@josephlr josephlr self-assigned this Oct 25, 2022
Copy link
Member

@josephlr josephlr left a comment

Choose a reason for hiding this comment

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

Other than some nits for the CI, this looks reasonable.

Bumping the libc version is fine. For reference rust-lang/libc@bf618c9 is the commit we need.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
@thomcc
Copy link
Contributor Author

thomcc commented Oct 25, 2022

Thanks, is this more what you meant?

@thomcc
Copy link
Contributor Author

thomcc commented Oct 25, 2022

Ah, -Zbuild-std requires nightly, do you want me to move it to another job, or would you rather make that one use nightly?

@josephlr
Copy link
Member

requires nightly, do you want me to move it to another job, or would you r

Having that one use nightly is fine, ideally it would be in its own job, but macOS runners are slow and constrained on Github Actions, so reducing the jobs generally makes things better.

@thomcc
Copy link
Contributor Author

thomcc commented Oct 25, 2022

I've updated that job to nightly.

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