search

zyantific / zydis-rs

Zydis Rust Bindings
MIT License
83 stars 14 forks source link

build.rs: fix build on Windows/Visual Studio #14

Closed williballenthin closed 6 years ago

williballenthin commented 6 years ago

see #13.

williballenthin commented 6 years ago

Things may break if users directly set cmake env vars (e.g. CMAKE_PREFIX_PATH), but the default case is much better now.

williballenthin commented 6 years ago

works on Windows/Visual Studio:

image

works on linux:

image

th0rex commented 6 years ago

Hey, thanks for your PR!

I personally don't really care about users setting CMAKE_PREFIX_PATH, so not supporting that is fine (honestly the cmake crate should provide us with the proper path, and I was under the impression it does).

However, the cmake crate seems to prefer the OPT_LEVEL environment Variable and the DEBUG environment Variable over the PROFILE, and it also supports the RelWithDebInfo and MinSizeRel builds. It would be nice if you could add support for that to the PR, otherwise I'll add it later on. It can be pretty much just copied from the cmake crate.

Also, if you want, you can just .unwrap the env Variables directly, they're always set, but I'm also fine with the .unwrap_or(String::new()) thing.

Just let me know if you want to add support for the RelWithDebInfo and MinSizeRel variants, then I'll leave this PR open until thats done, otherwise I'll merge this so the basic case works on windows and will add the other things later on.

williballenthin commented 6 years ago

hey @th0rex, thanks for your patience with this PR and for suggesting great enhancements. I've pulled the logic from cmake-rs to detect the rust compilation profile and use this to derive the build directory on Windows. Happy to make further changes --- just let me know where I should focus my attention.

th0rex commented 6 years ago

This looks good to me, thanks!

athre0z commented 6 years ago

Thanks for the PR! In the long run, it appears to me like this would be something that might be interesting for any user of the cmake-rs crate and thus worth contributing to their crate instead of pretty much duplicating part of their logic in our code. I only had a quick look on the issue and the solution, so please excuse me if I'm not making any sense.

williballenthin commented 6 years ago

Thanks @athre0z, you are making (at least to me).

As I get more experience with Rust, I'll keep this in mind and look to contribute back upstream. At the moment, I realize I'm using an uncommon build environment while learning Rust at the same time. So, I'd like to be a bit more confident that I truly understand how things work, and after that, participate in the community.