Fix the default value for create_hook_set_value()

[meyerj]

The default value is supposed to be the current prefix only, COLCON_CURRENT_PREFIX, and not the concatenated list of all prefixes, COLCON_PREFIX_PATH. This fixes a bug(?) introduced in #304.

As originally commented at colcon/colcon-ros#95 (comment):

With the patches

• standardize behavior when passing an empty string colcon-core#304 and
• standardize behavior when passing an empty string colcon-bash#25

for the various shells the value '' passed to create_hook_set_value() is replaced by $COLCON_PREFIX_PATH, so the concatenated list of all colcon workspaces separted with colons. But before this patch the value was $COLCON_CURRENT_PREFIX, a single path only, and that is also what is required to set CATKIN_ENV_HOOK_WORKSPACE here to mimic the behavior of catkin. Only for 1st-level workspaces and if COLCON_PREFIX_PATH was initially empty the difference does not matter, because the values of COLCON_PREFIX_PATH and COLCON_CURRENT_PREFIX are the same.

Is this a bug? At least CATKIN_ENV_HOOK_WORKSPACE is not set in the same way as with catkin in a 2nd- or higher level workspace with catkin env-hooks when it is built with colcon. I am not sure what the implied behavior of passing an empty value for dsv is, but I guess it should have been $COLCON_CURRENT_PREFIX for colcon/colcon-core#304 and colcon/colcon-bash#25?

For a package named my_package with an env-hook named foo.sh that prints CATKIN_ENV_HOOK_WORKSPACE, built in /path/to/my/workspace with COLCON_PREFIX_PATH=/opt/ros/noetic:

$ set -x
$ . /path/to/my/workspace/install/setup.sh
[...]
++++ COLCON_CURRENT_PREFIX=/path/to/my/workspace/install
++++ _colcon_prefix_sh_source_script /path/to/my/workspace/install/share/my_package/hook/catkin_env_hook_workspace.sh
++++ '[' -f /path/to/my/workspace/install/share/my_package/hook/catkin_env_hook_workspace.sh ']'
++++ '[' -n '' ']'
++++ . /path/to/my/workspace/install/share/my_package/hook/catkin_env_hook_workspace.sh
++++ . /path/to/my/workspace/install/share/my_package/catkin_env_hook/foo.sh
+++++ echo 'CATKIN_ENV_HOOK_WORKSPACE = /path/to/my/workspace/install:/opt/ros/noetic'
CATKIN_ENV_HOOK_WORKSPACE = /path/to/my/workspace/install:/opt/ros/noetic

I am not sure what the implied behavior of passing an empty value for dsv is, but I guess it should have been $COLCON_CURRENT_PREFIX for colcon/colcon-core#304 and colcon/colcon-bash#25?

For dsv an empty value is replaced by the prefix here, so a single path only.

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: -0.09% ⚠️

Comparison is base (4e10411) 82.01% compared to head (b65c797) 81.93%.
Report is 15 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #549      +/-   ##
==========================================
- Coverage   82.01%   81.93%   -0.09%     
==========================================
  Files          62       65       +3     
  Lines        3642     3753     +111     
  Branches      705      724      +19     
==========================================
+ Hits         2987     3075      +88     
- Misses        603      622      +19     
- Partials       52       56       +4     

Files Changed	Coverage Δ
colcon_core/shell/__init__.py	98.61% <ø> (ø)
colcon_core/shell/bat.py	91.54% <0.00%> (ø)
colcon_core/shell/sh.py	91.54% <0.00%> (ø)

... and 13 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cottsay]

Sorry this has gone unnoticed for so long. I think your analysis is correct, and something needs to change here.

At first I convinced myself that the existing code was established far too long ago for us to change the behavior now, even if it isn't doing what colcon-ros needs it to do, but then I looked at the DSV shell's handling of an empty string and it seems to (indirectly) end up with COLCON_CURRENT_PREFIX.

Given that the current implementation is inconsistent, I think we can go ahead and change it to the behavior needed to support colcon-ros.

I'll ask, however, that you clarify the corresponding documentation for the parameter in colcon_core/shell/__init__.py.

[meyerj]

I'll ask, however, that you clarify the corresponding documentation for the parameter in colcon_core/shell/__init__.py.

Something like 61724cd?

[cottsay]

Thanks for your patience!