Closed ErikEverson closed 3 months ago
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.
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!
@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. https://github.com/rust-lang/rust-clippy/issues/4573#issuecomment-535521420
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 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.
@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
ErikEverson force-pushed the EE/tvOS_support branch from 80cd835 to e9dc1b9
Thanks! Commit history is looking better. A couple suggestions:
git
dependency temporarily and then bumping the sec-framework dep versions separately it'd be best to do that all in one commit.So the end state would be something like:
cargo fmt
.@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:
iOS targets:
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.
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.
I will start preparing a point release :rocket:
Added support to build tvOS targets so that consumers of rustls-platform-verifier can use it with tvOS devices
Target support added for:
Motivation:
How was this tested: