Make rclcpp in to a library #140

wjwwood · 2015-10-29T09:31:39Z

Connects to #48

This pr currently builds on #137, but I'll rebase it against master once that is merged. In the mean time you can review changes specific to this pr with this link: allocator_template...rclcpp_library

I have not reenabled the intra process manager tests because it was using a somewhat peculiar method of mocking the classes within rclcpp and I'll have to come up with an alternative approach.

On Windows this relies on the unresolved library symbols workaround. The Windows CI job will need to be updated before it can handle the new limitations this puts on Windows, see: #48 (comment)

dirk-thomas · 2015-10-29T17:28:23Z

rclcpp/CMakeLists.txt

+  src/rclcpp/utilities.cpp
+)
+add_library(${PROJECT_NAME} SHARED ${${PROJECT_NAME}_SRCS})
+ament_target_dependencies(${PROJECT_NAME} "rmw" "rcl_interfaces")


Dependencies in alphabetical order.

dirk-thomas · 2015-10-29T17:29:53Z

For a more detailed review I will wait until the PR is rebased since the referenced diff is still difficult to read. Can I also assume that all the created .cpp files contain the exact code extracted from the .hpp files and don't need to be reviewed?

wjwwood · 2015-10-29T22:29:42Z

For a more detailed review I will wait until the PR is rebased since the referenced diff is still difficult to read.

I don't expect it will be easier to read, after the rebase, than this: allocator_template...rclcpp_library because the only thing the rebase will do is remove @jacquelinekay's commits from this pr.

Can I also assume that all the created .cpp files contain the exact code extracted from the .hpp files and don't need to be reviewed?

For the most part that is true. I can summarize the handful of cases where I had to do more:

I changed two functions in executor.hpp to no longer be templated so I could put them in .cpp. The wait_for_work and get_next_executable used to be templated on a chrono duration ratio, but now they just take chrono nanoseconds and the calling functions do the duration cast before passing the timeout.
I moved a static class member into a static global in a .cpp file in the IntraProcessManager class.
I also moved some functions from rclcpp.hpp into executors.hpp so they could be put into a .cpp file. The rclcpp.hpp header cannot be included by the rclcpp library, but must be included by the user, see: allocator_template...rclcpp_library#diff-690567624ee368716009118718985564R29 and the corresponding: allocator_template...rclcpp_library#diff-809285dba1df7726afedd63625b6a6ceR34

wjwwood · 2015-10-30T01:12:33Z

Rebased, but I'm still working on comments and fixing the intra process tests.

jacquelinekay · 2015-10-30T05:38:08Z

Linux build was broken, I added a missing include.

wjwwood · 2015-10-30T09:13:14Z

Thanks @jacquelinekay, I've updated it with fixes for uncrustify and I fixed the intra process tests.

wjwwood · 2015-10-30T09:22:23Z

Ok, I think I've addressed all of the comments, so here is the Linux and OS X CI:

The biggest thing left to do before I think this is ready for merge is to update the Windows CI farm to do a matrix build of the different DDS vendors rather than all in one workspace. The matrix can either be just running N builds back to back in the same job or really using Jenkin's matrix build pattern. I don't know what the best option is in that case. I'll look at it tomorrow morning, but if others have ideas on how that should work, I'd be happy to have some help on that.

wjwwood · 2015-10-30T10:14:38Z

I had to fix a problem that only showed up in isolated builds, where rclcpp had an undeclared dependency on rosidl_generator_cpp.

However, there is still and issue with how we are using some of the message related symbols within rclcpp which I haven't sorted out yet. I'll try to figure that out in the morning.

jacquelinekay · 2015-10-30T18:49:32Z

