Add -fPIC flag

[esteve]

Add -fPIC so that console_bridge can be linked statically with other libraries (i.e. BUILD_SHARED_LIBS is OFF)

[scpeters]

Thanks @esteve ! I'll need a second opinion though, perhaps from @j-rivero since I'm not sure about the implications of that compiler flag.

[esteve]

@scpeters no worries! Here's a very concise explanation of what the cons of -fPIC are (basically size), but why it's needed for linking statically against other libraries:

http://stackoverflow.com/a/4036497

I can make the logic a bit smarter, for example check if it's being built statically and only add the -fPIC flag if so.

[traversaro]

If we can bump the cmake_minimum_required version to 2.8.9, we can just set the POSITION_INDEPENDENT_CODE property of the target to true.

[scpeters]

Trusty uses cmake 2.8.12, so @traversaro's suggestion seems viable to me:

• http://packages.ubuntu.com/trusty/cmake

[dirk-thomas]

This seems to fail on Windows: http://ci.ros2.org/job/ci_windows/2451/

[j-rivero]

I don't see a major problem by adding the fPIC option to the build but I agree with Silvio that handled it using cmake (if possible) would be a better implementation.

I don't know the reason of generating only a static library for console_bridge. Adding the generation of a shared library is also an easy approach.

This seems to fail on Windows: http://ci.ros2.org/job/ci_windows/2451/

I'm unable to locate exactly the error in the build log ... :(

[dirk-thomas]

This seems to fail on Windows: http://ci.ros2.org/job/ci_windows/2451/

I'm unable to locate exactly the error in the build log ... :(

Search for "error " in the console output.

[scpeters]

I don't see the error either. There are 217 instances of error in that console log, but it's not obvious where the failure is coming from.

Also, it's not obvious why that windows build failure is related to this PR, since the change is inside an if(NOT WIN32) block.

[mikaelarguedas]

I think what @dirk-thomas is pointing out here is that this fix doesn't solve the fact that console_bridge can't be linked statically on Windows. As @scpeters pointed it out, this flag is added in a if(NOT WIN32) bloc without an MSVC equivalent in the else so it's not surprising that it still fails on Windows. @esteve any idea what MSVC flag would (automagically) make linking :unicorn: work on windows ?

[tfoote]

I found the error. You can find the error quickly using a search for "2 Error"

+1 for using the cmake property instead of the -Wextra

[j-rivero]

I found the error. You can find the error quickly using a search for "2 Error"

I see it now, thanks.

@esteve any idea what MSVC flag would (automagically) make linking :unicorn: work on windows ?

AFAIK, there is no Position Independent Code in Windows. It could be that the problem is related to how the visibility is handled in the code. I see that the code is using console_bridge_EXPORTS to change between when a symbol is exported or imported. I did not find any place in the build system where that flag is turned on when creating the library.

[traversaro]

I see that the code is using console_bridge_EXPORTS to change between when a symbol is exported or imported. I did not find any place in the build system where that flag is turned on when creating the library.

The <targetname>_EXPORTS is automatically defined by CMake when compiling a shared library, see DEFINE_SYMBOL .

[j-rivero]

The _EXPORTS is automatically defined by CMake when compiling a shared library, see DEFINE_SYMBOL .

Oh, I did not know about this one, thanks. I'm afraid that we are mixing the shared visibility with the generation of a static library. Looking at the exportdecl.h file something like the following patch should make it to compile:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index d64bd17..f32b78d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -39,6 +39,10 @@ if(NOT DEFINED BUILD_SHARED_LIBS)
   option(BUILD_SHARED_LIBS "Build dynamically-linked binaries" ON)
 endif()

+if(NOT BUILD_SHARED_LIBS)
+  add_definitions("-DCONSOLE_BRIDGE_STATIC")
+endif()
+
 # Control where libraries and executables are placed during the build
 set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_BINDIR}")
 set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_LIBDIR}")

[traversaro]

Yes, the Windows compilation failure is not related at all to POSITION_INDIPENDENT_CODE.

I had a bit of déjà vu discussing this, and then I remember that the same patch was proposed in this PR : https://github.com/ros/console_bridge/pull/40 . I think this is not the ideal solution, as outlined in https://github.com/ros/console_bridge/pull/40#issuecomment-269460721 . Interestingly, the same problem is shared by a lot ros2 libraries, that seem to use a visibility_control.h header.

It is worth mentioning that a way to avoid dealing with this kind of issues directly is to export all the symbols present in a given library, using CMake's CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and WINDOWS_EXPORT_ALL_SYMBOLS. However with respect to some point of view, exporing all symbols of a library is undesirable, as discussed in How To Write Shared Libraries by Ulrich Drepper .

[esteve]

And here I was searching for "supercalifragilisticexpialidocious", no wonder I never find anything wrong in my log files :stuck_out_tongue_winking_eye:

@traversaro 's suggestion is much clearer than adding -fPIC manually, so if console_bridge can be upgraded to require CMake 2.8.9, I very much prefer using POSITION_INDEPENDENT_CODE over the change in this PR.

@mikaelarguedas as @j-rivero said, the error is not caused by the lack of POSITION_INDEPENDENT_CODE since it's not applicable on Windows. What I've seen other projects do is build the shared version of a library no matter what, and additionally build the static one. That way you ensure that the symbols are exported. Having said that, I had never tried compiling all of ROS 2.0 statically on Windows (see https://github.com/ros2/rmw_fastrtps/pull/87#issuecomment-288482634 for how I got here) , so I assume this error may show up in other libraries.

For this specific case, @traversaro suggested on https://github.com/ros/console_bridge/pull/40#issuecomment-269460721 a while ago several approaches on how to address this.

[esteve]

Oops! I just saw @traversaro 's previous comment :smile:

[j-rivero]

It is worth mentioning that a way to avoid dealing with this kind of issues directly is to export all the symbols present in a given library, using CMake's CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and WINDOWS_EXPORT_ALL_SYMBOLS. However with respect to some point of view, exporing all symbols of a library is undesirable, as discussed in How To Write Shared Libraries by Ulrich Drepper .

-1 , we can not scale it to other (bigger) libraries due to the mentioned performance reasons.

For this specific case, @traversaro suggested on #40 (comment) a while ago several approaches on how to address this.

+1

[scpeters]

I'm +1 on requiring cmake 2.8.9 since 2.8.12 is available in trusty

[j-rivero]

For this specific case, @traversaro suggested on #40 (comment) a while ago several approaches on how to address this.

+1

I've implemented (hopefully right) the suggestion of using the export header macro in #43

[esteve]

Closing in favor of #43 Thanks @j-rivero ! @dirk-thomas triggered a CI build for Windows (http://ci.ros2.org/job/ci_windows/2474/) and it seems to have passed, yay!