-
Notifications
You must be signed in to change notification settings - Fork 789
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 BUILD_SHARED_LIBS as a configurable option #1946
Add BUILD_SHARED_LIBS as a configurable option #1946
Conversation
My understanding is that we have custom flags since it gives us more control over the subprojects. I may be wrong and if we can deem these custom flags unnecessary, that may be a good thing. |
What if we add |
1ecb023
to
a453b66
Compare
I added |
I'm a bit confused. Doesn't adding |
I think @Gold856 's point is that CMake's standard is BUILD_SHARED_LIBS, and most experienced users should be aware of it, while the others are GTSAM's custom cmake variables. This PR is now IMO good to go as a workaround to keep backwards compatibility... but, thinking of a next release, it might probably be just easier to provide BUILD_SHARED_LIBS as in this PR, and remove all the logic around static/dynamic libraries and let CMake to just follow BUILD_SHARED_LIBS. Docs should then be updated, but it would make gtsam to behave as a "standard" CMake package. |
Yeah, that was the idea. I use |
05e65b6
to
f9cdd99
Compare
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 like this if @jlblancoc likes this :-)
Update build logic to match and also prevent conflicting options
f9cdd99
to
fb0d41d
Compare
fb0d41d
to
7b9b14e
Compare
All checks should pass now. Apparently building metis as a shared library was broken on Windows due to symbols not being exported, which I fixed by turning on |
GTSAM_FORCE_SHARED_LIB
defaulting toON
is unintuitive. This means that building static libraries requires settingGTSAM_FORCE_SHARED_LIB
to false andGTSAM_FORCE_STATIC_LIB
to true. It would be significantly more natural to setBUILD_SHARED_LIBS
once, and only reach into theFORCE
options if needed. By addingBUILD_SHARED_LIBS
as a configurable option defaulting toON
and settingGTSAM_FORCE_SHARED_LIB
toOFF
, backwards compatibility is maintained, as shared libraries are still the default and theFORCE
options work as intended, but now onlyBUILD_SHARED_LIBS
needs to be set to select a build type.