Why do node.hpp and node_impl.hpp need to be different files? (I'll consider consolidating them in another PR.)

jacquelinekay · 2015-10-30T19:43:05Z

rclcpp/test/test_intra_process_manager.cpp

@@ -122,7 +122,8 @@ class SubscriptionBase
 #define Publisher mock::Publisher
 #define PublisherBase mock::PublisherBase
 #define SubscriptionBase mock::SubscriptionBase
-#include <rclcpp/intra_process_manager.hpp>
+#include "../src/rclcpp/intra_process_manager.cpp"


Ah. Interesting workaround.

wjwwood · 2015-10-30T19:46:34Z

I think I have fixed the type support problems.

jacquelinekay · 2015-10-30T21:49:37Z

It's interesting that the Linux CI job failed, because the isolated build actually works on my desktop... maybe I have fpermissive turned on?

wjwwood · 2015-10-30T22:17:38Z

Thanks @jacquelinekay, I'm kicking off new CI jobs:

wjwwood · 2015-11-02T11:13:33Z

I spent some time working on the Windows issues, but I've been unable to finish those changes. Instead I've just pushed some changes that should fix Linux's build.

The root of the issue on Linux was that sometimes no template functions which use rmw_* functions where used from rclcpp's headers. This resulted in a situation where librclcpp was missing rmw symbols but the user's executable or library was not. So when linked against the rmw implementation shared library, the linker discarded it even though librclcpp would have used them. By using an rmw symbol directly in rclcpp.hpp I've forced all users of the header to depend on at least one rmw symbol, and therefore avoiding the aforementioned scenario.

There's still a problem that I introduced while working on Windows which introduces an infinite recursion that I need to track down, and so I won't start new CI. I'll have to tackle that tomorrow.

wjwwood · 2015-11-02T19:25:27Z

I believe I have fixed the infinite recursion issue (it was a consequence of a Windows fix), so I'll do CI for Linux and OS X:

wjwwood · 2015-11-02T19:28:32Z

It might be that the code I conditioned on WIN32 in d6769db maybe related to the SEH errors in Windows that we were seeing (they could also be caused by infinite recursion calls). In that case we would need to find another way to do 9cad948.

wjwwood · 2015-11-02T20:54:47Z

Rebased after #142

wjwwood · 2015-11-02T22:00:14Z

I have this branch building and tests running on all three platforms now, but there are still many warnings that need to be looked at on Windows, but in the mean time new CI:

wjwwood · 2015-11-02T22:55:59Z

Ok I've fixed the linkage warnings on Windows. Now the only thing remaining is figuring out what to do about the dependency structure on Windows. That last CI job failed because rmw_opensplice_cpp was not built and installed before rclcpp:

http://ci.ros2.org/job/ros2_batch_ci_windows/556/

Strangely rmw_connext_cpp is built before, but I'm not sure why that might happen just yet. We'll probably end up having to put a dependency on the default rmw_implementations from some package that rclcpp depends on. We'll have to see what affect that has on Linux/OS X. Currently some of them build before rclcpp so I guess it shouldn't be that big of a change, e.g.:

# Topological order
[ ... ]
 - connext_cmake_module
 - opensplice_cmake_module
 - rmw
 - rmw_implementation
 - rmw_implementation_cmake
 - rosidl_cmake
 - rosidl_default_generators
 - rosidl_generator_c
 - rosidl_generator_dds_idl
 - rosidl_parser
 - rcl
 - rclc
 - rmw_connext_shared_cpp
 - rosidl_generator_cpp
 - rosidl_typesupport_connext_cpp
 - rmw_connext_cpp
 - rosidl_typesupport_introspection_cpp
 - rmw_connext_dynamic_cpp
 - rosidl_typesupport_opensplice_cpp
 - builtin_interfaces
 - example_interfaces
 - pendulum_msgs
 - rcl_interfaces
 - rclcpp
 - rmw_opensplice_cpp
[ ... ]

As you can see, both connext rmw implementations come before rclcpp, but rmw_opensplice_cpp comes just after for some reason.

wjwwood · 2015-11-02T23:27:04Z

Ok, I added a build dependency on rmw_implementation in rclcpp. I'm not convinced this is correct, but I don't have a better idea and I think it will allow the branches to build and pass on all platforms, so here is CI:

If the CI looks ok, then we can start to discuss the content of the pull request and I can make any adjustments that we think are necessary.

wjwwood · 2015-11-03T00:57:26Z

With ros2/rmw_opensplice#90 I can build Windows completely. New CI for Windows (the Linux and OS X jobs look good):

http://ci.ros2.org/job/ros2_batch_ci_windows/560/

I'll start a Connext one if that one looks good (and it doesn't interfere with ros2 packaging jobs).

wjwwood · 2015-11-05T21:00:52Z

Ok I squashed and I'll try to get the connext on Windows tests going right after lunch and then it should be good to merge.

wjwwood · 2015-11-06T03:03:17Z

Ok, hopefully final CI:

http://ci.ros2.org/job/ros2_batch_ci_linux/566/
http://ci.ros2.org/job/ros2_batch_ci_osx/439/
http://ci.ros2.org/job/ros2_batch_ci_windows/584/ (opensplice)
http://ci.ros2.org/job/ros2_batch_ci_windows/585/ (connext static)
http://ci.ros2.org/job/ros2_batch_ci_windows/586/ (connext dynamic)

wjwwood · 2015-11-06T05:00:49Z

Ok, they're all green except the connext versions of the Windows jobs. I'm gonna merge.

Make rclcpp in to a library

esteve · 2015-11-06T17:32:53Z

👏 👏 👏 🎆

tfoote · 2015-11-06T19:03:16Z

🎉

dirk-thomas · 2015-12-03T23:29:14Z

This PR results in the following compiler warning: http://ci.ros2.org/view/packaging/job/packaging_linux/51/warnings17Result/package.-1174361695/

esteve · 2015-12-04T00:34:32Z

I've pushed this branch (https://github.com/ros2/rclcpp/tree/avoid_compiler_warning) to hopefully fix the compiler warning. CI jobs:

http://ci.ros2.org/job/ci_linux/659/
http://ci.ros2.org/job/ci_osx/542/
http://ci.ros2.org/job/ci_windows_opensplice/735/
http://ci.ros2.org/job/ci_windows_connext_dynamic/22/
http://ci.ros2.org/job/ci_windows_connext_static/21/

dirk-thomas · 2015-12-04T00:40:32Z

Since the warning is only present in the packaging job but not in the CI job we should run the packaging job if we want to actually check if the patch makes a difference.

