basic ROS2 port by rhaschke · Pull Request #213 · ros/xacro

rhaschke · 2019-09-25T15:02:30Z

This is work-in-progress, consolidating and cleaning up #211 for final merging.
In contrast to previous porting attempts, this PR focusses on the core porting effort:

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 were more catkin and ROS1 artifacts in the source code.
Make all the unit tests pass again.

rhaschke · 2019-09-25T15:47:32Z

I managed to get Travis running the colcon build. However, there is a caveat:
colcon test will run the tests from some arbitrary working directory. But, I would like to run them from xacro's test folder. I didn't find any option to convince pytest to change the working directory. Hence, I'm manually running pytest like this:
source install/setup.bash; cd src/xacro/test; python ../setup.py test
@clalancette, @dirk-thomas: any suggestions?

dirk-thomas · 2019-09-25T15:59:44Z

I would like to run them from xacro's test folder. I didn't find any option to convince pytest to change the working directory.

Why? You should be able to write the tests in a way that it works the way it is.

rhaschke · 2019-09-25T16:10:41Z

Why? You should be able to write the tests in a way that it works the way it is.

Why should I modify all tests to include the relative folder name? This will introduce an (in my opinion) unneccessary deviation from the ROS1 source code.

rhaschke · 2019-09-25T16:14:30Z

Another open question to me is how to install (and test) a custom cmake extension. In catkin we had:

catkin_package(
  CFG_EXTRAS xacro-extras.cmake
)

Can a package use both build types, ament_python and ament_cmake?

dirk-thomas · 2019-09-25T17:00:43Z

Why should I modify all tests to include the relative folder name?

I don't know anything about this package and why you would need to include relative folder name. But it just works fine in all other ROS packages with the standard way tests are written. So I would be surprised if it doesn't for this package. Maybe you can elaborate what the exact problem is and what you have tried so far as well as what change works but you think is not appropriate.

In catkin we had: catkin_package(CFG_EXTRAS ...)

The same functionality exists in ament_cmake: ament_package(CONFIG_EXTRAS ...) (see API doc)

Can a package use both build types, ament_python and ament_cmake?

No, either it is being built using CMake or using Python setuptools.

rhaschke · 2019-09-25T17:24:33Z

Can a package use both build types, ament_python and ament_cmake?

No, either it is being built using CMake or using Python setuptools.

Looks like, I need to use ament_cmake then. How do I trigger python setuptools from cmake then? Is there an ament_python_setup() similar to catkin_python_setup()?

