-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add vcpkg support and corresponding CI. #251
Conversation
Use vcpkg-rs to manage z3 instead. A non-default feature vcpkg is added. However, vcpkg-rs does not support wasm32 target currently. I created a pull request there at mcgoo/vcpkg-rs#53
Make emscripten visible to wasm32-unknown-unknown if emscripten is installed.
Z3_HEADER_VAR will disappear when the feature vcpkg is enabled.
Use is_err instead of pattern matching.
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.
We'll also want to do something to add CI coverage of this to make sure it works ... like adding a vcpkg build on Windows in CI.
Despite all of the above, I do want to say that I'm very happy to see this and thanks so much for starting us down this path! |
Okay, but I'm afraid that I need some time for it. I may be busy for several days as I'm attending a game. |
z3-sys/build.rs
Outdated
let wasm32 = target.starts_with("wasm32-unknown"); | ||
let wasm32_emscripten = target == "wasm32-unknown-emscripten"; | ||
if wasm32 { | ||
let sysroot = env::var("EMSDK") |
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.
It seems that sometimes the cache may be missing for fresh emscripten installation. So I'm not sure whether we should add some notices here.
.github/workflows/rust.yml
Outdated
build_with_vcpkg_installed_z3: | ||
strategy: | ||
matrix: | ||
build: [linux, macos, windows] |
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.
For now, let's just do this on Windows since that's where GitHub has it installed.
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.
Okay, I see. Thanks. And sorry, I just copied some codes from previous CI.
And it's weird that CI failed on Windows, as I've built z3 that way on Windows several times.
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.
I saw it failing on Linux and that caused the macOS and Windows builds to get cancelled. (At least in the run that I looked at.)
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.
I've added an option to disable fail-fast in my branch. Here's the result.
Oops, it's caused because the runner run out of space. I didn't notice that.
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.
This may be fixed by cleaning the vcpkg build tree.
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.
I'll have more comments later ... but wanted to at least flag this for now.
const Z3_HEADER_VAR: &str = "Z3_SYS_Z3_HEADER"; | ||
|
||
fn main() { | ||
// Feature `vcpkg` is prior to `static-link-z3` as vcpkg-installed z3 is also statically linked. | ||
|
||
#[cfg(not(feature = "vcpkg"))] | ||
#[cfg(feature = "static-link-z3")] |
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.
I want to rename static-link-z3
to bundled
as the static linking isn't what's actually getting handled here. Either vcpkg
or static-link-z3
should be set, but not both.
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.
Okay, I see. I may do that tomorrow as it's midnight here. Thanks.
So that rust can continue to build.
By this, we can check whether the toolchain matches the vcpkg triplet.
Merge tested CI changes
I'm going to merge this and then see about further / subsequent changes! |
Thank you again for working on this! |
Support using vcpkg-rs to manage z3 library.
Added a non-default feature vcpkg.
However, vcpkg-rs does not support wasm32 target currently. I created a pull request there at mcgoo/vcpkg-rs#53.