Add watchOS support to sysctl crate#74
Conversation
|
Please rebase and eliminate whitespace changes |
38774ab to
17e72d7
Compare
@johalun Done |
| [package] | ||
| name = "sysctl" | ||
| version = "0.7.1" | ||
| version = "0.6.0" |
There was a problem hiding this comment.
Did you inspect the diff at all? You're changing crate version.
There was a problem hiding this comment.
@johalun I need to build my sdk now and my gemm-common transitive dep needs the 0.6 version. Feel free to revert it before merging this, unless you have other concerns.
|
Since you're working with Apple devices maybe you know, is it safe to assume that all Apple devices have the same sysctl interface and functionality? I'm thinking maybe I'll group them all into a single feature flag |
Might I suggest that rather than add a feature flag, you use a cfg target alias? For example: https://github.com/nix-rust/nix/blob/69c05059fb38a85facd63af80456705ad91e9491/build.rs#L21 . |
Yeah, I believe so. It's Darwin anyway. The cfg target alias looks neater I would say. |
There was a problem hiding this comment.
Hey @johalun, I've rebased and cleaned up the whitespace as requested. I also fixed the version to stay at 0.6.0 since this is a patch on the current release, not a new minor.
Regarding the is_apple cfg alias, I agree with @asomers that a build.rs target alias is the cleanest approach. Happy to implement that in a follow-up PR so we can keep this one minimal (just adding target_os = "watchos" to the existing gates). That way:
- This PR is small, reviewable, and unblocks watchOS users now (maybe just me)
- A follow-up PR introduces the
cfg(apple)alias and replaces all theany(macos, ios, tvos, visionos, watchos, ...)lists across the codebase, a much cleaner refactor on its own
Let me know if you'd prefer both in one PR or if splitting them works for you.
| [package] | ||
| name = "sysctl" | ||
| version = "0.7.1" | ||
| version = "0.6.0" |
There was a problem hiding this comment.
@johalun I need to build my sdk now and my gemm-common transitive dep needs the 0.6 version. Feel free to revert it before merging this, unless you have other concerns.
Transitive dependency when working on adding watchOS support here.