prebuilt bindings for win + mac + linux #40
Conversation
step 1: win bindings step 2: linux + cfg + feature only check for clang when building bindings split up the OS specializations just use inverted logic macos
There was a problem hiding this comment.
How much of this is direct bindgen output vs hand-edited? Would it make sense to include a README.md under bindings with the command to reproduce it, or would the expectation be that future changes to the soapysdr header would require manual merging with this tweaked output?
It seems like everything in the platform-specific files are either the same on all platforms, a function that can't be validly called from Rust at present, and junk from system headers that bindgen decided to include for some reason but shouldn't be part of the soapysdr-sys public API. So it seems like we might be able to get away without the platform-specific code here.
| #[doc = "! Possible data types for argument info"] | ||
| pub type SoapySDRArgInfoType = ::std::os::raw::c_uint; | ||
|
|
||
| #[doc = " The available priority levels for log messages.\n\n The default log level threshold is SOAPY_SDR_INFO.\n Log messages with lower priorities are dropped.\n\n The default threshold can be set via the\n SOAPY_SDR_LOG_LEVEL environment variable.\n Set SOAPY_SDR_LOG_LEVEL to the string value:\n \"WARNING\", \"ERROR\", \"DEBUG\", etc...\n or set it to the equivalent integer value."] | ||
| pub type SoapySDRLogLevel = ::std::os::raw::c_uint; | ||
|
|
There was a problem hiding this comment.
These are the same on all platforms?
There was a problem hiding this comment.
On windows they are signed, oddly (c_int vs c_uint). If they're the same width, then it seems it would generally be the same with a potential footgun that might eventually be triggered. I'm not sure it's worth preserving this discrepancy but I figured I'd at least surface it in this PR.
| ::std::mem::size_of::<__crt_locale_pointers>(), | ||
| 16usize, | ||
| concat!("Size of: ", stringify!(__crt_locale_pointers)) | ||
| ); | ||
| assert_eq!( | ||
| ::std::mem::align_of::<__crt_locale_pointers>(), | ||
| 8usize, | ||
| concat!("Alignment of ", stringify!(__crt_locale_pointers)) | ||
| ); | ||
| assert_eq!( | ||
| unsafe { ::std::ptr::addr_of!((*ptr).locinfo) as usize - ptr as usize }, | ||
| 0usize, | ||
| concat!( | ||
| "Offset of field: ", | ||
| stringify!(__crt_locale_pointers), | ||
| "::", | ||
| stringify!(locinfo) | ||
| ) | ||
| ); | ||
| assert_eq!( | ||
| unsafe { ::std::ptr::addr_of!((*ptr).mbcinfo) as usize - ptr as usize }, | ||
| 8usize, | ||
| concat!( | ||
| "Offset of field: ", | ||
| stringify!(__crt_locale_pointers), | ||
| "::", | ||
| stringify!(mbcinfo) | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| pub type _locale_t = *mut __crt_locale_pointers; | ||
|
|
||
| #[repr(C)] | ||
| #[derive(Debug, Copy, Clone)] | ||
| pub struct _Mbstatet { | ||
| pub _Wchar: ::std::os::raw::c_ulong, | ||
| pub _Byte: ::std::os::raw::c_ushort, | ||
| pub _State: ::std::os::raw::c_ushort, | ||
| } | ||
|
|
||
| #[test] | ||
| fn bindgen_test_layout__Mbstatet() { | ||
| const UNINIT: ::std::mem::MaybeUninit<_Mbstatet> = ::std::mem::MaybeUninit::uninit(); | ||
| let ptr = UNINIT.as_ptr(); | ||
| assert_eq!( | ||
| ::std::mem::size_of::<_Mbstatet>(), | ||
| 8usize, | ||
| concat!("Size of: ", stringify!(_Mbstatet)) | ||
| ); | ||
| assert_eq!( | ||
| ::std::mem::align_of::<_Mbstatet>(), | ||
| 4usize, | ||
| concat!("Alignment of ", stringify!(_Mbstatet)) | ||
| ); | ||
| assert_eq!( | ||
| unsafe { ::std::ptr::addr_of!((*ptr)._Wchar) as usize - ptr as usize }, | ||
| 0usize, | ||
| concat!( | ||
| "Offset of field: ", | ||
| stringify!(_Mbstatet), | ||
| "::", | ||
| stringify!(_Wchar) | ||
| ) | ||
| ); | ||
| assert_eq!( | ||
| unsafe { ::std::ptr::addr_of!((*ptr)._Byte) as usize - ptr as usize }, | ||
| 4usize, | ||
| concat!( | ||
| "Offset of field: ", | ||
| stringify!(_Mbstatet), | ||
| "::", | ||
| stringify!(_Byte) | ||
| ) | ||
| ); | ||
| assert_eq!( | ||
| unsafe { ::std::ptr::addr_of!((*ptr)._State) as usize - ptr as usize }, | ||
| 6usize, | ||
| concat!( | ||
| "Offset of field: ", | ||
| stringify!(_Mbstatet), | ||
| "::", | ||
| stringify!(_State) | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| pub type mbstate_t = _Mbstatet; | ||
|
|
||
| #[repr(C)] | ||
| #[derive(Debug, Copy, Clone)] | ||
| pub struct __crt_locale_data { | ||
| pub _address: u8, | ||
| } | ||
|
|
||
| #[repr(C)] | ||
| #[derive(Debug, Copy, Clone)] | ||
| pub struct __crt_multibyte_data { | ||
| pub _address: u8, | ||
| } |
There was a problem hiding this comment.
Seems like these shouldn't be exposed by this library? Just stuff from system headers.
There was a problem hiding this comment.
Yeah, probably so. I wanted to preserve any tests that bindgen created (just to check something if we can) and I believe these are a dependency, but there's probably a smarter way to test the bindings.
| assert_eq!( | ||
| ::std::mem::size_of::<__va_list_tag>(), | ||
| 24usize, | ||
| concat!("Size of: ", stringify!(__va_list_tag)) | ||
| ); | ||
| assert_eq!( | ||
| ::std::mem::align_of::<__va_list_tag>(), | ||
| 8usize, | ||
| concat!("Alignment of ", stringify!(__va_list_tag)) | ||
| ); | ||
| assert_eq!( | ||
| unsafe { ::std::ptr::addr_of!((*ptr).gp_offset) as usize - ptr as usize }, | ||
| 0usize, | ||
| concat!( | ||
| "Offset of field: ", | ||
| stringify!(__va_list_tag), | ||
| "::", | ||
| stringify!(gp_offset) | ||
| ) | ||
| ); | ||
| assert_eq!( | ||
| unsafe { ::std::ptr::addr_of!((*ptr).fp_offset) as usize - ptr as usize }, | ||
| 4usize, | ||
| concat!( | ||
| "Offset of field: ", | ||
| stringify!(__va_list_tag), | ||
| "::", | ||
| stringify!(fp_offset) | ||
| ) | ||
| ); | ||
| assert_eq!( | ||
| unsafe { ::std::ptr::addr_of!((*ptr).overflow_arg_area) as usize - ptr as usize }, | ||
| 8usize, | ||
| concat!( | ||
| "Offset of field: ", | ||
| stringify!(__va_list_tag), | ||
| "::", | ||
| stringify!(overflow_arg_area) | ||
| ) | ||
| ); | ||
| assert_eq!( | ||
| unsafe { ::std::ptr::addr_of!((*ptr).reg_save_area) as usize - ptr as usize }, | ||
| 16usize, | ||
| concat!( | ||
| "Offset of field: ", | ||
| stringify!(__va_list_tag), | ||
| "::", | ||
| stringify!(reg_save_area) | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| // preserve any bindgen tests for all platforms out of an abundance of caution | ||
| // these get run as part of `cargo test` | ||
|
|
||
| #[repr(C)] | ||
| #[repr(align(16))] | ||
| #[derive(Debug, Copy, Clone)] | ||
| pub struct max_align_t { | ||
| pub __clang_max_align_nonce1: ::std::os::raw::c_longlong, | ||
| pub __bindgen_padding_0: u64, | ||
| pub __clang_max_align_nonce2: u128, | ||
| } | ||
|
|
||
| #[test] | ||
| fn bindgen_test_layout_max_align_t() { | ||
| const UNINIT: ::std::mem::MaybeUninit<max_align_t> = ::std::mem::MaybeUninit::uninit(); | ||
| let ptr = UNINIT.as_ptr(); | ||
| assert_eq!( | ||
| ::std::mem::size_of::<max_align_t>(), | ||
| 32usize, | ||
| concat!("Size of: ", stringify!(max_align_t)) | ||
| ); | ||
| assert_eq!( | ||
| ::std::mem::align_of::<max_align_t>(), | ||
| 16usize, | ||
| concat!("Alignment of ", stringify!(max_align_t)) | ||
| ); | ||
| assert_eq!( | ||
| unsafe { ::std::ptr::addr_of!((*ptr).__clang_max_align_nonce1) as usize - ptr as usize }, | ||
| 0usize, | ||
| concat!( | ||
| "Offset of field: ", | ||
| stringify!(max_align_t), | ||
| "::", | ||
| stringify!(__clang_max_align_nonce1) | ||
| ) | ||
| ); | ||
| assert_eq!( | ||
| unsafe { ::std::ptr::addr_of!((*ptr).__clang_max_align_nonce2) as usize - ptr as usize }, | ||
| 16usize, | ||
| concat!( | ||
| "Offset of field: ", | ||
| stringify!(max_align_t), | ||
| "::", | ||
| stringify!(__clang_max_align_nonce2) | ||
| ) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think a bindgen-translated definition of va_list is going to be usable from Rust. There's an unstable type and a crate but both appear to only support a Rust function receiving a va_list from C to Rust, not the other way around.
So SoapySDR_vlogf should probably just be removed pending support for that.
| let mut cc_builder = cc::Build::new(); | ||
| cc_builder.file("wrapper.c"); |
There was a problem hiding this comment.
You're correct that this is no longer necessary if dropping support for 0.7, but needs further cleanup:
- Delete the wrapper.c file since it is no longer built.
- Remove the
_rust_wrapper_SoapySDRDevice_setupStreamfromwrapper.h - Un-blocklist
SoapySDRDevice_setupStreambelow - Remove the re-export in
lib.rs - Update the pre-generated bindings accordingly
There was a problem hiding this comment.
I'm happy to do if you want to drop support, but I believe the bindings offered here would still work for 0.7. I generally tend to err on the side of more compatibility, so I'm happy to do any extra steps to preserve 0.7 if you think any are needed (and/or worth it).
I had some confusion about this initially, but now that I understand that the Rust portion is offered on top of wrapper.c + soapy, I feel that the compat is not actually very difficult. It seems like as long as wrapper.c makes up for any differences, then the bindgen portion doesn't really care what's underneath.
Yeah, I can add some notes. Broadly this is direct output but I removed the compiler junk where I could. All platforms capture some junk. Interestingly, the docstrings varied from platform to platform. I'm open to ideas re: future changes. I suspect that probably you/we would always want to remove the unused/unrelated bits. If the future changes are just adding a couple new functions, then probably I'd advocate for running bindgen somewhere and manually updating the file here from the diff in the output. If it were more major than that (say, a complete rearchitecting of the API), then it probably makes sense to start over. Comparing notes with rusqlite, I feel they have probably done some manual editing to capture just the parts they care about. If the file is checked in, there's probably some value in cleaning it up a bit to make it easier to read. |
Following our discussion in #38 , I created a new feature
build_bindingswhich is not enabled by default. Enabling the feature triggers the previous behavior of running bindgen. Leaving it off causes a vendored binding to be used. Even if the feature is not enabled, bindings will still be built on OS other than win/mac/linux. We might want to consider renaming the feature (always_build_bindings?).I have only tried this against soapysdr ABI 0.8 so far. I'm actually a bit curious how changes in bindings.rs across different ABIs could even work since this would change the interface presented to the Rust part of the codebase.
Looking at the wrapper, if there is an issue with 0.7, I'd guess it would be a problem withedit: I see that bindgen also exposes the names of functions in the wrapper, so this makes sense to me, and I feel more confident about how older ABIs might workSoapySDR_freepossibly being redeclared. I'm not sure currently (on master) how SoapySDR_free is declared for 0.7 users.Across the different operating systems on ABI 0.8, there ended up not being too much difference. I preserved any tests created by bindgen, even if they seemed to test unused functionality.