colcon / colcon-ros

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

[question] colcon handling of CATKIN_DEVEL_PREFIX #119

Open tylerjw opened 3 years ago

tylerjw commented 3 years ago

What does colcon do with the cmake variable CATKIN_DEVEL_PREFIX?

These lines in moveit use it:

https://github.com/ros-planning/moveit/blob/ac55f9e252fb24321c4066d71bca4f447b55d961/moveit_ros/planning_interface/py_bindings_tools/CMakeLists.txt#L12-L16

and as a result tests that use the python bindings for the planning interface fail with an error about not being able to find the symbol _moveit_roscpp_initializer.

I know colcon doesn't have the concept of a devel space so how is this variable handled when building a catkin project?

dirk-thomas commented 3 years ago

Since colcon always installs a CMake package and therefore also catkin packages it doesn't do anything with CATKIN_DEVEL_PREFIX.

The default value within catkin points to a subdirectory devel within the build directory (https://github.com/ros/catkin/blob/a9672d78ec483c3a991a051c365da329fceaa9f2/cmake/all.cmake#L15).

asherikov commented 3 years ago

The problem is that this variable has been used in some packages to detect if the package is built with catkin or not, e.g., https://answers.ros.org/question/230877/optionally-build-a-package-with-catkin/, https://github.com/PX4/PX4-Autopilot/blob/master/CMakeLists.txt#L220. CATKIN_DEVEL_PREFIX can be set manually, but may be it should be set by colcon when building catkin packages to replicate behavior, or mentioned explicitly in migration section of documentation.

dirk-thomas commented 3 years ago

I don't think the CMake variable CATKIN_DEVEL_PREFIX should be used to (and can't be used to) detect which tool invoked cmake. A user might as well pass a a custom devel prefix even though invoking cmake manually.

Maybe a package wants to distinguish if the build was invoked through catkin or colcon. Trying to match the behavior as suggested would even go against that.

Imo the only clean and correct way to detect if a specific tool was used to build a package is for the tool to set an environment variable were it discloses its own presence (e.g. CATKIN_TOOLS=1, COLCON=1).

asherikov commented 3 years ago

Well, yes, but this is a different topic. My point is that there are packages that do this, and I think that colcon users in general expect the same behavior as catkin_tools, this is why it should at least be indicated in the manual.

Speaking of your suggestion: package developers may be more interested in knowing if the package is built in ROS context or as a standalone package and don't care much about the particular build tool -- a more generic convention would be preferable, e.g, ROS_BUILD_TOOL=colcon (catkin_tools, etc).

tfoote commented 3 years ago

Unfortunately the definition of what is a "ROS" context is application and environment specific. And the build tool is not a proxy for that either. For example the debian packaging pipeline which builds the ROS packages for Ubuntu don't use any of the buildtools but invoke pure cmake. And on the other side. People use colcon (and catkin or catkin_tools) for things that have nothing to do with ROS.

Detecting an implementation specific flag, should be considered as detecting that specific implementation. The logic is incorrect to assume that only that specific implementation of the buildtool can be used. Presumably that detector is being used as a proxy for some other detector which changes behavior of the build. It would be cleaner to actually detect the specific requirements in the build system(do you need some ros dependency on your path? cmake has lots of find logic for this explicitly) or set a flag to change the desired behavior explicitly instead of relying on a correlated heuristic such as detecting the build tool. We've designed things such that the build of a package should be fully captured by the build system and the build tool is just a developer convenience. The distinction is layed out here: https://design.ros2.org/articles/build_tool.html

asherikov commented 3 years ago

Ok, there are several parts in this conversation:

  1. [CATKIN_DEVEL_PREFIX] The first one is related to the existing practice of using CATKIN_DEVEL_PREFIX to detect if packages are built in 'ROS context' or no. I see your point regarding validity of such check, but it is necessary to make users aware of the absense of this variable in colcon builds and pitfalls of this practice.
  2. [detection of 'ROS context'] The fact that people use CATKIN_DEVEL_PREFIX this way indicates that there is certain demand for such a feature. I've experienced this myself, but ended up with creating wrapper packages, which are annoying to maintain by the way. Package developer might just want to know if, e.g., some launch scripts should be installed or not. How to control such behavior? A specific flag like MYPACKAGE_INSTALL_ROS_SCRIPTS is hardly an option: I don't see how it can be specified in the packaging pipeline, also somebody is inevitably going to come up with something like ENABLE_ROS which is going to collide with other stuff. For this reason I am advocating for a global flag -- it can be set by default in the pipeline, and optionally in development workspace.
  3. [detecting build tool] I did not express myself in the right way above -- what I had in mind is a variable which would indicate if package can use catkin or ament_cmake. This should be possible to handle with find_package() functionality indeed. The particular build tool such as colcon/catkin_tools should hardly be important.