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

Add dynamic linking support for faster builds #26

Closed
wants to merge 5 commits into from

Conversation

DSchroer
Copy link
Contributor

@DSchroer DSchroer commented May 28, 2023

Working out of multiple repositories, the opencascade-sys library tends to be a major bottleneck for building. Added a dynamic feature that uses system libs rather than building opencascade from scratch each time.

Copy link
Owner

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! There's a bit more work to be done to make this portable but it's going in the right direction.

@@ -1,44 +1,29 @@
const LIBS: [&str; 18] = [
Copy link
Owner

Choose a reason for hiding this comment

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

You can avoid having to specify the array length:

Suggested change
const LIBS: [&str; 18] = [
const LIBS: &[&str] = &[

println!("cargo:rustc-link-lib=static=TKBool");
println!("cargo:rustc-link-lib=static=TKBO");
println!("cargo:rustc-link-lib=static=TKOffset");
let is_dynamic = std::env::var("CARGO_FEATURE_DYNAMIC").is_ok();
Copy link
Owner

Choose a reason for hiding this comment

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

We should be able to use the cfg macro or a cfg attribute.

if cfg!(feature = "dynamic") {
    ...
}

or something like

#[cfg(feature = "dynamic")]
fn is_dynamic() {
    true    
}

#[cfg(not(feature = "dynamic"))]
fn is_dynamic() {
    false
}

In this case the cfg!() macro probably makes more sense.

for lib in LIBS {
println!("cargo:rustc-link-lib=dylib={lib}");
}
build.include("/usr/include/opencascade");
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure this is very portable, for example if I install opencascade with homebrew on macos, it'll be in /opt/homebrew/include/opencascade.

pkg-config is pretty commonly used for this sort of task.

I have an example build.rs for another project which has a feature flag for dynamic vs. static linking, as well as usage of pkgconfig.

It'll increase the complexity of build.rs, but it makes the library more likely to build without errors when dynamically linking.


println!("cargo:rustc-link-search=native={}", dst.join("lib").display());

format!("{}", dst.join("include").display())
Copy link
Owner

Choose a reason for hiding this comment

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

Then this return value can just be

Suggested change
format!("{}", dst.join("include").display())
dst.join("include")

@@ -63,3 +61,26 @@ fn main() {
println!("cargo:rerun-if-changed=src/lib.rs");
println!("cargo:rerun-if-changed=include/wrapper.hxx");
}

fn build_opencascade() -> String {
Copy link
Owner

Choose a reason for hiding this comment

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

This can be PathBuf instead of String (see the comment below)

Suggested change
fn build_opencascade() -> String {
fn build_opencascade() -> PathBuf {

tuna-f1sh added a commit that referenced this pull request Jul 16, 2023
Could add thiserror as build depenancy for proper Error exit and
possibly cache bundled INCLUDE/LIB like occt-sys crate
@bschwind
Copy link
Owner

Closed in favor of #87

@bschwind bschwind closed this Aug 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 this pull request may close these issues.

2 participants