IntelRealSense / realsense_sdk_zr300

Toolkit built on top of the Intel® RealSense™ Cross Platform API (librealsense)
Apache License 2.0
55 stars 39 forks source link

Don't explicitly define library type #10

Closed srware closed 8 years ago

srware commented 8 years ago

Some people may want to build shared libraries rather than static. By explicitly defining their type in the CMake files the user doesn't get a choice.

By default static libs are built but the user can specify '-DBUILD_SHARED_LIBS:BOOL=ON' in their cmake command to build shared libs.

Bolshem commented 8 years ago

rs_logger cannot be built as static lib. It is not mandatory .so file. The log_utils looks HARDCODED for librs_logger.so, if the file found then the logger will work and logs will be written, if the file not found, the logger will be replaced by empty logger, which writes the logs to /dev/null. Compiling rs_logger as static makes no sense. (Look in log_utils.cpp source code). Compiling the rest of the libraries as static/shared should be discussed with the owners

srware commented 8 years ago

Thanks @Bolshem. I hadn't looked into it that closely, just got annoyed that it was forcing me to build static libs and couldn't be overridden using standard CMake flags. I have updated the pull request to always build librs_logger as a shared library.

Looks like a CMake option is required for building with log support or not...

Netanel-Kaufman commented 8 years ago

What about users who wants to build static libraries of the SDK, but to be latter linked into a dynamic library? We probably don't want SDK to build with -fPIC by default. Should there also be another CMake option for that? any other CMake-magic to do that?

srware commented 8 years ago

Why would building with -fPIC stop users compiling a shared library from a static library? I would have thought this would be a requirement so best left in. Compilers often ignore it in certain situations anyway.

Netanel-Kaufman commented 8 years ago

I'm sorry if I wasn't clear.. what I meant is that currently the libraries are not built with -fPIC. afaik if you build shared library the compiler takes care for that for you, but if you build static libraries it doesn't. so wither we'll add -fPIC by default (but I'm not sure if that's a good practice, as it might come with some, e.g. in code size? performance? I'm not sure..), or we'll make that optional, or let the user tweak it by himself?

srware commented 8 years ago

From what I can see the libraries are built with -fPIC currently as it is set globally in the sdk root CMakeLists ('set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -fPIC"') which I think is the best practice in this case.

Bolshem commented 8 years ago

Thats true. -fPIC is our default option for libraries.

EldarZ commented 8 years ago

hi @srware, Thanks!, we added this build option internally, taking into account libs that must be shared for a good reason : https://github.com/IntelRealSense/realsense_sdk/blob/development/sdk/CMakeLists.txt

#----------- Build shared or static library ------------
option(BUILD_STATIC "Set to ON to build static libraries. Some libraries are always shared" OFF)
if(BUILD_STATIC)
    MESSAGE("Building libraries as static objects")
    set(SDK_LIB_TYPE "STATIC")
else()
    MESSAGE("Building libraries as shared objects(default)")
    set(SDK_LIB_TYPE "SHARED")
endif(BUILD_STATIC)