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 BUILD_SHARED_LIBS as a configurable option #1946

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

Gold856
Copy link

@Gold856 Gold856 commented Dec 24, 2024

GTSAM_FORCE_SHARED_LIB defaulting to ON is unintuitive. This means that building static libraries requires setting GTSAM_FORCE_SHARED_LIB to false and GTSAM_FORCE_STATIC_LIB to true. It would be significantly more natural to set BUILD_SHARED_LIBS once, and only reach into the FORCE options if needed. By adding BUILD_SHARED_LIBS as a configurable option defaulting to ON and setting GTSAM_FORCE_SHARED_LIB to OFF, backwards compatibility is maintained, as shared libraries are still the default and the FORCE options work as intended, but now only BUILD_SHARED_LIBS needs to be set to select a build type.

@Gold856 Gold856 changed the title Don't force shared libraries Don't force shared libraries by default Dec 24, 2024
@varunagrawal
Copy link
Collaborator

  1. Either GTSAM_FORCE_SHARED_LIB or GTSAM_FORCE_STATIC_LIB should be enabled. We can't have both OFF by default currently.
  2. Another option is to get rid of these Cmake flags altogether and use only BUILD_SHARED_LIBS, with it being set to ON by default. This is tricky since it may break backwards compatibility.
  3. The third option is to add a check that GTSAM_FORCE_SHARED_LIB is the same as BUILD_SHARED_LIBS and error out if it is not.

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.

@Gold856
Copy link
Author

Gold856 commented Dec 25, 2024

What if we add BUILD_SHARED_LIBS as an option with the default value being ON, with GTSAM_FORCE_SHARED_LIB defaulting to OFF? CMake docs suggest this, and it's exactly the same behavior (shared libraries by default), with the option of forcing it to shared or static somewhere else if desired.

@Gold856 Gold856 closed this Dec 25, 2024
@Gold856 Gold856 deleted the dont-force-shared-libraries branch December 25, 2024 05:46
@Gold856 Gold856 restored the dont-force-shared-libraries branch December 25, 2024 05:46
@Gold856 Gold856 reopened this Dec 25, 2024
@Gold856 Gold856 force-pushed the dont-force-shared-libraries branch from 1ecb023 to a453b66 Compare December 25, 2024 05:56
@Gold856 Gold856 changed the title Don't force shared libraries by default Add BUILD_SHARED_LIBS as a configurable option Dec 25, 2024
@Gold856
Copy link
Author

Gold856 commented Dec 25, 2024

I added BUILD_SHARED_LIBS as an option and also added an error if both GTSAM_FORCE_SHARED_LIB and GTSAM_FORCE_STATIC_LIB were true. I think this is the best way to provide control over library type, since it allows BUILD_SHARED_LIBS to be used while allowing the FORCE options to override it for advanced use cases. I'm still open to other suggestions though, so let me know!

@varunagrawal
Copy link
Collaborator

I'm a bit confused. Doesn't adding BUILD_SHARED_LIBS as an option now do the same thing as just using GTSAM_FORCE_SHARED_LIB?
I was reading the CMake docs and it says that this option has to be provided by the project and is not defined by default. SInce GTSAM doesn't provide BUILD_SHARED_LIBS as an option, we should probably point users to using GTSAM_FORCE_SHARED_LIB as an equivalent variable.

@jlblancoc
Copy link
Member

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.

@Gold856
Copy link
Author

Gold856 commented Dec 27, 2024

Yeah, that was the idea. I use BUILD_SHARED_LIBS all the time to define library type, and a bunch of projects that I work on use it as well, so it's a little inconvenient that gtsam ignores it completely. And it would be nice one day to have the custom variables removed (maybe not now though.)

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.

3 participants