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

Using installed version OpenCASCADE #87

Merged
merged 3 commits into from
Aug 26, 2023
Merged

Conversation

katyo
Copy link
Contributor

@katyo katyo commented Jul 15, 2023

This PR adds support installed version of OpenCASCADE library when it found and fits prerequisites. This drastically speeds up build. You can use builtin feature to prefer builtin version and skip detecting installed one.

Also added support for dynamic linking via using dynamic feature so this PR also superseeds #26.

@katyo katyo mentioned this pull request Jul 15, 2023
@katyo
Copy link
Contributor Author

katyo commented Jul 17, 2023

@bschwind What do you think?

@bschwind
Copy link
Owner

Hey @katyo, thanks for the PR! I think once we figure out where opencascade-sys is going (being discussed in #86), we can then add these changes on top (or vice-versa).

I should also see if this helps resolve #27.

In any case, please give me a bit of time to test and review :)

Copy link
Collaborator

@strohel strohel left a comment

Choose a reason for hiding this comment

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

Thanks @katyo, this is indeed a very useful change. I posted some thoughts/suggestions for the implementation. We've also working on #86 with @bschwind. I'm happy to rebase/adapt this if #86 lands sooner.

BTW this could be even a solution for slow CI - at least the Ubuntu runner provides opencascade in the repository.

crates/opencascade-sys/OCCT_PKG/CMakeLists.txt Outdated Show resolved Hide resolved
crates/opencascade-sys/Cargo.toml Outdated Show resolved Hide resolved
crates/opencascade-sys/build.rs Outdated Show resolved Hide resolved
crates/opencascade-sys/build.rs Outdated Show resolved Hide resolved
crates/opencascade-sys/build.rs Outdated Show resolved Hide resolved
crates/opencascade-sys/build.rs Outdated Show resolved Hide resolved
@katyo katyo force-pushed the feat/dynamic-link branch from 1fe781f to 5b605f0 Compare July 18, 2023 16:55
@katyo katyo requested a review from strohel July 18, 2023 16:56
@katyo katyo force-pushed the feat/dynamic-link branch from 5b605f0 to 0035f94 Compare July 23, 2023 07:27
@katyo
Copy link
Contributor Author

katyo commented Jul 23, 2023

@bschwind I rebased this PR to current master.
@strohel I changed it as you suggested before but I have some doubts.

Maybe we should enable builtin feature by default or remove it in favor of external feature which will enable using installed OpenCASCADE libraries instead of compiling builtin one. But it seems cargo does not provide a way to disable optional dependency (occt-sys) conditionally when some feature is enabled.

@katyo katyo force-pushed the feat/dynamic-link branch 5 times, most recently from e297b8f to 2829cb8 Compare July 23, 2023 08:15
@bschwind
Copy link
Owner

I've been delayed in reviewing this but I'll give it a try soon!

Copy link
Collaborator

@strohel strohel left a comment

Choose a reason for hiding this comment

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

Looking good! Just adding some nits.

Maybe we should enable builtin feature by default

Yes, please, on opencascade level.

crates/opencascade-sys/Cargo.toml Outdated Show resolved Hide resolved
crates/opencascade-sys/build.rs Outdated Show resolved Hide resolved
crates/opencascade-sys/build.rs Outdated Show resolved Hide resolved
crates/opencascade-sys/build.rs Outdated Show resolved Hide resolved
crates/opencascade-sys/build.rs Outdated Show resolved Hide resolved
crates/opencascade-sys/build.rs Outdated Show resolved Hide resolved
crates/opencascade-sys/build.rs Outdated Show resolved Hide resolved
crates/opencascade-sys/build.rs Outdated Show resolved Hide resolved
@katyo katyo force-pushed the feat/dynamic-link branch 3 times, most recently from 0bfaf52 to 1db87f7 Compare July 23, 2023 17:08
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.

This is looking good, just need a few tweaks:

  • Fix the build errors
  • Fix the clippy warnings (we run cargo clippy --release --all-targets -- -D warnings in CI)

Could you also add a bit of documentation in the README on how to use the bundled OCCT, vs. using the system-installed one?

crates/opencascade-sys/build.rs Outdated Show resolved Hide resolved
@katyo katyo force-pushed the feat/dynamic-link branch 2 times, most recently from e99cb3c to 251153c Compare July 24, 2023 04:45
@katyo
Copy link
Contributor Author

katyo commented Jul 24, 2023

@bschwind @strohel I thought a more proper solution is using cmake in both cases (when builtin feature is enabled and also when it is disabled).

@katyo katyo force-pushed the feat/dynamic-link branch from 251153c to 5c30bc7 Compare July 24, 2023 06:01
@strohel
Copy link
Collaborator

strohel commented Jul 24, 2023

@bschwind @strohel I thought a more proper solution is using cmake in both cases (when builtin feature is enabled and also when it is disabled).

Yeah this should work! We'd just need to provide a good error message in case the version doesn't check with the builtin feature (basically say that the developers are to blame, that's not on the user side).

@katyo katyo force-pushed the feat/dynamic-link branch 5 times, most recently from 808745d to 4164df4 Compare July 24, 2023 11:55
@katyo katyo force-pushed the feat/dynamic-link branch from 4164df4 to 2ae6e25 Compare July 25, 2023 08:48
@katyo katyo requested a review from strohel July 25, 2023 13:12
@bschwind
Copy link
Owner

@katyo could you please rebase this on the latest main (which is now based on opencascade 7.7.1)?

@katyo
Copy link
Contributor Author

katyo commented Jul 31, 2023

@bschwind Does this mean that OCCT 7.7.1 is a minimum supported version?

@katyo katyo force-pushed the feat/dynamic-link branch 2 times, most recently from 56090d3 to 7bb9e21 Compare July 31, 2023 06:39
@katyo
Copy link
Contributor Author

katyo commented Jul 31, 2023

@bschwind Currently CI won't work as expected when occt-sys needs to be updated too. Hope such behavior was predictable.

@bschwind
Copy link
Owner

@katyo that's okay! I see what you mean, we need to update occt-sys first with the changes, and then publish that to 0.3.

@bschwind Does this mean that OCCT 7.7.1 is a minimum supported version?

Unfortunately yes. We could go back and see when exactly the ComputeNormals function was added and use that version I suppose, but it was a nightmare to get the crate size down to 10MB, and that later version was needed so I didn't have to depend on the StdPrs package.

I can help to support this PR and get it through, as I currently have the publishing rights to occt-sys, just give me some time to complete that.

@katyo
Copy link
Contributor Author

katyo commented Aug 12, 2023

@bschwind Any chance that this PR will be merged soon?
Seems currently development stuck. I would like add many more changes.

@bschwind
Copy link
Owner

Hi @katyo , yes I still want to get to this for sure. I've been on a two week trip so development and review has been a bit slow. This PR is the next one I want to test so I'll update you here when I've done so. Sorry for the wait!

DSchroer and others added 2 commits August 19, 2023 17:34
This commit uses cmake to find installed OpenCASCADE libraries in system.
If it found it will be used if it version fits to requirements.
Otherwise the builtin one will be used.
The `builtin` feature may be enabled to force use builtin OpenCASCADE.
@bschwind
Copy link
Owner

bschwind commented Aug 19, 2023

@strohel could I get another review from you on this? Especially concerning the newly added builtin feature flags on the examples and viewer crates, I wasn't sure if that was the right approach or not.

Also, I excluded occt-sys from the workspace in c59c037 and 96a6634, does that seem logical to you?

@bschwind
Copy link
Owner

@katyo I'm back from the trip I was on. I pushed some changes to your branch and published 0.3 of occt-sys to get this building. Thanks so much for your patience! Please take a look at the changes and let me know if you agree with them.

I've tested this fairly thoroughly and things are working well!

Interestingly, I seem to get better performance when using the builtin version of OCCT. Anecdotally on my mac mini m1, the keycap model takes:

  • ~1.58 seconds using the statically-linked OCCT
  • ~2.01 seconds using the dynamically-linked, system-provided OCCT

@bschwind bschwind mentioned this pull request Aug 19, 2023
@katyo
Copy link
Contributor Author

katyo commented Aug 20, 2023

@bschwind

Are you sure that this works as expected?
Because I tested it with linux only maybe something went wrong.
Please look at ldd output in both cases to check which OCCT libs actually used.

@bschwind
Copy link
Owner

@katyo I tested it with --no-default-features and without --no-default-features:

Dynamic Linking

$ cargo build --release --no-default-features --bin viewer

opencascade $ otool -L target/release/viewer                          
target/release/viewer:
	/System/Library/Frameworks/QuartzCore.framework/Versions/A/QuartzCore (compatibility version 1.2.0, current version 1.11.0)
	/System/Library/Frameworks/Metal.framework/Versions/A/Metal (compatibility version 1.0.0, current version 258.17.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.0.0)
	/usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)
	/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit (compatibility version 45.0.0, current version 2113.20.111)
	/System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices (compatibility version 1.0.0, current version 56.0.0)
	/System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics (compatibility version 64.0.0, current version 1557.3.2)
	/System/Library/Frameworks/CoreVideo.framework/Versions/A/CoreVideo (compatibility version 1.2.0, current version 1.5.0)
	/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1856.105.0)
	/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation (compatibility version 300.0.0, current version 1856.105.0)
	/opt/homebrew/opt/opencascade/lib/libTKMath.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKernel.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKFeat.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKGeomBase.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKG2d.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKG3d.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKTopAlgo.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKGeomAlgo.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKBRep.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKPrim.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKSTEP.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKSTEPAttr.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKSTEPBase.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKSTEP209.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKSTL.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKMesh.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKShHealing.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKFillet.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKBool.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKBO.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKOffset.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/opt/homebrew/opt/opencascade/lib/libTKXSBase.7.7.dylib (compatibility version 7.7.0, current version 7.7.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1200.3.0)
	/usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)
	/System/Library/Frameworks/ColorSync.framework/Versions/A/ColorSync (compatibility version 1.0.0, current version 4.7.0)

Static Linking

$ cargo build --release --bin viewer

opencascade $ otool -L target/release/viewer 
target/release/viewer:
	/System/Library/Frameworks/QuartzCore.framework/Versions/A/QuartzCore (compatibility version 1.2.0, current version 1.11.0)
	/System/Library/Frameworks/Metal.framework/Versions/A/Metal (compatibility version 1.0.0, current version 258.17.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.0.0)
	/usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)
	/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit (compatibility version 45.0.0, current version 2113.20.111)
	/System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices (compatibility version 1.0.0, current version 56.0.0)
	/System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics (compatibility version 64.0.0, current version 1557.3.2)
	/System/Library/Frameworks/CoreVideo.framework/Versions/A/CoreVideo (compatibility version 1.2.0, current version 1.5.0)
	/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1856.105.0)
	/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation (compatibility version 300.0.0, current version 1856.105.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1200.3.0)
	/usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)
	/System/Library/Frameworks/ColorSync.framework/Versions/A/ColorSync (compatibility version 1.0.0, current version 4.7.0)

@katyo
Copy link
Contributor Author

katyo commented Aug 20, 2023

@bschwind Seems it works as expected.

@bschwind
Copy link
Owner

@katyo were you having trouble making it work on linux? I can test it out on a linux machine today too.

@katyo
Copy link
Contributor Author

katyo commented Aug 21, 2023

@bschwind No, it works fine for me (NixOS). I think it shouldn't have problems with other distros either.

Maybe we won't get significant speedup using dynamic linking itself but using pre-installed OCCT allows skip building it which takes several minutes on my laptop.

@bschwind
Copy link
Owner

@strohel I'm going to merge this, but please leave any post-merge reviews you may have and we can always open up new PRs to make any adjustments you might suggest.

@katyo thanks again for your work and patience on this.

@bschwind bschwind merged commit a3f6b7c into bschwind:main 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.

4 participants