Simplify bindings and add SCPreferences + SCNetworkConfiguration #9
Conversation
generate_bindings.sh
Outdated
| --raw-line "use core_foundation_sys::runloop::CFRunLoopRef;" \ | ||
| --raw-line "" \ | ||
| --raw-line "use dispatch_queue_t;" \ | ||
| --raw-line "use std::os::raw::c_void;" \ |
There was a problem hiding this comment.
Use libc::c_void is better ?
See a35a48c#diff-037df056d8dd92f60acd1374dc295111R163
if use libc::c_void , we do not need use mem::transmute.
There was a problem hiding this comment.
You should not have to transmute anyway, a raw pointer can be cast between types anyway. But yes, libc::c_void vs std::os::raw::c_void can still be discussed.
It's a hassle that libc define their own c_void, it's identical to the one in std. I would very much like if libc just did pub use std::os::raw::c_void so they became the same type. I'm usually trying to stick with the std::os::raw module where possible since that can completely eliminate the libc dependency if all one need is the c_* types, which are in std. But yeah, here we will need libc anyway and since core-foundation uses libc it makes sense to be consistent! I'll change it over to libc::c_void!
generate_bindings.sh
Outdated
| --raw-line "use dispatch_queue_t;" \ | ||
| --raw-line "use libc::sockaddr;" \ | ||
| --raw-line "use preferences::SCPreferencesRef;" \ | ||
| --raw-line "use std::os::raw::{c_void, c_char, c_int};" \ |
system-configuration-sys/src/lib.rs
Outdated
|
|
||
| pub mod dynamic_store; | ||
| pub mod network_configuration; | ||
| pub mod preferences; |
There was a problem hiding this comment.
reexport all is better ? since they constant and function name is already have namespace.
pub use dynamic_store::*;
pub use network_configuration::*;
pub use preferences::*;There was a problem hiding this comment.
Since there are sooo many functions and types I think that would make the crate root too messy. I'm also aiming to re-export the modules in the higher level modules, like how core-foundation does. See https://github.com/mullvad/system-configuration-rs/blob/master/system-configuration/src/dynamic_store.rs#L23
|
@faern i have test your ffi bindings with my high-level code, it's fine. |
| @@ -16,6 +16,12 @@ | |||
| #![allow(non_upper_case_globals)] | |||
| #![allow(non_snake_case)] | |||
|
|
|||
| extern crate core_foundation_sys; | |||
| pub extern crate core_foundation_sys; | |||
| pub extern crate libc; | |||
There was a problem hiding this comment.
not reexport libc is better ?
Use case:
Cargo.toml
[package]
name = "my-project"
version = "0.1.0"
[dependencies]
core-foundation-sys = "0.5"
libc = "10.10" # Version Problem
that's maybe have problem if you reexport a diffence libc version,
that's maybe is why env_logger is not reexport log.
but i'm not sure.
There was a problem hiding this comment.
I do re-export it to get rid of version incompatibility problems. I can now use system_configuration_sys::libc::c_void (see https://github.com/mullvad/system-configuration-rs/blob/simplify-bindings/system-configuration/src/dynamic_store.rs#L24) in order to reach this type. Then above crates don't have to depend on libc themselves in order to use system_configuration_sys. This should get rid of incompatibility, because now a crate that uses this can depend on libc = "10.10" and use that freely, but it can also reach the c_void in libc 0.2 because we re-export it. If we don't re-export libc that means a part of our public API is not reachable from our crate and that forces the user of system_configuration_sys to also depend on the exact same version of libc as us, now they don't have to.
There was a problem hiding this comment.
Your example with libc = "10.10" would not work if we don't re-export libc. Because then the c_void available in my-project would be different from the one required in the API of system_configuration_sys (which will always require the c_void from libc 0.2 until this is upgraded).
|
Now I have no more questions. |
|
Reviewed 1 of 6 files at r2. Comments from Reviewable |
|
Reviewed 1 of 6 files at r1. Comments from Reviewable |
|
Reviewed 1 of 6 files at r2. Comments from Reviewable |
|
Reviewed 3 of 6 files at r2, 1 of 1 files at r3. Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 4 unresolved discussions. Comments from Reviewable |
This does not generate the complete bindings. They still need manual work after generation. But it's a good start.
faern
force-pushed
the
simplify-bindings
branch
from
0377219 to
9f566d1
Compare
I generated these complete bindings because of #8 in order to get the complete bindings in one go and automatically generated.
Now
generate_bindings.shis not super strict any longer. It's more to generate initial bindings that can then be manually edited to come up with nice ones.@LuoZijun Could you please check that your high level bindings work on top of these ffi bindings? I did not manually verify the bindings very carefully since they were generated by bindgen.
This change is