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 option to disable forcing to build MCAP library as shared library #4

Closed

Conversation

wrightsg
Copy link

The CMake option MCAP_BUILDER_BUILD_SHARED_LIB can be used to disable forcing to build the MCAP library as shared library.

The default behavior (i.e., if the option is not specified) remains the same as previously.

We ran into issues when running our application which linked against MCAP library while having ROS2 environment sourced. Sourcing ROS2 environment will modify LD_LIBRARY_PATH and cause applications that dynamically link against MCAP library to pick up the MCAP library from the ROS2 installation instead of the one that was used during the build.

Linking MCAP library statically is a possible solution to this problem.

Comment on lines +3 to +7
if (MCAP_BUILDER_BUILD_SHARED_LIB)
add_library(mcap SHARED lib.cpp)
else()
add_library(mcap lib.cpp)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

why not?

Suggested change
if (MCAP_BUILDER_BUILD_SHARED_LIB)
add_library(mcap SHARED lib.cpp)
else()
add_library(mcap lib.cpp)
endif()
if (BUILD_SHARED_LIBS)
add_library(mcap SHARED lib.cpp)
else()
add_library(mcap lib.cpp)
endif()

Copy link
Author

Choose a reason for hiding this comment

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

Two reasons

  1. Your change is not backwards compatible: currently, mcap will always be built as shared library and with your change it will now depend on BUILD_SHARED_LIBS variable
  2. Having a separate MCAP_BUILDER_BUILD_SHARED_LIB option allows building mcap library as shared library even if the global BUILD_SHARED_LIBS is set to OFF

If you don't care about these, your suggestion is fine as well.

Note that

if (BUILD_SHARED_LIBS)
  add_library(mcap SHARED lib.cpp)
else()
  add_library(mcap lib.cpp)
endif()

is equivalent to

add_library(mcap lib.cpp)

See https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the reply.
I don't have an issue with having.

add_library(mcap lib.cpp)

We indeed change the behaviour in this way, but we are also fixing a bug, so I feel the breaking change is wanted.

If you can update the PR, I will be happy to merge it.

@filippobrizzi
Copy link
Contributor

I am closing this as I have update the CMake to allow to build mcap as static library

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.

2 participants