There is another testing issue: xacro uses files in its test folder (which, of course, is not installed) and accesses them (in catkin's devel space) via substitution args: $(find xacro)/test/<filename>.
However, as far as I understand, ament doesn't provide the concept of a devel space anymore. So, I cannot access the source folder via $(find xacro)/test anymore, right?

rhaschke · 2019-09-25T17:47:30Z

Why should I modify all tests to include the relative folder name?

I would be surprised if it doesn't for this package. Maybe you can elaborate what the exact problem is and what you have tried so far as well as what change works but you think is not appropriate.

I am just surprised why the xacro test cases should be adapted to work with a new build system. This shouldn't be necessary - tests should be agnostic to the actual build system used.

The situation is as follows:
The test cases access files relative to the test folder. In ROS1 / catkin, the tests were run from this test folder, i.e. the working directory was the test folder, and the files were found correctly. Now, with colcon, the tests are run from the root folder of the package, and - obviously - the files are not found anymore. Rather, all file names in my tests need to be prefixed with test/ to found again.
If I am running pytest from the test folder - as before - everything works as usual.

Hence, I was asking for an option to change the working directory for running certain test cases.
Actually, I would expect pytest to support such a feature: As pytest automatically finds tests, it should be possible to specify a separate working directory for each of them, for example with corresponding .cfg files.

dirk-thomas · 2019-09-25T18:08:32Z

How do I trigger python setuptools from cmake then? Is there an ament_python_setup() similar to catkin_python_setup()?

The same as in ROS 1 there is no functionality to actually invoke the setup.py file from CMake. The catkin_python_setup() function looked for Python packages and modules to be installed but e.g. was not recognizing entry points and other features of setuptools. In ROS 2 you can install Python packages ([1]) and/or Python modules ([2]) directly without a setup.py file.

I am just surprised why the xacro test cases should be adapted to work with a new build system. This shouldn't be necessary - tests should be agnostic to the actual build system used.

This is not related to the build system (ament_cmake) but to the way packages are found. In ROS 1 a package was discovered by its package.xml file which required crawling the file system. It also considered the source space when doing so which is how $(find xacro) was able to find the test directory in the package sources. In ROS 2 it is not crawling of the file system since that part was notoriously slow. Instead it looks up packages and their prefix path based in the resource index. An example for inspiration how this can be done: https://github.com/ros2/rclcpp/blob/7728d8a38c19c3f3daec3691791cde3088fe0910/rclcpp_components/CMakeLists.txt#L77-L89

If you don't want to update the tests you could also create a "fake" resource index in the build space and copy/symlink the test directory there. Then the lookup would just the same as before.

Another option would be to just generate the paths at CMake time and insert absolute paths into your test. Either by using something like configure_file() or setting environment variables which are then used by the test. The former being easier to reproduce by users though are a bit more effort.

the tests are run from the root folder of the package, and - obviously - the files are not found anymore. Rather, all file names in my tests need to be prefixed with test/ to found again.

Yes, that is the standard way of doing it - as for any non-ROS Python package.

rhaschke · 2019-09-25T23:43:27Z

@clalancette, @gonzodepedro This is ready for review. It realizes a basic ROS2 port.

All unit tests are passing - requiring minimal changes to the code and tests.
flake8 and pep257 cleanup changes are explicitly not (yet) included
These cleanups as well as python2 cleanup, I would like to apply for the Noetic branch already and then forward-port to Dashing via merging.

In principle, we could go for a release of this. What is the procedure to (bloom?) release into ROS2?
Which release version should we use? I think 1.14.x should be reserved for the upcoming Noetic distro.

- Removed CMakeLists.txt - Removed wrapper scripts: xacro.py and scripts/xacro - Moved source contents from "src/xacro" to "xacro" - Modifed package.xml and setup.py according to ROS2 requirements - Added linters: test_copyright, test_flake8, test_pep257 - Configure completion hook

rhaschke · 2019-09-26T07:24:47Z

In ROS 2 it is not crawling of the file system since that part was notoriously slow. Instead, it looks up packages and their prefix path based in the resource index.

@dirk-thomas, probably I'm missing something, but in my point of view, searching ament_index is crawling the file system as well. However, I agree, that this is faster than in ROS1.

clalancette · 2019-09-26T12:13:51Z

In principle, we could go for a release of this. What is the procedure to (bloom?) release into ROS2?
Which release version should we use? I think 1.14.x should be reserved for the upcoming Noetic distro.

@rhaschke I need to sync with @gonzopedro later today, but here's what I think we should do:

Get this PR reviewed and finished
Rebase Ros2 port #211 on top of this one, and get that one reviewed and finished
Figure out whether Changed in favour of package_shared_directory #212 applies anymore; either close it or apply it
Do a release based on all of that

As far as the release number, for ROS 2 we've been generally bumping the major number when we do the initial release. So I'd suggest that the first release from the dashing branch becomes 2.0.0, and the 1.x numbers are reserved for ROS 1 releases.

Does that all make sense to you?

rhaschke · 2019-09-26T13:34:36Z

@clalancette I coarsely agree with you. This is my refined todo list:

Get this PR reviewed. Could you have a look?
Cleanup deprecated stuff (Noetic branch: remove deprecated stuff #214). Are you willing to review this as well?
Remove python2 compatibility stuff (my next step)
Apply Ros2 port #211's linting changes (will do this after the first two steps)
Release as 2.0.0

Figure out whether #212 applies anymore; either close it or apply it.

It's already part of this PR. However, I merged it into #211 as well.

dirk-thomas · 2019-09-26T14:45:12Z

searching ament_index is crawling the file system as well.

Sure, technically it still has to list the (non-recursive) files in a known directory. In ROS 1 it needs to crawl a base path recursively to find package manifest files which is orders of magnitude slower.

clalancette · 2019-09-26T15:22:55Z

@clalancette I coarsely agree with you. This is my refined todo list:

Yep, that list sounds good to me. I'll do a review of this PR today.

clalancette

The only comment I have revolves around the setup for travis. Otherwise, this looks good to me.

clalancette · 2019-09-26T18:59:27Z

.travis.yml

+  - sudo sh -c 'echo "deb http://packages.ros.org/ros2/ubuntu `lsb_release -cs` main" > /etc/apt/sources.list.d/ros2-latest.list'
  - sudo apt -qq update
-  - sudo apt -qq install -y ros-kinetic-catkin ros-kinetic-roslaunch ros-kinetic-rostest ros-kinetic-roslint
+  - sudo apt -qq install ros-ardent-ament-index-python ros-ardent-ament-cmake ros-ardent-ament-lint-auto ros-ardent-ament-cmake-pytest


We shouldn't be using ardent, it is long out of support.

Instead I'd recommend making an environment variable at the top at pointing it to dashing:

ROS_DISTRO=dashing

Then change all of these lines to use the environment variable instead.

Event better, you could switch this thing to use rosdep instead, which will generate and install the dependencies based on the package.xml. That's a bunch more work, so I won't insist on it here, but it would be good followup work.

Using rosdep is a great idea. Unfortunately, we need to stick with ardent, because that's the only ROS2 version supported in Xenial (and Xenial is the "latest" Ubuntu distro supported by Travis).
Of course, we could run a docker container, but I would like to avoid this.

@clalancette, I implemented the rosdep-based installation. However, rosdep doesn't know about ardent anymore (see Travis).
I'm afraid we are stuck to ardent and manual installation.
We should enable the ROS build farm for CI testing as well when releasing.

I'm pretty sure Travis supports Bionic: https://docs.travis-ci.com/user/reference/bionic/

Thanks! I didn't look for a while...

test_pr2() and test_substitution_args_find() relied on a devel space layout to resolve $(find xacro) to the source folder. Creating a dummy ament_index and a link from there to the source's test folder, correctly resolves $(find xacro) again. Using os.path.realpath() to compare canonical path names, fixes test_substitution_args_find()

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

@clalancette, @jacobperron: I think I finished all prerequisites for a ROS2 release.
Would be great, if you could have a final look.
@clalancette: Is the procedure for releasing the same as for ROS1 packages?

jacobperron · 2019-09-27T00:38:02Z

setup.py

-    packages=['xacro'],
-    package_dir={'': 'src'},
-    scripts=['scripts/xacro']
+setup(


I may be mistaken, but I don't believe we need both a CMakeLists.txt and a setup.py.
I think we can remove setup.py given CMakeLists.txt contains the following commands related to installing the Python package and the script:

ament_python_install_package(xacro) install(PROGRAMS scripts/xacro DESTINATION bin)

You are right. But, given all the effort we have put into creating the setup.py, I didn't want to delete this file again.

clalancette · 2019-09-27T12:13:53Z

@rhaschke Awesome, thanks for merging this. Yes, the procedure for releasing for ROS 2 is the same as ROS 1; source release (catkin_prepare_changelog/catkin_prepare_release) followed by a bloom-release. The only thing to be careful of is that you need an up-to-date bloom-release, at least version 0.8.0.

rhaschke · 2019-09-27T12:19:01Z

I think this could be released into both, crystal and dashing. Do you agree?

clalancette · 2019-09-27T12:20:36Z

I think this could be released into both, crystal and dashing. Do you agree?

Yes, good idea. The branch name is slightly odd for that, but it really isn't a big deal. Thanks!

rhaschke · 2019-09-28T04:47:08Z

Releases prepared: 🎉
Crystal: ros/rosdistro#22434
Dashing: ros/rosdistro#22435

rhaschke mentioned this pull request Sep 25, 2019

Ros2 port #211

Closed

rhaschke force-pushed the dashing-devel branch from 11fec1a to 355cea8 Compare September 25, 2019 15:40

rhaschke force-pushed the dashing-devel branch 4 times, most recently from 501e833 to 742e961 Compare September 25, 2019 23:28

rhaschke marked this pull request as ready for review September 25, 2019 23:35

vandanamandlik and others added 3 commits September 26, 2019 09:08

use ament_cmake

506c338

remove catkin artifacts

4c0d8fb

rhaschke force-pushed the dashing-devel branch from 742e961 to b98279e Compare September 26, 2019 07:25

clalancette reviewed Sep 26, 2019

View reviewed changes

rhaschke force-pushed the dashing-devel branch 4 times, most recently from 2c0de2a to 168d001 Compare September 26, 2019 20:25

rhaschke force-pushed the dashing-devel branch 4 times, most recently from a8a4392 to e0c180b Compare September 26, 2019 22:02

jacobperron and others added 4 commits September 27, 2019 00:22

substition args

7310f9a

Travis: colcon build

7df4641

Travis: install dependencies via rosdep

4581aef

rhaschke force-pushed the dashing-devel branch from e0c180b to 08eeb4f Compare September 26, 2019 22:23

rhaschke merged commit 08eeb4f into ros:dashing-devel Sep 26, 2019

jacobperron reviewed Sep 27, 2019

View reviewed changes

rhaschke mentioned this pull request Mar 1, 2024

Resolve $(find ...) as a result of a substitution argument #339

Merged

Uh oh!

Conversation

rhaschke commented Sep 25, 2019

Uh oh!

rhaschke commented Sep 25, 2019

Uh oh!

dirk-thomas commented Sep 25, 2019

Uh oh!

rhaschke commented Sep 25, 2019

Uh oh!

rhaschke commented Sep 25, 2019

Uh oh!

dirk-thomas commented Sep 25, 2019

Uh oh!

rhaschke commented Sep 25, 2019

Uh oh!

rhaschke commented Sep 25, 2019

Uh oh!

dirk-thomas commented Sep 25, 2019

Uh oh!

rhaschke commented Sep 25, 2019

Uh oh!

rhaschke commented Sep 26, 2019

Uh oh!

clalancette commented Sep 26, 2019

Uh oh!

rhaschke commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dirk-thomas commented Sep 26, 2019

Uh oh!

clalancette commented Sep 26, 2019

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

clalancette Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

rhaschke Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

rhaschke Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

jacobperron Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

rhaschke Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

rhaschke commented Sep 26, 2019

Uh oh!

jacobperron Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

rhaschke Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

clalancette commented Sep 27, 2019

Uh oh!

rhaschke commented Sep 27, 2019

Uh oh!

clalancette commented Sep 27, 2019

Uh oh!

rhaschke commented Sep 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rhaschke commented Sep 26, 2019 •

edited

Loading