-
Notifications
You must be signed in to change notification settings - Fork 48
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
Port to ROS 2 #41
Port to ROS 2 #41
Conversation
Any changes required here? |
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.
The basic idea is fine, but there are some details that need fixing. Also, this will need to be rebased and conflicts fixed since the ros2
branch is branched off of melodic-devel, not indigo-devel.
@@ -21,13 +23,14 @@ | |||
<url type="bugtracker">https://github.com/ros/urdf_parser_py/issues</url> | |||
<url type="repository">https://github.com/ros/urdf_parser_py</url> | |||
|
|||
<buildtool_depend>catkin</buildtool_depend> | |||
<buildtool_depend>ament_cmake</buildtool_depend> |
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.
From what I can tell looking elsewhere, with a pure python package we don't need any buildtool_depend
line, so just remove this.
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.
This comment is still open.
setup.py
Outdated
) | ||
|
||
setup(**d) | ||
# from distutils.core import setup |
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.
Just delete all of this commented code.
'Topic :: Software Development', | ||
], | ||
description='Python implementation of the URDF parser.', | ||
license='Apache License, Version 2.0', |
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.
This has to stay BSD, since we aren't really changing the underlying code.
d = generate_distutils_setup( | ||
setup( | ||
name=package_name, | ||
version='0.3.3', |
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.
melodic-devel is currently on 0.4.0, but since this is going to be a new branch, we probably want to bump up to something like 1.0.0 here (and in the package.xml).
setup.py
Outdated
zip_safe=True, | ||
author='Víctor Mayoral Vilches', | ||
author_email='vmayoral@acutronicrobotics.com', | ||
maintainer='Víctor Mayoral Vilches', |
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 don't mind adding you as a maintainer, but if you want to be the maintainer I'd suggest that we add you to the package.xml as well. If you don't want to be a maintainer, you can just put my name/email here.
Fixes bug in xml_string method and updates rosdeps for ROS2
Remove non-ascii characters from setup config
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.
The biggest thing left to fix up here is the licensing question, which should go back to BSD. Besides that there are a few nits here and there.
(also, it needs to be rebased)
@@ -21,13 +23,14 @@ | |||
<url type="bugtracker">https://github.com/ros/urdf_parser_py/issues</url> | |||
<url type="repository">https://github.com/ros/urdf_parser_py</url> | |||
|
|||
<buildtool_depend>catkin</buildtool_depend> | |||
<buildtool_depend>ament_cmake</buildtool_depend> |
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.
This comment is still open.
if addHeader: | ||
xmlString = '<?xml version="1.0"?>\n' + xmlString |
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.
Any particular reason for this change? I think this can stay as-is.
Closing in favor of #53 |
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/the-moveit-2-journey-part-1-porting-and-understanding-moveit-core/8718/1 |
Note to maintainers: Needs to be merged in a new
ros2
branch.