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

Added tvOS support #78

Merged
merged 2 commits into from
Apr 10, 2024
Merged

Added tvOS support #78

merged 2 commits into from
Apr 10, 2024

Conversation

ErikEverson
Copy link
Contributor

Added support to build tvOS targets so that consumers of rustls-platform-verifier can use it with tvOS devices

Target support added for:

  • x86_64-apple-tvos
  • aarch64-apple-tvos
  • aarch64-apple-tvos-sim

Motivation:

  • We are adding support for tvOS to our library and consuming iOS application that relies on this Rust dependency directly.

How was this tested:

  • cargo test (passed)
  • cargo +nightly build -Z build-std --lib --target x86_64-apple-tvos -v
  • cargo +nightly build -Z build-std --lib --target aarch64-apple-tvos-v
  • cargo +nightly build -Z build-std --lib --target aarch64-apple-tvos-sim -v

@ErikEverson ErikEverson marked this pull request as draft April 8, 2024 14:50
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Nice, thanks! Can you clean up the formatting issue?

@complexspaces
Copy link
Collaborator

Is it possible to add a CI job that at least runs our Clippy checks on the tvOS target? It would be nice if this didn't regress quietly by mistake.

@cpu
Copy link
Member

cpu commented Apr 9, 2024

I think it's part of why this PR is marked a draft, but just to be explicit it would also be great if you could tidy up the commit history when you add the CI coverage. Thank you!

@ErikEverson
Copy link
Contributor Author

ErikEverson commented Apr 9, 2024

@complexspaces I looked into adding a clippy ci job for tvOS
I am not sure if Clippy can easily be run or at all on tier 3 targets. rust-lang/rust-clippy#4573 (comment)

Also I looked into running just Check which is not as good as Clippy but at least something.
I was able to get cargo +nightly -Zbuild-std check --target aarch64-apple-tvos to work.

@ErikEverson ErikEverson marked this pull request as ready for review April 9, 2024 17:17
@complexspaces
Copy link
Collaborator

@ErikEverson Thanks for taking an initial look at that! I checked out this branch locally, and I think that I was able to get Clippy to work on a recent nightly. Could you see if this is reproducible for you as well? I'm on an M1 macOS 14 system.

These are my versions:

cargo +nightly --version
cargo 1.79.0-nightly (0637083df 2024-04-02)

rustc +nightly --version
rustc 1.79.0-nightly (385fa9d84 2024-04-04)

Running cargo clean and then cargo +nightly clippy -Zbuild-std --target aarch64-apple-tvos completes in a few seconds after building the standard library. I tested its functionality by adding #![warn(clippy::as_conversions)] to lib.rs and then removing the #[allow(clippy::as_conversions)] annotation on system_time_to_cfdate. After running it again the warnings I'd expect appeared:

warning: using a potentially dangerous silent `as` conversion
  --> rustls-platform-verifier/src/verification/apple.rs:32:36
   |
32 |     let unix_adjustment = unsafe { kCFAbsoluteTimeIntervalSince1970 as u64 };
   |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: consider using a safe wrapper for this conversion
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#as_conversions
note: the lint level is defined here
  --> rustls-platform-verifier/src/lib.rs:4:9
   |
4  | #![warn(clippy::as_conversions)]
   |         ^^^^^^^^^^^^^^^^^^^^^^

If -Zbuild-std is placed before clippy in the command it fails to work though.

@ErikEverson
Copy link
Contributor Author

ErikEverson commented Apr 9, 2024

@complexspaces Awesome I will see if I get the same on my side. I was putting -Zbuild-std first. Then I will get a CI clippy working

@cpu
Copy link
Member

cpu commented Apr 9, 2024

ErikEverson force-pushed the EE/tvOS_support branch from 80cd835 to e9dc1b9

Thanks! Commit history is looking better. A couple suggestions:

  • I think rather than take the git dependency temporarily and then bumping the sec-framework dep versions separately it'd be best to do that all in one commit.
  • Similarly, the fmt fix should get folded into commit that introduced the change being reformatted.

So the end state would be something like:

  • One commit updating the required deps, adding the new cfg gates, and correctly formatted with cargo fmt.
  • One commit updating CI.

Updated security-framework to 2.10
Added cfg target os for tvOS
@ErikEverson
Copy link
Contributor Author

@cpu @complexspaces I got clippy to work locally and on CI. One quick question should I run clippy against all tvOS targets or just one like iOS currently is?
tvOS targets:

  • aarch64-apple-tvos
  • aarch64-apple-tvos-sim
  • x86_64-apple-tvos

iOS targets:

  • aarch64-apple-ios
  • aarch64-apple-ios-sim
  • x86_64-apple-ios (only one with clippy ran)

@cpu
Copy link
Member

cpu commented Apr 9, 2024

One quick question should I run clippy against all tvOS targets or just one like iOS currently is?

I don't feel strongly. Matching the existing iOS target seems sensible to me.

@complexspaces
Copy link
Collaborator

complexspaces commented Apr 9, 2024

I also don't think this matters too much, so let's just target the one like iOS does. We don't have any architecture or environment specific code in rustls-platform-verifier so it shouldn't make a difference to coverage.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thank you!

Looks good to me, I only had some nits about the CI task to suggest.

@cpu cpu merged commit c77fc70 into rustls:main Apr 10, 2024
17 checks passed
@cpu
Copy link
Member

cpu commented Apr 10, 2024

I will start preparing a point release 🚀

@cpu
Copy link
Member

cpu commented Apr 10, 2024

This was included in v/0.3.1.

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.

None yet

4 participants