Try all available implementations instead of hardcoding OpenSplice #5

esteve · 2016-02-25T19:32:45Z

No description provided.

esteve · 2016-02-25T19:33:31Z

CI:

http://ci.ros2.org/job/ci_linux/1036/
http://ci.ros2.org/job/ci_osx/855/
http://ci.ros2.org/job/ci_windows/1098/

dirk-thomas · 2016-02-25T19:35:07Z

Embedding a compile time hard coded list into the code is not necessary here. This should use the ament resource index instead.

wjwwood · 2016-02-25T20:38:02Z

@esteve the Python implementation of the ament index API is here: https://github.com/ament/ament_index/blob/master/ament_index_python/ament_index_python/__init__.py

However, there might be an issue with using the index since a new rmw impl could be added after this package is built and so the index may contain more implementations than existed when the C extensions were built. So long as the code is gracefully degrading that shouldn't be an issue though.

esteve · 2016-02-25T22:28:39Z

@dirk-thomas @wjwwood I've replaced the vendor.py.in file with a Python module that queries the index and retrieves all the rmw implementations at runtime.

esteve · 2016-02-25T22:29:33Z

CI:

http://ci.ros2.org/job/ci_linux/1037/
http://ci.ros2.org/job/ci_osx/856/
http://ci.ros2.org/job/ci_windows/1099/

dirk-thomas · 2016-02-25T22:31:17Z

rclpy/CMakeLists.txt

+macro(rmw_list)
+  set(RMW_IMPLEMENTATIONS "${RMW_IMPLEMENTATIONS} ${rmw_implementation}")
+endmacro()
+call_for_each_rmw_implementation(rmw_list)


There is much simpler API for this: https://github.com/ros2/rmw/blob/master/rmw_implementation_cmake/cmake/get_available_rmw_implementations.cmake

Also what is this variable used for?

This is a remnant of the old code, I just removed it.

esteve · 2016-02-26T00:15:06Z

I've changed the code so that the rclpy implementation can be set via an environment variable and also via Python.

CI:

http://ci.ros2.org/job/ci_linux/1039/
http://ci.ros2.org/job/ci_osx/858/
http://ci.ros2.org/job/ci_windows/1101/

esteve · 2016-03-04T22:42:40Z

After fixing the pyflakes warnings:

http://ci.ros2.org/job/ci_linux/1072/

esteve · 2016-03-04T22:42:54Z

This is ready for review.

dirk-thomas · 2016-03-04T22:46:17Z

rclpy/rclpy/__init__.py

+if _rclpy is None:
+    for rmw_implementation in rmw_implementations:
+        rmw_module = _load_rmw(rmw_implementation)
+        if rmw_module is not None:


In which cases does this return None?

esteve · 2016-03-09T18:34:59Z

Latest CI:

http://ci.ros2.org/job/ci_linux/1089/
http://ci.ros2.org/job/ci_osx/881/
http://ci.ros2.org/job/ci_windows/1154/

wjwwood · 2016-03-09T18:44:18Z

rclpy/rclpy/__init__.py


 def init(args):
+    global _rclpy


It looks like the other functions do not use global _rclpy. Can you either remove it here or add it to the other functions?

Or is that not necessary since this is the only one where you assign it?

Answered my own question: http://stackoverflow.com/a/423596/671658

dirk-thomas · 2016-03-30T18:12:08Z

rclpy/test/test_select_rmw_implementation.py

+    try:
+        return func(*args, **kwargs)
+    except:
+        print("exception in {0}():".format(func.__name__))


To stderr.

wjwwood · 2016-03-30T18:41:26Z

Nitpick: single quotes wherever possible and error messages should go to stderr.

I tried to follow those when doing this. Please point out specific locations if you see them.

I think I addressed them all in f161c04. Btw, for those kind of style only changes I think we agreed that it would generate less churn if you just fixed them directly when you see them, and I'm fine with anyone pushing directly to my PR's.

wjwwood · 2016-03-31T00:42:31Z

I'm going to fire off another Linux CI to test the new pr included in this set (ament/ament_tools#86):

http://ci.ros2.org/job/ci_linux/1129/

wjwwood · 2016-03-31T01:28:29Z

I forgot to push a local change to rosidl, new pr at ros2/rosidl#112, new CI:

http://ci.ros2.org/job/ci_linux/1130/

