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.

Sign up for GitHub

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

fix: allow cross compilation under builtin flag v2 #185

Merged
merged 2 commits into from
Nov 30, 2024
Merged

fix: allow cross compilation under builtin flag v2 #185

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ba3d56
fix: allow cross compilation under builtin flag
DSchroer Jan 16, 2024
85bd71d
undo sys changes
fidoriel Nov 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions crates/occt-sys/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,4 @@ exclude = [
]

[dependencies]

[build-dependencies]
cmake = "0.1"

# Adding an empty workspace table so occt-sys doesn't believe
# it's in the parent workspace. This crate is excluded from
# the top-level workspace because it takes quite awhile to
# build and the crate doesn't change very often.
[workspace]
39 changes: 9 additions & 30 deletions crates/occt-sys/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,12 @@
const LIB_DIR: &str = "lib";
const INCLUDE_DIR: &str = "include";
use std::{env::var, path::Path};

fn main() {
let current_dir = std::env::current_dir().expect("Should have a 'current' directory");
let patch_dir = current_dir.join("patch");

let dst = cmake::Config::new("OCCT")
.define("BUILD_PATCH", patch_dir)
.define("BUILD_LIBRARY_TYPE", "Static")
.define("BUILD_MODULE_ApplicationFramework", "FALSE")
.define("BUILD_MODULE_Draw", "FALSE")
.define("USE_D3D", "FALSE")
.define("USE_DRACO", "FALSE")
.define("USE_EIGEN", "FALSE")
.define("USE_FFMPEG", "FALSE")
.define("USE_FREEIMAGE", "FALSE")
.define("USE_FREETYPE", "FALSE")
.define("USE_GLES2", "FALSE")
.define("USE_OPENGL", "FALSE")
.define("USE_OPENVR", "FALSE")
.define("USE_RAPIDJSON", "FALSE")
.define("USE_TBB", "FALSE")
.define("USE_TCL", "FALSE")
.define("USE_TK", "FALSE")
.define("USE_VTK", "FALSE")
.define("USE_XLIB", "FALSE")
.define("INSTALL_DIR_LIB", LIB_DIR)
.define("INSTALL_DIR_INCLUDE", INCLUDE_DIR)
.build();

println!("cargo:rustc-env=OCCT_PATH={}", dst.to_str().expect("path is valid Unicode"));
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()
);
}
Comment on lines +4 to +11
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

43 changes: 40 additions & 3 deletions crates/occt-sys/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,46 @@
use std::path::Path;
use std::{
env::var,
path::{Path, PathBuf},
};

const LIB_DIR: &str = "lib";
const INCLUDE_DIR: &str = "include";

/// Get the path to the OCCT library installation directory to be
/// used in build scripts.
///
/// Only valid during build (`cargo clean` removes these files).
pub fn occt_path() -> &'static Path {
Path::new(env!("OCCT_PATH"))
pub fn occt_path() -> PathBuf {
// moves the output into target/TARGET/OCCT
// this way its less likely to be rebuilt without a cargo clean
Path::new(&var("OUT_DIR").expect("missing OUT_DIR")).join("../../../../OCCT")
}

/// Build the OCCT library.
pub fn build_occt() {
cmake::Config::new(Path::new(env!("OCCT_SRC_DIR")))
Copy link
Collaborator

@strohel strohel Dec 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nobody calls this (as of this PR), is that intended?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.define("BUILD_PATCH", Path::new(env!("OCCT_PATCH_DIR")))
.define("BUILD_LIBRARY_TYPE", "Static")
.define("BUILD_MODULE_ApplicationFramework", "FALSE")
.define("BUILD_MODULE_Draw", "FALSE")
.define("USE_D3D", "FALSE")
.define("USE_DRACO", "FALSE")
.define("USE_EIGEN", "FALSE")
.define("USE_FFMPEG", "FALSE")
.define("USE_FREEIMAGE", "FALSE")
.define("USE_FREETYPE", "FALSE")
.define("USE_GLES2", "FALSE")
.define("USE_OPENGL", "FALSE")
.define("USE_OPENVR", "FALSE")
.define("USE_RAPIDJSON", "FALSE")
.define("USE_TBB", "FALSE")
.define("USE_TCL", "FALSE")
.define("USE_TK", "FALSE")
.define("USE_VTK", "FALSE")
.define("USE_XLIB", "FALSE")
.define("INSTALL_DIR_LIB", LIB_DIR)
.define("INSTALL_DIR_INCLUDE", INCLUDE_DIR)
.profile("Release")
.out_dir(occt_path())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.build();
}
2 changes: 1 addition & 1 deletion crates/opencascade-sys/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bschwind not sure about this one here

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I use the 0.5.1 version?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.2 or 0.6 depending on if it's a breaking change or not
  • We can then make the (somewhat trivial) opencascade-sys change 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable.

Copy link
Contributor Author

@fidoriel fidoriel Nov 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. Then there will be a follow-up once if released. Will do a squash on approve.


[features]
builtin = ["occt-sys"]
1 change: 1 addition & 0 deletions crates/opencascade-sys/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ impl OcctConfig {
// Add path to builtin OCCT
#[cfg(feature = "builtin")]
{
occt_sys::build_occt();
std::env::set_var("DEP_OCCT_ROOT", occt_sys::occt_path().as_os_str());
}

Expand Down
Loading