fix: allow cross compilation under builtin flag v2 #185
Conversation
crates/opencascade-sys/Cargo.toml
Outdated
| @@ -13,7 +13,7 @@ cxx = "1" | |||
| [build-dependencies] | |||
| cmake = "0.1" | |||
| cxx-build = "1" | |||
| occt-sys = { version = "0.5.1", optional = true } | |||
| occt-sys = { path = "../occt-sys", optional = true } | |||
There was a problem hiding this comment.
@bschwind not sure about this one here
There was a problem hiding this comment.
Ah, I think we intentionally don't use the local dependency in order for our CI setup to cache it properly. See this comment.
Though we should revisit if that assumption still holds.
There was a problem hiding this comment.
so I use the 0.5.1 version?
There was a problem hiding this comment.
I think we'll have to do it in two parts:
- I'll review the occt-sys changes and once we agree on those we can publish
0.5.2or0.6depending on if it's a breaking change or not - We can then make the (somewhat trivial)
opencascade-syschange separately
Sorry for the trouble on that, it's mostly trying to optimize for caching and faster CI build times. I'm also working on getting a windows dev machine set up to more easily be able to test this.
There was a problem hiding this comment.
done. Then there will be a follow-up once if released. Will do a squash on approve.
fidoriel
changed the title
|
Am using my feature branch within https://github.com/fidoriel/MeshVault/actions/runs/11877885906/job/33097958333. THis is a ci run of a cross compiled x64/aarch64 target, while compiling aarch within a x64 environment. So it works :) |
|
@bschwind ping |
|
Hi @fidoriel, sorry for the delay. I haven't yet been able to get a windows machine up and running and I'll be traveling fairly soon so it might take me some time. At the very least I'll test this on MacOS and Linux first and we can merge it if that goes well, and then I'll check the windows stuff later. Edit - actually, I forgot this is a pretty small change now and we need to publish the next occt-sys, so I'll try to get that done soon. |
|
@fidoriel I published your changes to occt-sys under version 0.6 There are some unrelated CI issues I'll sort out later. |
There was a problem hiding this comment.
Alright, this makes some sense, the OCCT is built for the "target", not to run as part of the build system. At the same time, I see that other -sys crates in the ecosystem usually perform the cmake build in build.rs.
I'm looking at #160 and I wonder (and this may be a silly question): wouldn't it be the solution that occt-sys is a runtime dependency of opencascade-sys?
For context @fidoriel: I briefly worked on these crates with @bschwind in the past, and he asked me to take a quick look.
| .define("USE_XLIB", "FALSE") | ||
| .define("INSTALL_DIR_LIB", LIB_DIR) | ||
| .define("INSTALL_DIR_INCLUDE", INCLUDE_DIR) | ||
| .profile("Release") |
There was a problem hiding this comment.
I think cmake crate itself derives the "cmake profile" from the "Rust profile" being used, so explicitly passing it should not be in theory advantageous. Though I'm not sure if the new occt_path() includes the "Rust profile" (I'd say it would be better if it did).
| println!( | ||
| "cargo:rustc-env=OCCT_SRC_DIR={}", | ||
| Path::new(&var("CARGO_MANIFEST_DIR").unwrap()).join("OCCT").to_string_lossy() | ||
| ); | ||
| println!( | ||
| "cargo:rustc-env=OCCT_PATCH_DIR={}", | ||
| Path::new(&var("CARGO_MANIFEST_DIR").unwrap()).join("patch").to_string_lossy() | ||
| ); |
There was a problem hiding this comment.
Maybe a silly question, but do these variables even have to be set here? I.e. lib.rs may be able to derive them in the same way, so build.rs may not even have to exist?
| } | ||
|
|
||
| /// Build the OCCT library. | ||
| pub fn build_occt() { |
There was a problem hiding this comment.
Nobody calls this (as of this PR), is that intended?
There was a problem hiding this comment.
I believe there's a part 2 coming for this PR. We first wanted to merge this to publish the next version of occt-sys since it is not included in the workspace.
Supersedes #163 while closing #160