Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register language of typesupport for rmw implementation #63

Merged
merged 32 commits into from
Apr 29, 2016

Conversation

jacquelinekay
Copy link
Contributor

I believe that when typesupport implementations are registered in the ament index, there should be a way to distinguish between C and C++ typesupport packages. This is one way to implement it, I'm open to feedback about how to change the design. A companion PR to rosidl_generator_py and various other packages will accompany this to show usage.

Right now a typesupport implementation is registered twice: as a "rmw_typesupport" and as a "rmw_typesupport_c/cpp". That way all typesupports can be selected using the "rmw_typesupport" identifier, but in contexts where either C or C++ typesupport is needed, there is a way to distinguish between them.

@jacquelinekay
Copy link
Contributor Author

endmacro()

#
# Get the C type support package name for a specific ROS middleware implementation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this return more then one?

Same below.

@jacquelinekay
Copy link
Contributor Author

I realized after starting the build that PR was subtly broken. I renamed the key for the ament index entry to rmw_typesupport (from rmw_implementation, which I thought was misleading). Fixed in 929018e.

@jacquelinekay
Copy link
Contributor Author

I launched a new build where everything looks good except for failing tests in rclpy. I cannot reproduce this locally. Hmmm.

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

@wjwwood
Copy link
Member

wjwwood commented Apr 12, 2016

@jacquelinekay it's failing because you don't have any rmw implementations enabled which are supported by rclpy. The error message for this case is improved in https://github.com/ros2/rclpy/pull/12/files#diff-aa7e84a9e617f0578e51ed0daace94aaR91.

@jacquelinekay
Copy link
Contributor Author

But use_connext and use_opensplice are both set to true in the job with the failing test (and disable_connext_static is false), so you would expect those rmw implementations to be found and for the test to pass. This leads me to believe that my changes have broken the logic for finding an rmw implementation in Python.

@wjwwood
Copy link
Member

wjwwood commented Apr 12, 2016

@jacquelinekay maybe. It's also possible there is a problem with how my code changes and yours have merged? The supported Python rmw extensions are now registered separately.

@jacquelinekay
Copy link
Contributor Author

That's what I was thinking. I thought I rebuilt my workspace locally with an up-to-date version of rclpy; I'm going to try again and take a closer look at your changes in rclpy.

@wjwwood
Copy link
Member

wjwwood commented Apr 12, 2016

Ok, if there is an issue, then this commit will be the culprit: ros2/rclpy@0bf9640

@jacquelinekay
Copy link
Contributor Author

The rclpy tests passed on Jenkins in this new build:

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

@wjwwood
Copy link
Member

wjwwood commented Apr 12, 2016

So, what made the difference in your opinion?

@jacquelinekay
Copy link
Contributor Author

It looks like the Jenkins job with the failed rclpy test was before your rclpy test was merged, and the successful one was launched afterwards.

All I know is that if I do a git reset --hard HEAD~1 locally in rclpy to revert your most recent PR and rebuild my workspace, the rclpy tests fail (for the same reason as seen on Jenkins, no valid rmw implementation for rclpy found.) But both on Jenkins and locally, if I go back to the current state of rclpy on master, then the test passes.

@wjwwood
Copy link
Member

wjwwood commented Apr 13, 2016

Well there was a matching pr merged, so to "undo" what I merged, you'd need to also undo this one: ament/ament_cmake#65

@jacquelinekay
Copy link
Contributor Author

@dirk-thomas I tried to address the multiple typesupports issue but I didn't understand how to use ament_index_get_resources and ament_index_register_resource for this purpose.

For example, I want to do something like this:

ament_index['rmw_typesupport'] = {'rmw_connext_cpp' : ['rosidl_typesupport_connext_c', 'rosidl_typesupport_connext_cpp'] }

ament_index['rmw_typesupport_c'] = {'rmw_connext_cpp' : ['rosidl_typesupport_connext_c'] }
ament_index['rmw_typesupport_cpp'] = {'rmw_connext_cpp' : ['rosidl_typesupport_connext_cpp'] }

but the first resource map doesn't work because I cannot store a list of string at rmw_connext_cpp.

Should I expect to be able to do this?

I have a workaround in 12b9777.

@dirk-thomas
Copy link
Member

Within one resource file you can story any kind of content. If you choose to store a CMake list in it that will be exactly what you will get back out.

@jacquelinekay
Copy link
Contributor Author

I see, so if I expect an entry to store multiple strings, I should initialize it and append to it as a list.

@jacquelinekay
Copy link
Contributor Author

I fixed the index entries in this pull request so that they all can store a list of strings in b7d7897

@dirk-thomas
Copy link
Member

Instead of creating new macros for each language why not have a single function with a language parameter?

list(LENGTH arg arg_length)
if(${arg_length} LESS 1)
message(FATAL_ERROR
"register_rmw_implementation() called with invalid input: '${arg}'")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line doesn't need quoting imo since the value is already clearly separated by the colon (and can't be empty).

The error message should print the unmodified arg (without the colon being replaced already). Otherwise the user might wonder where the change is coming from.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list(LENGTH arg arg_length)
if(${arg_length} LESS 1)
message(FATAL_ERROR
"register_rmw_implementation() called with invalid input: ${ARGN}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't tell the user which argument was the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it--I'm saving the argument with colons in it now: 18a7e1d

# :type rmw_implementation: string
# :param LANGUAGE: Optional flag for the language of the type support to get.
# If language is omitted, type supports for all packages are returned.
Copy link
Member

@dirk-thomas dirk-thomas Apr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be: "If LANGUAGE is omitted, type supports for all languages are returned."?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@jacquelinekay
Copy link
Contributor Author

Latest CI:

http://ci.ros2.org/job/ci_linux/1232/
http://ci.ros2.org/job/ci_osx/989/

Fixed the linter warnings and launched a Windows job (pending):
http://ci.ros2.org/job/ci_windows/1273

@jacquelinekay
Copy link
Contributor Author

Had to fix a rebase, the latest Windows job is clean as far as Windows CI goes: http://ci.ros2.org/job/ci_windows/1274

ament_index_get_resource(${var} "rmw_implementation" "${rmw_implementation}")
endmacro()
set(resource_type "rmw_typesupport")
if(NOT "${ARG_LANGUAGE}" STREQUAL "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs trailing spaces in both strings.

endmacro()
foreach(arg_raw ${ARGN})
# replace colon with semicolon to turn into a list
string(REGEX REPLACE ":" ";" arg "${arg_raw}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is not a regex a simple string replace is sufficient.

@jacquelinekay
Copy link
Contributor Author

bump... any pending comments on this set of PRs?

@dirk-thomas
Copy link
Member

Beside mini comments on some of the related PRs this LGTM.

@jacquelinekay jacquelinekay merged commit 206c543 into master Apr 29, 2016
@jacquelinekay jacquelinekay deleted the register_typesupport_language branch April 29, 2016 22:32
@jacquelinekay jacquelinekay removed the in review Waiting for review (Kanban column) label Apr 29, 2016
ahcorde pushed a commit to erlerobot/rmw that referenced this pull request Jun 2, 2016
Add macros for getting type support for a particular language for an rmw implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants