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.

Sign up for GitHub

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

Jump to bottom

Allow override of BUILD_SHARED_LIBS #1464

Merged
merged 5 commits into from
Feb 16, 2023
Merged

Allow override of BUILD_SHARED_LIBS #1464

merged 5 commits into from
Feb 16, 2023

Conversation

Ibarria
Copy link
Contributor

@Ibarria Ibarria commented Feb 12, 2023 

The cmake option BUILD_SHARED_LIBS is a cmake built-in option
to control if libraries are by default SHARED or STATIC.

For large projects, it is desired to be able to design gtsam as a
shared or static library regardless of the value of BUILD_SHARED_LIBS.
This change is unobtrusive, two new cmake options are created, to
force gtsam to be a shared or static library. If neither option is
set (this is the default), the behavior of gtsam remains unchanged
which is to use BUILD_SHARED_LIBS decision.

@Ibarria
Allow override of BUILD_SHARED_LIBS
1bed89e 
The cmake option BUILD_SHARED_LIBS is a cmake built-in option
to control if libraries are by default SHARED or STATIC.

For large projects, it is desired to be able to design gtsam as a
shared or static library regardless of the value of BUILD_SHARED_LIBS.
This change is unobtrusive, two new cmake options are created, to
force gtsam to be a shared or static library. If neither option is
set (this is the default), the behavior of gtsam remains unchanged
which is to use BUILD_SHARED_LIBS decision.
@Ibarria
Minor cleanup on HandleEigen.cmake
5347d20
dellaert
dellaert approved these changes Feb 12, 2023
View reviewed changes
Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

LGTM !

@dellaert
Copy link
Member

I will merge after CI passes.

@dellaert
Copy link
Member

Again, windows fails. It is weird, this issue was fixed by defining _ENABLE_EXTENDED_ALIGNED_STORAGE in GtsamBuildTypes.cmake, but you did not modify that...

@dellaert dellaert mentioned this pull request Feb 14, 2023
Merged
@Ibarria
Fix cmake issue with msvc
b103fea 
The cmake call to set_properties is fully overriding a property,
in this case the COMPILE_DEFINITIONS and we were losing an eigen
definition needed. The correct way to do this is to use the cmake
function target_compile_definitions
@varunagrawal varunagrawal mentioned this pull request Feb 15, 2023
Closed
@dellaert
Copy link
Member

I will merge this as many Linux CI's actually succeed, so I think it's more OOM stuff.

@dellaert dellaert closed this Feb 16, 2023
@dellaert dellaert reopened this Feb 16, 2023
@dellaert
Copy link
Member

Dang, close by accident. Will run CI again, but only required...

@dellaert dellaert merged commit a96b691 into borglab:develop Feb 16, 2023
@jlblancoc jlblancoc mentioned this pull request Mar 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dellaert dellaert dellaert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Ibarria @dellaert @varunagrawal