wjwwood · 2015-12-04T00:41:15Z

Thanks for looking into it @esteve, you beat me to it.

esteve · 2015-12-04T01:10:29Z

@wjwwood no problem, it's been fun to learn about (yet) another GNU extension 😄

move time files to rcutils

* Include node namespace in IPC Action service name * Include node namespace in IPC Client/Service service name --------- Co-authored-by: Mauro Passerino <mpasserino@irobot.com>

wjwwood added the in progress Actively being worked on (Kanban column) label Oct 29, 2015

dirk-thomas reviewed Oct 29, 2015
View reviewed changes

wjwwood force-pushed the rclcpp_library branch from 4761732 to 0dd00dd Compare October 30, 2015 01:11

jacquelinekay reviewed Oct 30, 2015
View reviewed changes

jacquelinekay mentioned this pull request Oct 30, 2015

Fix build for making rclcpp a library ros2/demos#38

Merged

jacquelinekay self-assigned this Oct 30, 2015

wjwwood force-pushed the rclcpp_library branch from 21a5582 to f020cf1 Compare November 2, 2015 11:11

wjwwood force-pushed the rclcpp_library branch from d6769db to 37d18da Compare November 2, 2015 20:53

This was referenced Nov 5, 2015

Change behavior of timer on construction #146

Merged

Pass callbacks as universal references #147

Merged

wjwwood force-pushed the rclcpp_library branch from 1fa8f68 to f0988b6 Compare November 6, 2015 00:14

wjwwood mentioned this pull request Nov 6, 2015

Select one of connext or connext dynamic on Windows Jenkins job ros2/ros2#147

Closed

wjwwood and others added 6 commits November 5, 2015 18:55

prepare splitting headers into cpp files

f0faa1f

split headers into cpp files

f531b02

update code after splitting into headers and cpp files

0bbe9b2

[style] cpplint/uncrustify

51695d1

remove unnecessary header

ef89f5a

fixes for Windows

3dd193e

wjwwood force-pushed the rclcpp_library branch from c80ffeb to 3dd193e Compare November 6, 2015 02:58

[style] uncrustify

36cd6e3

wjwwood added a commit that referenced this pull request Nov 6, 2015

Merge pull request #140 from ros2/rclcpp_library

ca64af3

Make rclcpp in to a library

wjwwood merged commit ca64af3 into master Nov 6, 2015

wjwwood removed the in progress Actively being worked on (Kanban column) label Nov 6, 2015

wjwwood deleted the rclcpp_library branch November 6, 2015 05:01

dirk-thomas mentioned this pull request Dec 4, 2015

Avoid unused return value warning #168

Merged

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

Merge pull request ros2#140 from ros2/move_time

14a4966

move time files to rcutils

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make rclcpp in to a library #140

Make rclcpp in to a library #140

wjwwood commented Oct 29, 2015

dirk-thomas Oct 29, 2015

dirk-thomas commented Oct 29, 2015

wjwwood commented Oct 29, 2015

wjwwood commented Oct 30, 2015

jacquelinekay commented Oct 30, 2015

wjwwood commented Oct 30, 2015

wjwwood commented Oct 30, 2015

wjwwood commented Oct 30, 2015

jacquelinekay commented Oct 30, 2015

jacquelinekay Oct 30, 2015

wjwwood commented Oct 30, 2015

jacquelinekay commented Oct 30, 2015

wjwwood commented Oct 30, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 3, 2015

wjwwood commented Nov 5, 2015

wjwwood commented Nov 6, 2015

wjwwood commented Nov 6, 2015

esteve commented Nov 6, 2015

tfoote commented Nov 6, 2015

dirk-thomas commented Dec 3, 2015

esteve commented Dec 4, 2015

dirk-thomas commented Dec 4, 2015

wjwwood commented Dec 4, 2015

esteve commented Dec 4, 2015

Make rclcpp in to a library #140

Make rclcpp in to a library #140

Conversation

wjwwood commented Oct 29, 2015

dirk-thomas Oct 29, 2015

Choose a reason for hiding this comment

dirk-thomas commented Oct 29, 2015

wjwwood commented Oct 29, 2015

wjwwood commented Oct 30, 2015

jacquelinekay commented Oct 30, 2015

wjwwood commented Oct 30, 2015

wjwwood commented Oct 30, 2015

wjwwood commented Oct 30, 2015

jacquelinekay commented Oct 30, 2015

jacquelinekay Oct 30, 2015

Choose a reason for hiding this comment

wjwwood commented Oct 30, 2015

jacquelinekay commented Oct 30, 2015

wjwwood commented Oct 30, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 2, 2015

wjwwood commented Nov 3, 2015

wjwwood commented Nov 5, 2015

wjwwood commented Nov 6, 2015

wjwwood commented Nov 6, 2015

esteve commented Nov 6, 2015

tfoote commented Nov 6, 2015

dirk-thomas commented Dec 3, 2015

esteve commented Dec 4, 2015

dirk-thomas commented Dec 4, 2015

wjwwood commented Dec 4, 2015

esteve commented Dec 4, 2015