colcon / colcon-ros

Extension for colcon to support ROS packages
http://colcon.readthedocs.io
Apache License 2.0
13 stars 26 forks source link

Ignore CATKIN_IGNORE paths only when build_type is catkin (same for AMENT_IGNORE) #127

Open janstrohbeck opened 3 years ago

janstrohbeck commented 3 years ago

My use case: I have a shared source space in which both ROS1 and ROS2 packages reside (the migration to ROS2 is ongoing). Now I want to build the ROS1 packages using catkin_tools, same as before, and the ROS2 packages using colcon. To do this, I place CATKIN_IGNORE files in the ROS2 packages, and COLCON_IGNORE files in the ROS1 packages.

Problem: colcon-ros ignores the ROS2 packages due to an existing CATKIN_IGNORE.

I think colcon should ignore ROS2 packages only when there is a COLCON_IGNORE file present (or AMENT_IGNORE), and respect CATKIN_IGNORE only when the build_type is catkin, as indicated by the package.xml. This PR implements this behavior.

janstrohbeck commented 3 years ago

Not sure why the CI fails, test/test_flake8.py does not report errors for me locally. Is the DeprecationWarning causing the failure?

cottsay commented 2 years ago

FWIW, the current behavior prevents the parsing of the manifest in a directory blocked by a *_IGNORE file. If I had a WIP package that had an invalid manifest, this change would result in a parsing warning even if I dropped a COLCON_IGNORE file in the directory.

I'm not sure that's a common situation, but I thought I'd mention it.

wodtko commented 2 years ago

By the change, CATKIN_IGNORE or AMENT_IGNORE files are only considered, if the build type matches catkin/ament. The handling of COLCON_IGNORE files is not altered.

DavidATapia commented 2 years ago

The author's outlined use-case seems in some ways like it should really be the expected behavior for colcon build -- and not instead just assume that colcon will be used to compile both ROS1 and ROS2 source packages which are co-mingled in the same workspace.

If colcon's expected behavior in this case seems ambiguous, then I think it makes sense to at least allow for a CLI way to remove colcon-ros to permit distinct ROS1 & ROS2 package exclusion, depending on the running build system. colcon-ros's inclusion is defined here: https://github.com/colcon/colcon-common-extensions/blob/master/setup.cfg#L46

If this PR isn't going to be merged, then is there a command-line way to remove colcon-ros package? Will the removal of this package have other unintended negative effects (besides ignoring the *_IGNORE directive)?

@dirk-thomas