Ros2 port by gonzodepedro · Pull Request #211 · ros/xacro

gonzodepedro · 2019-09-17T17:51:26Z

This PR brings together changes made by @vandanamandlik and @jacobperron.

- Moved contents from "src/xacro/" folder to "xacro/" folder. - Renamed scripts folder to resource folder. - Modifed package.xml and setup.py according to ROS2 rules. - Deleted CMakeLists.txt

- Modified all python scripts as per ros2 coding guidelines. - Added substitution_args.py file, replacement for roslaunch's resolve_args() method, as it is not available in ROS2's ros2launch package. - Updated shebang line in all python scripts to use python3. - Resolved errors where some test cases were failing to find out some .xml files. (eg. include1.xml) - Added temporary workaround for rosgraph.names.load_mappings because rosgraph is not migrated to ROS2.

…, required for ament_copyright rule.

…test this package on bouncy.

- Added some more details in README.md file.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

clalancette

I've got a lot of things to fix in here. They fall into a few main categories:

Switch the license back to BSD everywhere
Be very careful about changing " -> '; it matters in some places
Get the dependencies correct
Miscellaneous fixes

I may have more on a subsequent review, but these are the major things I saw to start with.

clalancette · 2019-09-18T21:12:45Z

.travis.yml

 python:
-  - "2.7"
-  - "3.5"
+  # - "3.5"


We've been generally trying to keep Python 3.5 support for older Debian. So I'd suggest re-enabling this.

clalancette · 2019-09-18T21:13:29Z

.travis.yml

+  # - sudo sh -c 'echo "deb http://packages.ros.org/ros/ubuntu trusty main" > /etc/apt/sources.list.d/ros-latest.list'
+  # - wget http://packages.ros.org/ros.key -O - | sudo apt-key add -
+  # - sudo apt-get update
+  # - sudo apt-get install ros-indigo-catkin ros-indigo-roslaunch ros-indigo-rostest ros-indigo-roslint


Just kill off all of the commented lines above this.

clalancette · 2019-09-18T21:13:40Z

.travis.yml

+  - sudo apt update && sudo apt install curl
+  - curl http://repo.ros2.org/repos.key | sudo apt-key add -
+  - sudo sh -c 'echo "deb [arch=amd64,arm64] http://repo.ros2.org/ubuntu/main `lsb_release -cs` main" > /etc/apt/sources.list.d/ros2-latest.list'
+  - export ROS_DISTRO=ardent  # or ardent


Definitely switch this to Dashing.

clalancette · 2019-09-18T21:14:10Z

.travis.yml

+  - sudo -H python3 -m pip install -U setuptools

-before_script:
+# before_script:


You'll probably want to reenable the flake8 tests.

clalancette · 2019-09-18T21:14:28Z

.travis.yml

+  # - make
+  # - make run_tests
+  # - catkin_test_results --all ./
+  # - sudo make install


Kill off all of this commented-out code.

clalancette · 2019-09-18T21:46:42Z

xacro/xmlutils.py

@@ -1,38 +1,29 @@
+#!/usr/bin/env python3
+
+# Copyright 2018 Open Source Robotics Foundation, Inc.


BSD license.

clalancette · 2019-09-18T21:47:30Z

xacro/xmlutils.py


    for a_name in a_names:
-        writer.write(" %s=\"" % a_name)
+        writer.write(' %s=\"' % a_name)


We have to be careful here. I believe that we should get rid of the escape slash here.

clalancette · 2019-09-18T21:47:44Z

xacro/xmlutils.py

+        writer.write(' %s=\"' % a_name)
        xml.dom.minidom._write_data(writer, attrs[a_name].value)
-        writer.write("\"")
+        writer.write('\"')


I think we need to get rid of the escape slash here as well.

clalancette · 2019-09-18T21:48:08Z

xacro/substitution_args.py

@@ -0,0 +1,576 @@
+#!/usr/bin/env python3
+
+# Copyright 2018 Open Source Robotics Foundation, Inc.


BSD license.

This comment is still open.

clalancette · 2019-09-18T21:48:25Z

xacro/substitution_args.py

+
+
+try:
+    from cStringIO import StringIO  # Python 2.x


Remove Python 2 compatibility.

This comment is still open.

allenh1 · 2019-09-19T13:56:20Z

setup.py

@@ -1,13 +1,37 @@
-#!/usr/bin/env python
+#!/usr/bin/env python3


I'm not sure this #! line is needed, since it gets registered as a console script.

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

jacobperron · 2019-09-23T21:04:31Z

.travis.yml

+  - sudo -H python3 -m pip install -U setuptools

-before_script:
+ before_script:


I don't think this should be indented.

jacobperron · 2019-09-23T21:07:27Z

README.md

@@ -0,0 +1,43 @@
+This file describes the work done and steps to perform test on the xacro package.
+
+"https://github.com/vandanamandlik/xacro/tree/ros2"


