-
Notifications
You must be signed in to change notification settings - Fork 22
fix ffi for platform-specific uint_fast types for macOS and Android #334
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
Conversation
|
I verified this fix on the Intel Mac only; for Android, it will require some time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I am not sure about the
#[cfg(not(target_arch = "wasm32"))]
pub type c_uint_fast64_t = usize;Because of: https://en.cppreference.com/w/c/types/integer.html
As far as I understand on 32-bit platforms like armv7 because we are using the usize this thing could fail
19509c3 to
7a77539
Compare
I'll take it anyway. This is definitely an improvement, in that it at least separates out the arches a bit to make it easy to edit individual ones. |
|
If this approach doesn't work, another thing we can do is just patch the C code to #define these types as fixed-size ones which have Rust equivalents. We don't need the speed benefit that the Elements integration needs. |
|
Verified via Docker for the Android build |
apoelstra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 7a77539; successfully ran local tests
|
Tagged and published. |
…nd Android 7a77539 Bump simplicity-sys version to 0.6.1 (Kyryl R) 4126e2e fix ffi for platform-specific uint_fast types for macOS and Android (Kyryl R) Pull request description: Fixes: - macOS (x86_64 and arm64): uint_fast16_t is u16, not usize - Android (Bionic libc): uint_fast16_t is u32, not u16 - jet FFI calls failing on Intel Mac and Android Because of the previous setup I have encountered failures the one below: Blockstream/gdk#221 ACKs for top commit: apoelstra: ACK 7a77539; successfully ran local tests Tree-SHA512: e37f218916f086af480baf341756c94f27fc59700e38fe99595743bbae1f5d7dd2fc4b8f9734a189f8249ad857666dcd608abc981a1df9d47f5924e94a3bf01a
|
Should I bump simplicity-lang version now? UPD: bumped, let me know if there is a need to revert or update something |
|
Merged in: 7f42b53 |
Fixes:
Because of the previous setup I have encountered failures the one below:
Blockstream/gdk#221