colcon / colcon-ros

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

Fix the variable usage for CATKIN_ENV_HOOK_WORKSPACE #93

Closed seanyen closed 4 years ago

seanyen commented 4 years ago

This was manifesting when I tried to build ROS1 packages (I am using colcon to build noetic distro JFYI). And in my case, when colcon builds ros_environment, I saw catkin_env_hook_workspace.bat was generated in the install space and it has the following content:

:: generated from colcon_core/shell/template/hook_set_value.bat.em
@echo off

set "CATKIN_ENV_HOOK_WORKSPACE=$COLCON_CURRENT_PREFIX"

And subsequently, CATKIN_ENV_HOOK_WORKSPACE gets passed to catkin env-hook, for example, 1.ros_etc_dir.bat, and the hook generated variable with wrong substitute:

C:\>set ROS
ROS_DISTRO=noetic
ROS_ETC_DIR=$COLCON_CURRENT_PREFIX/etc/ros
ROS_MASTER_URI=http://localhost:11311
ROS_PACKAGE_PATH=c:\opt\ros\noetic\x64\share
ROS_PYTHON_VERSION=3
ROS_ROOT=$COLCON_CURRENT_PREFIX/share/ros
ROS_VERSION=1

I think my solution may not be an elegant one, but I just wanted to surface where the issue comes from. Any feedback is welcome.

dirk-thomas commented 4 years ago

After some consideration and reviewing the behavior of the current functions provided by the extension create_hook_set_value() is already providing the necessary behavior - but only for the dsv extension. When passing an empty value (which are interpreted as relative paths) that already implies that the prefix path is being used. So I came up withe the following set of changes:

@seanyen Can you please try these proposed changes and comment if they work for your use case.

dirk-thomas commented 4 years ago

@seanyen Friendly ping.

seanyen commented 4 years ago

@dirk-thomas Thanks for the reminder. I tried your rev and it works. I can see the path propagated to the environment:

ROS_DISTRO=noetic
ROS_ETC_DIR=f:\workspace\n_ws\install\/etc/ros
ROS_PACKAGE_PATH=f:\workspace\n_ws\install\share
ROS_PYTHON_VERSION=3
ROS_VERSION=1

There are some funny slash in the ROS_ETC_DIR but colcon does the job. And I think there are some path normalization work needs to be done in the ros_environment side.

dirk-thomas commented 4 years ago

Thanks for testing the proposed patches. Closing in favor of #95.