Control shared/static linking via BUILD_SHARED_LIBS and ament_cmake_ros #87

esteve · 2017-01-07T12:31:45Z

This PR delegates the decision to build a shared or a static library to CMake's BUILD_SHARED_LIBS

Update:

dirk-thomas · 2017-01-12T16:27:38Z

rmw_fastrtps_cpp/package.xml

  <buildtool_depend>fastrtps_cmake_module</buildtool_depend>

-  <buildtool_export_depend>ament_cmake</buildtool_export_depend>
+  <buildtool_export_depend>ament_cmake_ros</buildtool_export_depend>


The package doesn't export ament_cmake_ros anywhere therefore I don't think it needs to be exported here either.

Sorry, I forgot about this. I've reverted that change and rebased against master. Thanks!

dirk-thomas · 2017-03-22T16:48:32Z

Looks good to me.

Normal CI builds:

CI builds with -DBUILD_SHARED_LIBS=0:

dirk-thomas · 2017-03-22T16:50:45Z

It looks like the ament_cmake_ros package lacks an export directive for ament_cmake. Does it compile for you locally?

esteve · 2017-03-22T17:00:38Z

IIRC it used to, but haven't tried recently, I only updated this PR and addressed your comment because it was getting stale. Isn't this sufficient to export ament_cmake already?

https://github.com/ros2/ament_cmake_ros/blob/master/ament_cmake_ros/package.xml#L12

or you mean in the CMakeLists.txt file?

dirk-thomas · 2017-03-22T17:01:36Z

Yes, it needs to be exported in the CMake file in order to be part of the generated CMake config file.

esteve · 2017-03-22T17:08:33Z

@dirk-thomas I've submitted ros2/ament_cmake_ros#1, but don't review it just yet, I'm building this locally.

esteve · 2017-03-22T17:22:03Z

@dirk-thomas thanks for merging ros2/ament_cmake_ros#1 so quickly, but I wasn't done building this yet 😄

dirk-thomas · 2017-03-22T17:30:00Z

The change was trivial to review and definitely the right thing to do. The manifest already exported the information. The lack of exporting it in CMake was an obvious oversight.

esteve · 2017-03-22T17:40:33Z

@dirk-thomas I know, it was just a tongue-in-cheek comment, buddy.

esteve · 2017-03-22T17:47:07Z

@dirk-thomas I've submitted ros/console_bridge#42 so that console_bridge can be linked against class_loader statically, though I don't know what that usercase would be. IIRC, when I've built ROS 2.0 statically I've skipped class_loader altogether, since there's not need for it.

esteve · 2017-03-22T17:48:26Z

Regarding the -fPIC flag in console_bridge, here's the error output from http://ci.ros2.org/job/ci_linux/2345/ (BUILD_SHARED_LIBS=OFF)