wjwwood · 2016-03-31T23:15:47Z

All the pr's have been squashed and here is a final (hopefully) set of CI:

wjwwood · 2016-04-01T18:24:58Z

This CI set is clean:

http://ci.ros2.org/job/ci_linux/1138/
http://ci.ros2.org/job/ci_osx/923/
http://ci.ros2.org/job/ci_windows/1201/
- cppcheck failed on all packages (I think eatable needs a restart)

I also have +1's so I'm going to merge these.

dirk-thomas · 2016-04-04T21:37:59Z

@wjwwood It looks like this PR broke the packaging jobs on Linux and OS X.

wjwwood · 2016-04-04T21:53:11Z

@dirk-thomas the bridge tries to import from std_msgs.msg import Header and gets the ROS 2 version when it was expecting the ROS 1 version. I don't know how you want to address that. I guess we can either change the import pattern for ROS 2 or the ros bridge can do something to make sure it gets the ROS 1 version?

I think the ros bridge has been broken since Python messages were introduced, this pr just exposed the problem.

wjwwood · 2016-04-04T21:55:46Z

This is the start of the problem:

From http://ci.ros2.org/view/packaging/job/packaging_linux/199/console:

11:29:03   File "/opt/ros/indigo/lib/python2.7/dist-packages/rospy/__init__.py", line 47, in <module>
11:29:03     from std_msgs.msg import Header
11:29:03   File "/home/rosbuild/ci_scripts/ws/install/lib/python3.4/site-packages/std_msgs/msg/__init__.py", line 1, in <module>
11:29:03     from std_msgs.msg._bool import Bool

dirk-thomas · 2016-04-04T22:10:51Z

I don't know either what the best way to address this is. But I think the bridge is important and we should keep it working in the upcoming alpha.

wjwwood · 2016-04-04T22:17:20Z

I don't know either how to address it but I think the bridge is important and we should keep it working in the upcoming alpha.

I don't think that was in question?

Do you want me to change the import style of the Python code or do you want to change how the bridge builds its code to avoid imported the ROS 2 messages (and other Python 3 stuff from within Python 2)?

wjwwood · 2016-04-04T22:45:04Z

@ros2/dev bump. Sorry I'm not there to discuss in person, but we need to fix this, can you guys look at the issue and weigh in on our short term solution? I'm happy to make the change to the Python import structure, but I'd like some ideas on what to change it to.

jacquelinekay · 2016-04-04T23:11:29Z

I'm not an expert here; I think it will take me a little while to understand how the bridge works and how Python messages are imported before I can give you ideas about how to change it.

dirk-thomas · 2016-04-04T23:16:54Z

I am going to build the bridge locally to see what can be done...

wjwwood · 2016-04-04T23:39:44Z

I was also setting up my local build to build it, let me know if you want me to test something.

@jacquelinekay I'm pretty sure @dirk-thomas could explain it better than me (at least on the ROS bridge side), but the fundamental problem here is that if you source ROS 1 and then ROS 2, and afterwards do from std_msgs.msg import Bool you'll get the generated message from ROS 2. That's because the import signature is the same between ROS 1 and ROS 2 at the moment. This has probably been the case since we introduced the Python message generation, but hasn't been exposed as an issue until I made it impossible to use ROS 2 python modules before calling rclpy.init().

We need to fix that so you can import ROS 2 messages without rclpy being involved, but that would actually just hide this issue again making it logically broken (maybe) instead of throwing an exception as it is now.

So the question to everyone else is should we change the import signature of ROS 2 so this is never a problem, even accidentally. And if so, how should we change it.

Short term we can either change how the ros bridge builds or we can change the import signature in ROS 2. If we choose not to change the import signature then we'll have to do the former, which I guess @dirk-thomas is looking into now. So, I don't think a deep understanding of the bridge is necessary to express an opinion on how we should proceed for this release. Hopefully that explanation helps with that.

wjwwood · 2016-04-04T23:40:59Z

I made a small edit to the previous comment to be more accurate.

jacquelinekay · 2016-04-04T23:46:13Z

Yeah, I understood most of that from the previous comments, but I still have no idea what the best way to proceed is. Did you mean to say "We need to fix that so you can import ROS 2 messages without rclpy being involved"?

If I had to express an opinion, I would say that changing the import signature of ROS 2 messages doesn't feel like the right way to go, especially since the import signature matches in Python and C++ as it is right now (e.g. std_msgs.msg.Bool and std_msgs::msg::Bool).

wjwwood · 2016-04-04T23:54:05Z

Did you mean to say "We need to fix that so you can import ROS 2 messages without rclpy being involved"?

Yup, edited it again, thanks.

If I had to express an opinion, I would say that changing the import signature of ROS 2 messages doesn't feel like the right way to go, especially since the import signature matches in Python and C++ as it is right now (e.g. std_msgs.msg.Bool and std_msgs::msg::Bool).

I tend to agree with that sentiment, also because we have more flexibility with Python to "make it work" through dynamic behavior (:sparkles:magic:sparkles:). Which is not really an option in C++. So if we decide to stick with identical import statements, then I think we need to do some extra work to ensure the messages are being used in the right system. So, for example adding a "build for" marker in the generated code for both ROS 1 and ROS 2, and then we can add checks in common functions like publish which check that there isn't cross-talk. It isn't perfect, but at least it will give a more meaningful error in many cases.

esteve added the in progress Actively being worked on (Kanban column) label Feb 25, 2016

dirk-thomas reviewed Feb 25, 2016
View reviewed changes

esteve force-pushed the multiple-vendors branch from 349729e to 82efeba Compare February 26, 2016 00:13

esteve added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 4, 2016

dirk-thomas reviewed Mar 4, 2016
View reviewed changes

jacquelinekay mentioned this pull request Mar 7, 2016

Added support for introspection type support in C ros2/rmw_fastrtps#18

Closed

gerkey assigned esteve Mar 8, 2016

gerkey added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Mar 8, 2016

gerkey unassigned esteve Mar 8, 2016

gerkey removed the in progress Actively being worked on (Kanban column) label Mar 8, 2016

esteve force-pushed the multiple-vendors branch from 82efeba to 0da8225 Compare March 9, 2016 18:32

esteve added the in review Waiting for review (Kanban column) label Mar 9, 2016

wjwwood reviewed Mar 9, 2016
View reviewed changes

dirk-thomas reviewed Mar 30, 2016
View reviewed changes

wjwwood mentioned this pull request Mar 31, 2016

ensure exec dependencies are sourced when testing ament/ament_tools#86

Merged

wjwwood mentioned this pull request Mar 31, 2016

update get identifier function name ros2/rosidl#112

Merged

refactor selector into a singleton and more selection tools

35b07ce

wjwwood force-pushed the multiple-vendors branch from 711b61d to 35b07ce Compare March 31, 2016 22:54

wjwwood merged commit 65cefbc into master Apr 1, 2016

wjwwood deleted the multiple-vendors branch April 1, 2016 18:39

wjwwood removed the in review Waiting for review (Kanban column) label Apr 1, 2016

dirk-thomas mentioned this pull request Apr 5, 2016

workaround for Python search order ros2/ros1_bridge#23

Merged

reed-lau mentioned this pull request Aug 8, 2018

rclpy segmentfault when Ctrl-C #215

Closed

Try all available implementations instead of hardcoding OpenSplice #5

Try all available implementations instead of hardcoding OpenSplice #5

Conversation

esteve commented Feb 25, 2016

esteve commented Feb 25, 2016

dirk-thomas commented Feb 25, 2016

wjwwood commented Feb 25, 2016

esteve commented Feb 25, 2016

esteve commented Feb 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esteve commented Feb 26, 2016

esteve commented Mar 4, 2016

esteve commented Mar 4, 2016

Choose a reason for hiding this comment

esteve commented Mar 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood commented Mar 30, 2016

wjwwood commented Mar 31, 2016

wjwwood commented Mar 31, 2016

wjwwood commented Mar 31, 2016

wjwwood commented Apr 1, 2016

dirk-thomas commented Apr 4, 2016

wjwwood commented Apr 4, 2016

wjwwood commented Apr 4, 2016

dirk-thomas commented Apr 4, 2016

wjwwood commented Apr 4, 2016

wjwwood commented Apr 4, 2016

jacquelinekay commented Apr 4, 2016

dirk-thomas commented Apr 4, 2016

wjwwood commented Apr 4, 2016

wjwwood commented Apr 4, 2016

jacquelinekay commented Apr 4, 2016

wjwwood commented Apr 4, 2016