Always prepend with a trailing separator (#254)

* Always prepend with a trailing separator

This resolves an issue where environment variables can collid if modified by both .dsv files and scripts.
For more detail see ament/ament_package#103

The solution is to always prepend, which will avoid overwriting a variable if previously set by a script.
Then we introduce a cleanup command for each variable prepended to in order to remove a possible trailing separator.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add batch command for trailing separator cleanup

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Address review

* Rename operation to "remove trailing separator"
* Make the format string definition optional
* Simplify logic to remove a trailing separator if it exists (not caring about the last value)

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Minor cleanup

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Declare and document FORMAT_STR_REMOVE_TRAILING_SEPARATOR

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Simplify template logic

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Print cleanup commands directly instead of maintain list

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add comment explaining change

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Only cleanup for variables initially set by the Python script

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix lint and clarify expand documentation

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

colcon_core/shell/__init__.py

@@ -88,6 +88,17 @@ class ShellExtensionPoint:
     This attribute must be defined in a subclass.
     """
     FORMAT_STR_INVOKE_SCRIPT = None
+    """
+    The format string to remove a trailing separator.
+
+    When prepending to an environment variable, a trailing separator will be
+    left behind if the variable was not set previously.
+    This command is used to cleanup the trailing separarator.
+
+    It must have the placeholder {name} for the environment variable name.
+    This attribute is optionally defined in subclasses.
+    """
+    FORMAT_STR_REMOVE_TRAILING_SEPARATOR = None
 
     def get_file_extensions(self):
         """

colcon_core/shell/bat.py

@@ -26,6 +26,8 @@ class BatShell(ShellExtensionPoint):
     FORMAT_STR_USE_ENV_VAR = '%{name}%'
     FORMAT_STR_INVOKE_SCRIPT = 'call:_colcon_prefix_bat_call_script ' \
         '"{script_path}"'
+    FORMAT_STR_REMOVE_TRAILING_SEPARATOR = 'if "%{name}:~-1%==";" ' \
+        'set {name}=%{name}:~0,-1%'
 
     def __init__(self):  # noqa: D107
         super().__init__()

colcon_core/shell/sh.py

@@ -26,6 +26,8 @@ class ShShell(ShellExtensionPoint):
     FORMAT_STR_USE_ENV_VAR = '${name}'
     FORMAT_STR_INVOKE_SCRIPT = 'COLCON_CURRENT_PREFIX="{prefix}" ' \
         '_colcon_prefix_sh_source_script "{script_path}"'
+    FORMAT_STR_REMOVE_TRAILING_SEPARATOR = 'if test "$(echo -n ${name} | ' \
+        'tail -c 1)" = ":" ; then; export {name}=${{{name}%?}} ; fi'
 
     def __init__(self):  # noqa: D107
         super().__init__()

colcon_core/shell/template/prefix_util.py.em

@@ -16,6 +16,7 @@ FORMAT_STR_SET_ENV_VAR = '@(shell_extension.FORMAT_STR_SET_ENV_VAR)'
 FORMAT_STR_USE_ENV_VAR = '@(shell_extension.FORMAT_STR_USE_ENV_VAR)'
 @{assert shell_extension.FORMAT_STR_INVOKE_SCRIPT is not None}@
 FORMAT_STR_INVOKE_SCRIPT = '@(shell_extension.FORMAT_STR_INVOKE_SCRIPT)'
+FORMAT_STR_REMOVE_TRAILING_SEPARATOR = '@(shell_extension.FORMAT_STR_REMOVE_TRAILING_SEPARATOR)'
 
 DSV_TYPE_PREPEND_NON_DUPLICATE = 'prepend-non-duplicate'
 DSV_TYPE_PREPEND_NON_DUPLICATE_IF_EXISTS = 'prepend-non-duplicate-if-exists'
@@ -56,6 +57,9 @@ def main(argv=sys.argv[1:]):  # noqa: D103
         ):
             print(line)
 
+    for line in _remove_trailing_separators():
+        print(line)
+
 
 def get_packages(prefix_path, merged_install):
     """
@@ -295,11 +299,10 @@ def _prepend_unique_value(name, value):
             env_state[name] = set()
         if os.environ.get(name):
             env_state[name] = set(os.environ[name].split(os.pathsep))
-    if not env_state[name]:
-        extend = ''
-    else:
-        extend = os.pathsep + FORMAT_STR_USE_ENV_VAR.format_map(
-            {'name': name})
+    # prepend even if the variable has not been set yet, in case a shell script sets the
+    # same variable without the knowledge of this Python script.
+    # later _remove_trailing_separators() will cleanup any unintentional trailing separator
+    extend = os.pathsep + FORMAT_STR_USE_ENV_VAR.format_map({'name': name})
     line = FORMAT_STR_SET_ENV_VAR.format_map(
         {'name': name, 'value': value + extend})
     if value not in env_state[name]:
@@ -311,6 +314,23 @@ def _prepend_unique_value(name, value):
     return [line]
 
 
+# generate commands for removing prepended underscores
+def _remove_trailing_separators():
+    # do nothing if the shell extension does not implement the logic
+    if FORMAT_STR_REMOVE_TRAILING_SEPARATOR is None:
+        return []
+
+    global env_state
+    commands = []
+    for name in env_state:
+        # skip variables that already had values before this script started prepending
+        if name in os.environ:
+            continue
+        commands += [FORMAT_STR_REMOVE_TRAILING_SEPARATOR.format_map(
+            {'name': name})]
+    return commands
+
+
 def _set(name, value):
     global env_state
     env_state[name] = value