mysqlclient-sys icon indicating copy to clipboard operation
mysqlclient-sys copied to clipboard

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

Open xiangzhai opened this issue 1 year ago • 7 comments

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

xiangzhai avatar Apr 08 '24 12:04 xiangzhai

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

weiznich avatar Apr 10 '24 12:04 weiznich

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

xiangzhai avatar Apr 10 '24 13:04 xiangzhai

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

  • There is a way to disable this behavior
  • Or if there is some option to filter out these fields/structs/… (if they are not used by diesel)

weiznich avatar Apr 10 '24 14:04 weiznich

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 avatar Apr 10 '24 14:04 xiangzhai

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

xiangzhai avatar Apr 13 '24 00:04 xiangzhai

@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.

weiznich avatar Apr 13 '24 06:04 weiznich

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 :)

xiangzhai avatar Apr 13 '24 08:04 xiangzhai