I agree with @clalancette's comment above regarding refactoring the README.md.
In any case, we shouldn't be pointing to a fork of this repo.

jacobperron · 2019-09-23T21:12:26Z

setup.py

+    keywords=['ROS'],
+    classifiers=[
+        'Intended Audience :: Developers',
+        'License :: OSI Approved :: Apache Software License',


Suggested change

'License :: OSI Approved :: Apache Software License',

'License :: OSI Approved :: BSD License',

jacobperron · 2019-09-23T21:17:10Z

xacro/cli.py

@@ -0,0 +1,180 @@
+#!/usr/bin/env python3


I don't think the shebang line is necessary since this isn't intended to be executed as a script.

jacobperron · 2019-09-23T21:18:07Z

xacro/color.py

@@ -1,4 +1,5 @@
-#! /usr/bin/env python
+#!/usr/bin/env python3


Similar to my comment above, I think this line can be removed.

jacobperron · 2019-09-23T21:24:25Z

xacro/__init__.py

-    if s == '$(cwd)':
+    # TODO This is Temporary fix to return cwd() for 'find xacro' command.
+    # We can use resolve_args() when rospkg will be migrated to ROS2.
+    if s == '$(cwd)' or s == '$(find xacro)':


I guess this temporary fix isn't necessary since we're using ament to find packages now in substitution_args.py.

jacobperron · 2019-09-23T21:36:35Z

xacro/__init__.py

-        from rospkg.common import ResourceNotFound
-        return substitution_args.resolve_args(s, context=substitution_args_context, resolve_anon=False)
+        # TODO use following when roslaunch will be fully migrated to ROS2.
+        # from roslaunch import substitution_args


I'm not sure what's needed exactly to resolve this TODO, but I don't think depending on launch is an appropriate solution. It looks like substitution_args from roslaunch is already replicated in xacro, so maybe we can remove this TODO.

jacobperron · 2019-09-23T21:48:01Z

xacro/substitution_args.py

+
+import yaml
+
+from ament_index_python.packages import get_package_prefix


Following a discussion on a separate thread (kazuki0824#1 (comment)), I think it's better to default to using get_package_share_directory().

I agree. It is in PR #212

clalancette

This round of review still has a lot of comments (some still remaining from last time), but this is looking a lot better.

clalancette · 2019-09-23T21:16:47Z

README.md

@@ -0,0 +1,43 @@
+This file describes the work done and steps to perform test on the xacro package.


This comment is still open.

resource/xacro

test/test_copyright.py

test/test_flake8.py

test/test_pep257.py

clalancette · 2019-09-23T21:59:12Z

xacro/substitution_args.py

+
+import yaml
+
+from ament_index_python.packages import get_package_prefix


clalancette · 2019-09-23T21:59:21Z

xacro/substitution_args.py

+
+
+try:
+    from cStringIO import StringIO  # Python 2.x


This comment is still open.

clalancette · 2019-09-23T21:59:48Z

xacro/substitution_args.py

+
+
+def _eval_optenv(name, default=''):
+


This should probably get a pydoc comment.

clalancette · 2019-09-23T22:00:29Z

xacro/substitution_args.py

+
+
+def _eval_dirname(filename):
+


This could use a pydoc comment.

clalancette · 2019-09-23T22:01:36Z

xacro/substitution_args.py

+
+
+def convert_value(value, type_):
+        """


The indentation on this method is weird compared to the other ones in this file (and in this whole codebase). Fix this to be 4 spaces like the rest.

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

clalancette

There are still quite a few comments left-over from my last review. Please go through all of the open comments and address them.

clalancette · 2019-09-24T20:17:52Z

resource/xacro

@@ -1,4 +1,4 @@
-#! /usr/bin/env python
+


Nit: remove empty line at the top.

rhaschke · 2019-09-25T15:09:27Z

As I already pointed out in #203 (comment), I suggest to split this huge PR into independent components:

Porting to ROS2. This essentially includes:
- Port to colcon and apply required adaptions to setup.py, package.xml, etc.
- Adapt Travis config appropriately.
- Provide substitution args in a similar fashion as in ROS1.
  @jacobperron and @gonzodepedro have done some preliminary work into this direction.
  But there are more catkin and ROS1 artifacts in the source code.
- Make all the unit tests pass.

The first three points were nicely prepared by @vandanamandlik, @jacobperron, and @gonzodepedro. I'm working on the remaining issues (see #213).

The following improvments are independent from the ROS2 porting:

Formatting improvements to comply with flake8 and pep257
General source cleanup: Remove legacy processing. Remove python2 support.
I plan to do this cleanup for Noetic. These changes should be ported to ROS2 then.

As the present PR is a mixture of the first two items, I suggest to close it here and continue on #213.
@gonzodepedro, @clalancette do you agree?

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

clalancette · 2019-09-25T19:29:57Z

@gonzodepedro, @clalancette do you agree?

Only sort of. While I agree with the general principle, the ROS 2 ports have been sitting long enough and we've done enough iterations with this PR that I think it would be a waste now to throw this away. I'd much rather port your travis fixes into this PR and complete it here.

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

rhaschke · 2019-09-25T19:50:03Z

@clalancette, my motivation is primarily separation of concerns: The ROS2 port is a rather small fraction of this PR and should be decoupled from the python linting.
I'm not aiming to throw away the work done here on linting. Rather, I would like to resume this thread after having realized the basic ROS2 port.

rhaschke · 2019-09-25T19:57:39Z

Currently, this PR has a lot of unit test failures and nobody yet worked on this. So far, work focused on fixing flake8 and pep257 issues, which doesn't help us in porting to ROS2.
In #213, I am focusing on the latter...

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

rhaschke · 2019-09-26T19:52:59Z

Unfortunately, when rebasing #203 or #210 to latest melodic-devel, all changes to melodic-devel were overwritten, i.e. this PR lost some recent changes to melodic-devel.
Probably, it's faster to automatically redo the formatting changes than figuring out what is missing...

rhaschke · 2019-09-26T23:18:51Z

Manually transferred formatting changes (90fb2af)

vandanamandlik and others added 12 commits September 17, 2019 13:51

added setup.cfg

3d2dcc3

Modifed package structure as per ROS2 rules

9650da2

- Moved contents from "src/xacro/" folder to "xacro/" folder. - Renamed scripts folder to resource folder. - Modifed package.xml and setup.py according to ROS2 rules. - Deleted CMakeLists.txt

Added 3 cosmetics test cases- test_copyright, test_flak8 and test_pep257

590b194

Added files CONTRIBUTING.md and LICENSE (Apache License, Version 2.0)…

6d74eb5

…, required for ament_copyright rule.

Modified .travis.yml file for ROS2 colcon build.

2ab150a

Modified .travis.yml to start build for ros2 branch

d8c2d6c

Added ros2_migration_readme.md file which contain steps to build and …

b51b849

…test this package on bouncy.

- Renamed ros2_migration_readme.md with README.md.

308d544

- Added some more details in README.md file.

Replace rospack usage with ament_index_python

f096c15

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Cleanup

2d638bb

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Changes entry point

e02446b

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

This was referenced Sep 17, 2019

Ported to ROS2 Dashing #210

Closed

Ros2 #203

Closed

Changes to support ROS 2 #154

Closed

jacobperron self-requested a review September 17, 2019 18:02

gonzodepedro requested a review from clalancette September 17, 2019 18:37

gonzodepedro self-assigned this Sep 17, 2019

ros deleted a comment from gonzodepedro Sep 18, 2019

clalancette requested changes Sep 18, 2019

View reviewed changes

allenh1 reviewed Sep 19, 2019

View reviewed changes

Changes based on review

680c147

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro requested a review from clalancette September 23, 2019 20:39

jacobperron mentioned this pull request Sep 23, 2019

Replace rospack usage with ament_index_python kazuki0824/xacro#1

Closed

jacobperron reviewed Sep 23, 2019

View reviewed changes

clalancette requested changes Sep 23, 2019

View reviewed changes

Changes based on review

814c43a

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro requested a review from clalancette September 24, 2019 22:11

clalancette requested changes Sep 25, 2019

View reviewed changes

rhaschke mentioned this pull request Sep 25, 2019

basic ROS2 port #213

Merged

Changes based on review

29889b9

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

Changes based on review

7ac411e

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

Addresed comments in review

06d255e

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro requested a review from clalancette September 25, 2019 20:46

Changed in favour of package_shared_directory (#212)

2935246

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

rhaschke closed this Sep 26, 2019

rhaschke deleted the ros2-port branch September 26, 2019 23:20

IanTheEngineer mentioned this pull request Oct 1, 2019

[ros2] Debian ros-testing install fails to expand *any* xacro-macros #216

Closed

		@@ -1,38 +1,29 @@
		#!/usr/bin/env python3

		# Copyright 2018 Open Source Robotics Foundation, Inc.

		@@ -0,0 +1,576 @@
		#!/usr/bin/env python3

		# Copyright 2018 Open Source Robotics Foundation, Inc.

		@@ -1,13 +1,37 @@
		#!/usr/bin/env python
		#!/usr/bin/env python3

		@@ -0,0 +1,43 @@
		This file describes the work done and steps to perform test on the xacro package.

		"https://github.com/vandanamandlik/xacro/tree/ros2"

	'License :: OSI Approved :: Apache Software License',
	'License :: OSI Approved :: BSD License',


		import yaml

		from ament_index_python.packages import get_package_prefix



		def _eval_dirname(filename):

Uh oh!

Conversation

gonzodepedro commented Sep 17, 2019

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhaschke commented Sep 25, 2019