-
Notifications
You must be signed in to change notification settings - Fork 779
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
base: develop
Are you sure you want to change the base?
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 |
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.