sgrif / mysqlclient-sys

Rust bindings for libmysqlclient
Apache License 2.0
37 stars 31 forks source link

Add switch for whether or not use rust bindgen to autogen bindings #37

Closed xiangzhai closed 5 months ago

xiangzhai commented 6 months ago

Hi @weiznich @Mingun

I added switch for whether or not use rust-bindgen to autogen bindings. I also implemented get_libmysql_version_id helper for custom conditional compilation mysql_version_id and mariadb_version_id, so it is able to conditional compilation instead of not accurate target_os to support new and old versions of MySQL:

#[cfg(mysql_version_id = "80300")]
pub type my_bool = bool;
#[cfg(not(mysql_version_id = "80300"))]
pub type my_bool = ::std::os::raw::c_char;

cargo test passed and cargo build for diesel successfully for macOS Sonoma MySQL v8.3 and ArchLinux MariaDB v11.3.2.

Windows do not use rust-bindgen by default, it still use bindings_windows.rs.

Could you review my patch and give some comments please?

Thanks, Leslie Zhai

weiznich commented 6 months ago

I've added a CI configuration a few hours back in https://github.com/sgrif/mysqlclient-sys/pull/38, could you rebase your PR on top of the latest master to have a CI run for this? Also we want to actually test the new feature flag in CI, which means you need to add the buildtime_bindgen feature to this array: https://github.com/sgrif/mysqlclient-sys/blob/master/.github/workflows/ci.yml#L17

xiangzhai commented 6 months ago

Hi @weiznich

Any suggestions about rust-bindgen autogen bindings Werror: https://github.com/sgrif/mysqlclient-sys/actions/runs/8631983836/job/23661691801?pr=37#step:11:81

error: `extern` block uses type `u128`, which is not FFI-safe
    --> /home/runner/work/mysqlclient-sys/mysqlclient-sys/target/debug/build/mysqlclient-sys-8cda08bdd42bfb72/out/bindings.rs:3230:10
     |
3230 |     ) -> u128;
     |          ^^^^ not FFI-safe
     |
     = note: 128-bit integers don't currently have a known stable ABI
     = note: `-D improper-ctypes` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(improper_ctypes)]`

macOS Sonoma rustc 1.77.1 (7cf61ebde 2024-03-27) (Homebrew) and ArchLinux rustc 1.77.1 (7cf61ebde 2024-03-27) (Arch Linux rust 1:1.77.1-1) are not able to reproduce the issue:

$ grep u128 target/debug/build/mysqlclient-sys-92b20af7540e2870/out/bindings.rs |wc -l
0

Thanks, Leslie Zhai

weiznich commented 6 months ago

I think that depends on the mysql/mariadb version that's installed there. Someone needs to check the bindgen configuration if:

xiangzhai commented 6 months ago

Thanks for your hint!

I will install Ubuntu jammy and libmysqlclient-dev 8.0.36-0ubuntu0.22.04.1 in a KVM to reproduce the issue.

xiangzhai commented 6 months ago

Hi @weiznich

Sorry for late response!

I am porting JavaScriptCore virtual machine for LoongArch recently. There is only CLoop interpreter right now. I want to port LLint and Baseline, DFG JIT for performance improvement. It will upstream soon passed testmasm, testapi and jsc-stress regression testcase. So sorry again for the late response.

Thanks, Leslie Zhai

weiznich commented 6 months ago

@xiangzhai To be clear here: You don't need to worry about being late about anything here. It's nice to see you contribution and it's totally fine to go in your own pace on adding the requested features or even stop working on this as you loose interest. There are no obligations here to do anything. It's great to see what you already implemented and that will definitively help fixing the issue, even if someone else finishes the work.

xiangzhai commented 6 months ago

Hi @weiznich

Any suggestions about rust-bindgen autogen bindings Werror: https://github.com/sgrif/mysqlclient-sys/actions/runs/8631983836/job/23661691801?pr=37#step:11:81

error: `extern` block uses type `u128`, which is not FFI-safe
    --> /home/runner/work/mysqlclient-sys/mysqlclient-sys/target/debug/build/mysqlclient-sys-8cda08bdd42bfb72/out/bindings.rs:3230:10
     |
3230 |     ) -> u128;
     |          ^^^^ not FFI-safe
     |
     = note: 128-bit integers don't currently have a known stable ABI
     = note: `-D improper-ctypes` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(improper_ctypes)]`

Fixed :)