/usr/bin/ld: /home/rosbuild/ci_scripts/ws/install/console_bridge/lib/libconsole_bridge.a(console.cpp.o): relocation R_X86_64_32S against `.rodata' can not be used when making a shared object; recompile with -fPIC
16:48:11 /home/rosbuild/ci_scripts/ws/install/console_bridge/lib/libconsole_bridge.a: error adding symbols: Bad value
16:48:11 collect2: error: ld returned 1 exit status
16:48:11 CMakeFiles/class_loader.dir/build.make:174: recipe for target 'libclass_loader.so' failed
16:48:11 make[2]: *** [libclass_loader.so] Error 1
16:48:11 CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/class_loader.dir/all' failed
16:48:11 make[1]: *** [CMakeFiles/class_loader.dir/all] Error 2
16:48:11 Makefile:138: recipe for target 'all' failed
16:48:11 make: *** [all] Error 2

dirk-thomas · 2017-03-22T18:43:05Z

Normal CI builds look good:

The CI builds with -DBUILD_SHARED_LIBS=0 use ros/console_bridge#42:

http://ci.ros2.org/job/ci_linux/2349/
http://ci.ros2.org/job/ci_osx/1805/
http://ci.ros2.org/job/ci_windows/2451/ this one failed

dirk-thomas · 2017-03-27T17:47:49Z

The CI build with -DBUILD_SHARED_LIBS=0 using ros/console_bridge#43:

http://ci.ros2.org/job/ci_windows/2474/

esteve · 2017-03-27T18:10:58Z

@dirk-thomas thanks for triggering the build, @j-rivero 's pull request seems to have fixed the PIC issues, fantastic!

dirk-thomas · 2017-04-05T20:15:18Z

Waiting for the console_bridge PR to be merged.

esteve · 2017-04-10T10:32:46Z

@dirk-thomas now that ros/console_bridge#43 is merged, is there anything pending that needs to be addressed? Thanks.

dirk-thomas · 2017-04-10T15:32:30Z

This was still blocked by ament/gtest_vendor#4 et al which just got merged. Before running CI on this I am waiting for the comments on the other PRs in order to test them together.

dirk-thomas · 2017-04-10T15:50:18Z

@esteve This also needs to be rebased since it fails to compile against the current master of other packages.

esteve · 2017-04-13T10:32:47Z

@dirk-thomas rebased. IIRC the build farm would rebase before building if there are no conflicts, though.

mikaelarguedas · 2017-04-13T12:39:59Z

@esteve Unfortunately the buildfarm rebases only if you give it a branch name to rebase. If you provide it a repos file it just clones the repos and can't rebase because it doesn't know against what branch it should be rebased (discussion here if you're interested)

esteve · 2017-04-13T18:21:27Z

@mikaelarguedas that's unfortunate indeed, thanks for the explanation!

wjwwood · 2017-04-13T22:18:25Z

I ran into the same thing testing ros2/rmw#80, an unfortunate issue with testing forks in our CI.

dirk-thomas · 2017-04-20T17:04:53Z

Since this is unrelated to the rosidl PRs I triggered some CI builds just for this patch since it looks good as is:

@esteve needs a rebase again

esteve · 2017-04-27T08:54:32Z

@dirk-thomas I just pushed a rebased branch.

dirk-thomas · 2017-04-27T18:56:34Z

dirk-thomas · 2017-04-27T23:25:36Z

CI builds look good. Thanks for iterating on this.

esteve · 2017-04-28T07:43:54Z

@dirk-thomas thanks for the patience!

esteve added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jan 7, 2017

dirk-thomas reviewed Jan 12, 2017

View reviewed changes

esteve force-pushed the honor-build-shared-libs branch from df8be34 to 6577f9e Compare March 22, 2017 11:45

esteve mentioned this pull request Mar 22, 2017

Export ament_cmake ros2/ament_cmake_ros#1

Merged

esteve mentioned this pull request Mar 23, 2017

Add -fPIC flag ros/console_bridge#42

Closed

dirk-thomas mentioned this pull request Mar 27, 2017

Implement the use of the generate_export_header cmake macro ros/console_bridge#43

Merged

dirk-thomas mentioned this pull request Apr 12, 2017

Control shared/static linking via BUILD_SHARED_LIBS and ament_cmake_ros ros2/ros2#306

Closed

esteve force-pushed the honor-build-shared-libs branch from 6577f9e to 5c58ead Compare April 13, 2017 10:21

esteve force-pushed the honor-build-shared-libs branch from 5c58ead to 05894e7 Compare April 15, 2017 13:44

Control shared/static linking via BUILD_SHARED_LIBS and ament_cmake_ros

4412197

esteve force-pushed the honor-build-shared-libs branch from 05894e7 to 4412197 Compare April 27, 2017 08:54

dirk-thomas merged commit f0b9770 into ros2:master Apr 27, 2017

dirk-thomas removed the in review Waiting for review (Kanban column) label Apr 27, 2017

esteve deleted the honor-build-shared-libs branch April 28, 2017 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control shared/static linking via BUILD_SHARED_LIBS and ament_cmake_ros #87

Control shared/static linking via BUILD_SHARED_LIBS and ament_cmake_ros #87

esteve commented Jan 7, 2017 •

edited

Loading

dirk-thomas Jan 12, 2017

esteve Mar 22, 2017

dirk-thomas commented Mar 22, 2017

dirk-thomas commented Mar 22, 2017

esteve commented Mar 22, 2017

dirk-thomas commented Mar 22, 2017

esteve commented Mar 22, 2017

esteve commented Mar 22, 2017 •

edited

Loading

dirk-thomas commented Mar 22, 2017

esteve commented Mar 22, 2017

esteve commented Mar 22, 2017

esteve commented Mar 22, 2017

dirk-thomas commented Mar 22, 2017 •

edited

Loading

dirk-thomas commented Mar 27, 2017

esteve commented Mar 27, 2017

dirk-thomas commented Apr 5, 2017

esteve commented Apr 10, 2017

dirk-thomas commented Apr 10, 2017

dirk-thomas commented Apr 10, 2017 •

edited

Loading

esteve commented Apr 13, 2017

mikaelarguedas commented Apr 13, 2017

esteve commented Apr 13, 2017

wjwwood commented Apr 13, 2017

dirk-thomas commented Apr 20, 2017 •

edited

Loading

esteve commented Apr 27, 2017

dirk-thomas commented Apr 27, 2017 •

edited

Loading

dirk-thomas commented Apr 27, 2017

esteve commented Apr 28, 2017

Control shared/static linking via BUILD_SHARED_LIBS and ament_cmake_ros #87

Control shared/static linking via BUILD_SHARED_LIBS and ament_cmake_ros #87

Conversation

esteve commented Jan 7, 2017 • edited Loading

dirk-thomas Jan 12, 2017

Choose a reason for hiding this comment

esteve Mar 22, 2017

Choose a reason for hiding this comment

dirk-thomas commented Mar 22, 2017

dirk-thomas commented Mar 22, 2017

esteve commented Mar 22, 2017

dirk-thomas commented Mar 22, 2017

esteve commented Mar 22, 2017

esteve commented Mar 22, 2017 • edited Loading

dirk-thomas commented Mar 22, 2017

esteve commented Mar 22, 2017

esteve commented Mar 22, 2017

esteve commented Mar 22, 2017

dirk-thomas commented Mar 22, 2017 • edited Loading

dirk-thomas commented Mar 27, 2017

esteve commented Mar 27, 2017

dirk-thomas commented Apr 5, 2017

esteve commented Apr 10, 2017

dirk-thomas commented Apr 10, 2017

dirk-thomas commented Apr 10, 2017 • edited Loading

esteve commented Apr 13, 2017

mikaelarguedas commented Apr 13, 2017

esteve commented Apr 13, 2017

wjwwood commented Apr 13, 2017

dirk-thomas commented Apr 20, 2017 • edited Loading

esteve commented Apr 27, 2017

dirk-thomas commented Apr 27, 2017 • edited Loading

dirk-thomas commented Apr 27, 2017

esteve commented Apr 28, 2017

esteve commented Jan 7, 2017 •

edited

Loading

esteve commented Mar 22, 2017 •

edited

Loading

dirk-thomas commented Mar 22, 2017 •

edited

Loading

dirk-thomas commented Apr 10, 2017 •

edited

Loading

dirk-thomas commented Apr 20, 2017 •

edited

Loading

dirk-thomas commented Apr 27, 2017 •

edited

Loading