Skip to content
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

russcip fails to compile on Windows MSVC: u32 != i32 #114

Closed
KarelPeeters opened this issue Oct 26, 2023 · 2 comments · Fixed by #115
Closed

russcip fails to compile on Windows MSVC: u32 != i32 #114

KarelPeeters opened this issue Oct 26, 2023 · 2 comments · Fixed by #115

Comments

@KarelPeeters
Copy link
Contributor

KarelPeeters commented Oct 26, 2023

Problem

When trying to compile russcip on Windows MSVC, I get a couple of errors that are all similar to this:

error[E0308]: mismatched types
  --> [...]\.cargo\registry\src\index.crates.io-6f17d22bba15001f\russcip-0.2.6\src\branchrule.rs:34:43
   |
32 |     fn from(val: BranchingResult) -> Self {
   |                                      ---- expected `u32` because of return type
33 |         match val {
34 |             BranchingResult::DidNotRun => ffi::SCIP_Result_SCIP_DIDNOTRUN,
   |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u32`, found `i32`
   |
help: you can convert an `i32` to a `u32` and panic if the converted value doesn't fit
   |
34 |             BranchingResult::DidNotRun => ffi::SCIP_Result_SCIP_DIDNOTRUN.try_into().unwrap(),
   |                                                                          ++++++++++++++++++++

Cause

The underlying reason is that the enum SCIP_Result type is represented by unsigned int on Linux, but by int in Windows MSVC. Apparently this is allowed by the C++ spec:

Declares an unscoped enumeration type whose underlying type is not fixed (in this case, the underlying type is an implementation-defined integral type that can represent all enumerator values; this type is not larger than int unless the value of an enumerator cannot fit in an int or unsigned int.

bindgen then just exposes the underlying type as u32 or i32 to Rust, see rust-lang/rust-bindgen#1966.

In turn, russcip does not handle this type difference correctly, and fails to compile.

Solution

I see a couple of potential solutions:

  • Tell bindgen to generate an actual Rust enum instead. This might be problematic for the reasons described in Default to generating constified enums, rather than generating Rust enums rust-lang/rust-bindgen#758, but I don't know how this applies to SCIP specifically.
  • Expose this type difference to users russcip, by changing all types to SCIP_Result instead of u32. This is annoying because it just pushes the problem onto users, who will probably all also end up with either broken Linux or Windows builds.
  • Add casts to u32 in all the right places in russcip, at least for windows targets. Then everyone would be using u32 all the time, even though it's not what the underlying SCIP library is using.

I think the last solution is the easiest to implement, and also the best one. I could submit a PR for any solution if that's helpful and a decision has been made.

This might also be the only thing stopping Windows CI from working, in which case it can be turned on again!

# TODO: fix windows workflow

@mmghannam
Copy link
Member

Hi @KarelPeeters! thank you for the very well-structured detailed issue. I agree that the last solution sounds like the best fit, and I would be happy to review your PR :)

@KarelPeeters
Copy link
Contributor Author

Since looking into this more and actually putting together a fix, I think the second solution is actually the better one.

russcip already isolates users from SCIP_Result with its own separate enum types, eg. BranchingResult. The functions that were failing to compile earlier are exactly those that convert between those. Of course it's fine if they have to know about the underlying type, their entire purpose is hiding it from users!

I've submitted this second solution as #115.

@KarelPeeters KarelPeeters changed the title russcip fails to compile on Windows MSVC: u32 != i32 russip fails to compile on Windows MSVC: u32 != i32 Oct 26, 2023
@KarelPeeters KarelPeeters changed the title russip fails to compile on Windows MSVC: u32 != i32 russcip fails to compile on Windows MSVC: u32 != i32 Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants