-
Notifications
You must be signed in to change notification settings - Fork 92
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
Update rosidl_generator_java for Dashing compatibility #66
Conversation
Supporting ROS Dashing or later. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Accidentally broken during update. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
This remove some compile warnings. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Though Java supports unsigned values, it doesn't support unsigned literals (ie. literals that are larger than the max signed value). As a workaround, we can convert the literal to it's negative equivalent. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Otherwise the compiler complains about potential loss of data. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving myself some comments so I don't forget.
rosidl_generator_java/cmake/rosidl_generator_java_generate_interfaces.cmake
Outdated
Show resolved
Hide resolved
rosidl_generator_java/cmake/rosidl_generator_java_generate_interfaces.cmake
Outdated
Show resolved
Hide resolved
<test_depend>rosidl_cmake</test_depend> | ||
<test_depend>rosidl_parser</test_depend> | ||
<!-- Duplicated from rosidl_default_generator in order to avoid a circular dependency. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most (if not all) of the rosidl dependencies below can be removed. I'll give it a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jacobperron were you able to remove the rest of the dependencies? Does it still work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't seem to work. Needs more investigation, but leaving these dependencies here for now is okay.
@jacobperron thank you so much for your PR! I'm thinking of creating a dashing branch so that all the PRs can go in there and then merge the big one into master. What do you think? I'll review your changes shortly, give me a couple of days. Thanks! |
Yeah, a dashing branch makes sense if you want to review/merge my Dashing updates separately without breaking master 👍 |
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron I've created the dashing branch and changed the base for this pull request. |
@@ -0,0 +1,18 @@ | |||
/* Copyright 2019 Open Source Robotics Foundation, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be ok with OSRF if the copyright notice is the same as the rest of the files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this specific file it should no problem to use the same copyright notice since the actual code snippet is minimal / trivial. In other case where the contributions is significant I think the copyright notice should reflect the entity providing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In rviz we would also have multiple copyright claims when there were large changes, e.g.:
@jacobperron sorry for the slow response, just a couple of comments but overall looks great. Thanks! |
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@esteve Friendly ping. I think I've answered your comments. Let me know if you'd like me to change the copyright line for the small file(s); but I think it makes sense to include copyright lines for authors who make significant contributions. |
@esteve I have more changes ready to be reviewed, bringing rcljava up-to-date with Dashing and Eloquent. I'd prefer to get this PR merged before opening others that build on top of this (including enabling CI). I can probably recruit additional maintainers to help with reviews to lessen the burden. |
@jacobperron oops, I missed the latest changes. LGTM |
* Update rosidl_generator_java for new IDL pipeline Supporting ROS Dashing or later. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix name JNI name mangling Accidentally broken during update. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Remove java compile flags This remove some compile warnings. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Refactor to support services Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Wide string support Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Avoid duplicate includes Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Use test_interface_files Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Support for actions Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add suffix for long literals Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Handle unsigned literals Though Java supports unsigned values, it doesn't support unsigned literals (ie. literals that are larger than the max signed value). As a workaround, we can convert the literal to it's negative equivalent. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix escape string function Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Use cast to workaround integer literals Otherwise the compiler complains about potential loss of data. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Minor refactor Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Remove TODO Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Update rosidl_generator_java for new IDL pipeline Supporting ROS Dashing or later. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix name JNI name mangling Accidentally broken during update. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Remove java compile flags This remove some compile warnings. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Refactor to support services Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Wide string support Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Avoid duplicate includes Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Use test_interface_files Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Support for actions Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add suffix for long literals Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Handle unsigned literals Though Java supports unsigned values, it doesn't support unsigned literals (ie. literals that are larger than the max signed value). As a workaround, we can convert the literal to it's negative equivalent. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix escape string function Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Use cast to workaround integer literals Otherwise the compiler complains about potential loss of data. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Minor refactor Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Remove TODO Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Update rosidl_generator_java for new IDL pipeline Supporting ROS Dashing or later. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix name JNI name mangling Accidentally broken during update. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Remove java compile flags This remove some compile warnings. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Refactor to support services Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Wide string support Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Avoid duplicate includes Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Use test_interface_files Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Support for actions Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add suffix for long literals Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Handle unsigned literals Though Java supports unsigned values, it doesn't support unsigned literals (ie. literals that are larger than the max signed value). As a workaround, we can convert the literal to it's negative equivalent. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix escape string function Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Use cast to workaround integer literals Otherwise the compiler complains about potential loss of data. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Minor refactor Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Remove TODO Signed-off-by: Jacob Perron <jacob@openrobotics.org>
These changes enable compiling
rosidl_generator_java
(and other interfaces packages like test_msgs) for Dashing.Changes include:
I plan to follow-up these changes with updates to
rcljava
, so that it too will compile for Dashing.@esteve This is the first of several updates I plan to make related ROS 2 Java support.
Let me know if you'd like to continue to host the primary repository or if you think it would be better to move this to the ros2 organization.
I can assist in maintenance in either case.