Repository: enthought/okonomiyaki

Issue:
Problem when setting up environment with User name beginning with $
A problem arises from the following sequence when setting up a runtime environment where the user name begins with $:

https://github.com/enthought/okonomiyaki/blob/master/okonomiyaki/runtimes/runtime_info.py#L111

https://github.com/enthought/okonomiyaki/blob/master/okonomiyaki/runtimes/runtime_info.py#L204

https://github.com/enthought/okonomiyaki/blob/master/okonomiyaki/utils/misc.py#L67

A sample traceback is shown here:
```
2018-03-29 14:25:01 ERROR    1156 Future-thread-1 [edm.utils.misc:154] All Installing runtime attempts failed
2018-03-29 14:25:01 ERROR    1156 Future-thread-1 [canopy.user_env.env_installer:207] Error installing runtime
Traceback (most recent call last):
  File "build\bdist.win32\egg\canopy\user_env\env_installer.py", line 205, in do_install
  File "build\bdist.win32\egg\canopy_platform\runtimes_manager.py", line 375, in install_runtime_from_file
  File "build\bdist.win32\egg\canopy_platform\runtimes_manager.py", line 576, in _install_runtime
  File "build\bdist.win32\egg\canopy_platform\runtimes_manager.py", line 715, in _unpack_with_progress
  File "build\bdist.win32\egg\canopy_platform\edm_api.py", line 95, in wrapper
  File "build\bdist.win32\egg\canopy_platform\edm_api.py", line 338, in install_runtime
  File "build\bdist.win32\egg\edm\core\environments_manager.py", line 371, in install
  File "build\bdist.win32\egg\edm\runtimes\runtime_installer.py", line 282, in install
  File "C:\Program Files\Enthought\Canopy32\App\appdata\canopy-2.1.8.3709.win-x86\Lib\site-packages\okonomiyaki\runtimes\runtime_info.py", line 32, in factory_from_metadata
    return runtime_info_from_metadata(metadata, prefix, name)
  File "C:\Program Files\Enthought\Canopy32\App\appdata\canopy-2.1.8.3709.win-x86\Lib\site-packages\okonomiyaki\runtimes\runtime_info.py", line 201, in runtime_info_from_metadata
    return klass._from_metadata(metadata, prefix, name)
  File "C:\Program Files\Enthought\Canopy32\App\appdata\canopy-2.1.8.3709.win-x86\Lib\site-packages\okonomiyaki\runtimes\runtime_info.py", line 100, in _from_metadata
    return cls(*cls._from_metadata_impl(metadata, prefix, name))
  File "C:\Program Files\Enthought\Canopy32\App\appdata\canopy-2.1.8.3709.win-x86\Lib\site-packages\okonomiyaki\runtimes\runtime_info.py", line 157, in _from_metadata_impl
    metadata, prefix, name
  File "C:\Program Files\Enthought\Canopy32\App\appdata\canopy-2.1.8.3709.win-x86\Lib\site-packages\okonomiyaki\runtimes\runtime_info.py", line 111, in _from_metadata_impl
    variables = _compute_variables(metadata, prefix, name)
  File "C:\Program Files\Enthought\Canopy32\App\appdata\canopy-2.1.8.3709.win-x86\Lib\site-packages\okonomiyaki\runtimes\runtime_info.py", line 215, in _compute_variables
    return substitute_variables(variables, variables)
  File "C:\Program Files\Enthought\Canopy32\App\appdata\canopy-2.1.8.3709.win-x86\Lib\site-packages\okonomiyaki\utils\misc.py", line 89, in substitute_variables
    ret = _resolve(d)
  File "C:\Program Files\Enthought\Canopy32\App\appdata\canopy-2.1.8.3709.win-x86\Lib\site-packages\okonomiyaki\utils\misc.py", line 86, in _resolve
    ret[k] = substitute_variable(v, local_vars)
  File "C:\Program Files\Enthought\Canopy32\App\appdata\canopy-2.1.8.3709.win-x86\Lib\site-packages\okonomiyaki\utils\misc.py", line 97, in substitute_variable
    return string.Template(v).substitute(local_vars)
  File "C:\Program Files\Enthought\Canopy32\App\appdata\canopy-2.1.8.3709.win-x86\lib\string.py", line 176, in substitute
    return self.pattern.sub(convert, self.template)
  File "C:\Program Files\Enthought\Canopy32\App\appdata\canopy-2.1.8.3709.win-x86\lib\string.py", line 166, in convert
    val = mapping[named]
KeyError: u'xxxxxxxx
```
The KeyError is generated when the user name starts with $, for example $xxxxxxxx.

The substitute_variable() function at https://github.com/enthought/okonomiyaki/blob/master/okonomiyaki/utils/misc.py#L96 uses ```string.Template().substitute()``` to perform Shell/Perl style variable substitution. In this case, a user name beginning with $ is treated as an environment variable and an attempt is made to substitute in the value, which then results in a KeyError since the user name is not present as an environment variable.

A simple way to address this would be to use ```string.Template.safe_substitute()```. As shown here: https://gist.github.com/mgrady3/9bf677e4eb84c479095eca477b68f5ae

However, it may be better to instead try and catch KeyErrors and perform some type of check to see where the substitution failed at. Insert some logic to allow paths with directories beginning with '$' to stay, but raise the error if there indeed should have been a substitution but this failed for some other reason.



Discussion:

Patch:
diff --git a/okonomiyaki/runtimes/runtime_info.py b/okonomiyaki/runtimes/runtime_info.py
index 5b20cdf..eb9a8e5 100644
--- a/okonomiyaki/runtimes/runtime_info.py
+++ b/okonomiyaki/runtimes/runtime_info.py
@@ -110,13 +110,16 @@ class IRuntimeInfoV1(IRuntimeInfo):
 
         variables = _compute_variables(metadata, prefix, name)
 
-        executable = substitute_variable(metadata.executable, variables)
+        executable = substitute_variable(
+            metadata.executable, variables, template='curly_braces_only'
+        )
 
         paths = tuple(
-            substitute_variable(path, variables) for path in metadata.paths
+            substitute_variable(path, variables, template='curly_braces_only')
+            for path in metadata.paths
         )
         post_install = tuple(
-            substitute_variable(part, variables)
+            substitute_variable(part, variables, template='curly_braces_only')
             for part in metadata.post_install
         )
 
@@ -157,9 +160,15 @@ class PythonRuntimeInfoV1(IRuntimeInfoV1):
             metadata, prefix, name
         )
         variables = _compute_variables(metadata, prefix, name)
-        scriptsdir = substitute_variable(metadata.scriptsdir, variables)
-        site_packages = substitute_variable(metadata.site_packages, variables)
-        python_tag = substitute_variable(metadata.python_tag, variables)
+        scriptsdir = substitute_variable(
+            metadata.scriptsdir, variables, template='curly_braces_only'
+        )
+        site_packages = substitute_variable(
+            metadata.site_packages, variables, template='curly_braces_only'
+        )
+        python_tag = substitute_variable(
+            metadata.python_tag, variables, template='curly_braces_only'
+        )
 
         return args + (scriptsdir, site_packages, python_tag)
 
@@ -212,4 +221,6 @@ def _compute_variables(metadata, prefix, name):
     variables["prefix"] = prefix
     variables["name"] = name
 
-    return substitute_variables(variables, variables)
+    return substitute_variables(
+        variables, variables, template='curly_braces_only'
+    )
diff --git a/okonomiyaki/utils/misc.py b/okonomiyaki/utils/misc.py
index bddffb1..b441357 100644
--- a/okonomiyaki/utils/misc.py
+++ b/okonomiyaki/utils/misc.py
@@ -1,5 +1,6 @@
 import ast
 import contextlib
+import re
 import shutil
 import string
 import tempfile
@@ -64,14 +65,25 @@ def tempdir():
         shutil.rmtree(d)
 
 
-def substitute_variables(d, local_vars):
-    """Perform shell/Perl-style variable substitution.
+def substitute_variables(d, local_vars, template='standard'):
+    """Perform repeated shell/Perl-style variable substitution to dict values
 
-    Every occurrence of '${name}' name is considered a variable, and variable
-    is substituted by the value found in the `local_vars' dictionary.  Raise
-    ValueError for any variables not found in `local_vars'.
+    Every occurrence of '${name}' in the values of dict 'd' is considered a
+    variable, and the variable is substituted by the value found in the
+    'local_vars' dictionary. A ValueError is raised for any variable not found
+    in 'local_vars'. This is applied repeatedly until all nested variables are
+    resolved.
 
-    '$' may be escaped by using '$$'
+    If the 'standard' template is used, $name will also be substituted. There
+    is a bug with escapes using the 'standard' template. This occurs because
+    substitution is applied repeatedly and after $$name is translated to $name,
+    variable substitution is performed on $name.
+
+    With the 'curly_braces_only' template $name will not be substituted. Also
+    escapes using '$${name}' are ignored and not translated to '${name}'. This
+    allows the variable substitution to be applied repeatedly. The function
+    'substitute_variable' should be applied to the data after this function
+    which does translate the escape '$${name}' to '${name}' by default.
 
     Parameters
     ----------
@@ -79,11 +91,18 @@ def substitute_variables(d, local_vars):
         (str: str) mapping, where each value will be substituted.
     local_vars: dict
         dict of variables
+    template: ('standard' | 'curly_braces_only')
+        whether to use 'standard' string.Template or RequireCurlyTemplate
     """
     def _resolve(d):
         ret = {}
         for k, v in d.items():
-            ret[k] = substitute_variable(v, local_vars)
+            # Ignoring the escape sequence with ignore_escape=True allows
+            # substitute_variable to be run repeatedly over the same data
+            # ignore_escape=True has no effect with the old 'standard' template
+            ret[k] = substitute_variable(
+                v, local_vars, template=template, ignore_escape=True
+            )
         return ret
 
     ret = _resolve(d)
@@ -93,5 +112,76 @@ def substitute_variables(d, local_vars):
     return ret
 
 
-def substitute_variable(v, local_vars):
-    return string.Template(v).substitute(local_vars)
+class RequireCurlyTemplate(string.Template):
+    """This class inheriting from Template requires curly braces.
+       A '$' without curly braces will not be substituted.
+    """
+    delimiter = '$'
+    # named and escaped groups are always None
+    # This is because their patterns are a subset of the invalid group,
+    # i.e. the invalid group will always match first.
+    # According to the Python re documentation the "|" operator is never greedy,
+    # so the named and escaped groups will always be None.
+    ignore_escape_pattern_str = r"""
+    (?<!\$)\$(?:                        # Only match single dollar signs
+      {(?P<braced>[_a-z][_a-z0-9]*)} |  # Delimiter and braced identifier
+      {(?P<invalid>[^}]*)}           |  # Other ill-formed delimiter expr
+      {(?P<named>)}                  |  # named group is always None
+      {(?P<escaped>)}                   # escaped group is always None
+    )
+    """
+    ignore_escape_pattern = re.compile(
+        ignore_escape_pattern_str, re.IGNORECASE | re.VERBOSE
+    )
+    pattern = r"""
+    \$(?:
+      (?P<escaped>\$)(?={[^}]*})     |  # Extra delimiter followed by braces
+      {(?P<braced>[_a-z][_a-z0-9]*)} |  # Delimiter and braced identifier
+      {(?P<invalid>[^}]*)}           |  # Other ill-formed delimiter expr
+      {(?P<named>)}                     # named group is always None
+    )
+    """
+
+    def __init__(self, template, ignore_escape=False):
+        super(RequireCurlyTemplate, self).__init__(template)
+        if ignore_escape:
+            self.pattern = self.ignore_escape_pattern
+
+
+def substitute_variable(
+        v, local_vars, template='standard', ignore_escape=False):
+    """Perform shell/Perl-style variable substitution to a string 'v'
+
+    Every occurrence of '${name}' in the value of string 'v' is considered a
+    variable, and the variable is substituted by the value found in the
+    'local_vars' dictionary. A ValueError is raised for any variable not found
+    in 'local_vars'. This is only applied once unlike 'substitute_variables',
+    so nested variables will not be resolved.
+
+    If the 'standard' template is used, $name will also be substituted.
+
+    Escapes using '$${name}' are translated to '${name}'.
+
+    If the 'curly_braces_only' template is used and 'ignore_escape' is True,
+    escapes using '$${name}' will be ignored.
+
+    Parameters
+    ----------
+    v: string
+        string where each value will be substituted.
+    local_vars: dict
+        dict of variables
+    template: ('standard' | 'curly_braces_only')
+        whether to use 'standard' string.Template or RequireCurlyTemplate
+    ignore_escape: boolean
+        whether or not to ignore '$${name}' with RequireCurlyTemplate only
+    """
+    if template == 'curly_braces_only':
+        template_substitute = RequireCurlyTemplate(v, ignore_escape).substitute
+    elif template == 'standard':
+        template_substitute = string.Template(v).substitute
+    else:
+        raise ValueError(
+            'Template option must be "standard" or "curly_braces_only"'
+        )
+    return template_substitute(local_vars)


Test patch:
diff --git a/okonomiyaki/runtimes/tests/test_runtime_info.py b/okonomiyaki/runtimes/tests/test_runtime_info.py
index 86ea23f..b074177 100644
--- a/okonomiyaki/runtimes/tests/test_runtime_info.py
+++ b/okonomiyaki/runtimes/tests/test_runtime_info.py
@@ -39,6 +39,30 @@ class TestPythonRuntimeInfoV1(unittest.TestCase):
         self.assertEqual(runtime_info.name, name)
         self.assertEqual(runtime_info.executable, r_executable)
 
+    def test_dollar_in_prefix(self):
+        # Given
+        name = u"test"
+        prefix = os.path.abspath(os.path.join(u"$foo", u"bar$"))
+
+        if sys.platform == "win32":
+            path = PYTHON_CPYTHON_2_7_10_WIN_X86_64
+            r_executable = os.path.join(prefix, "python.exe")
+        else:
+            path = PYTHON_CPYTHON_2_7_10_RH5_X86_64
+            r_executable = os.path.join(prefix, "bin", "python")
+
+        metadata = IRuntimeMetadata.factory_from_path(path)
+
+        # When
+        runtime_info = IRuntimeInfo.factory_from_metadata(
+            metadata, prefix, name
+        )
+
+        # Then
+        self.assertEqual(runtime_info.prefix, prefix)
+        self.assertEqual(runtime_info.name, name)
+        self.assertEqual(runtime_info.executable, r_executable)
+
     def test_json_round_trip(self):
         # Given
         path = PYTHON_CPYTHON_2_7_10_RH5_X86_64
diff --git a/okonomiyaki/utils/tests/test_misc.py b/okonomiyaki/utils/tests/test_misc.py
index 305f2a5..8ba2ba6 100644
--- a/okonomiyaki/utils/tests/test_misc.py
+++ b/okonomiyaki/utils/tests/test_misc.py
@@ -2,7 +2,7 @@ import sys
 import textwrap
 
 from ...errors import OkonomiyakiError
-from ..misc import parse_assignments, substitute_variables
+from ..misc import parse_assignments, substitute_variables, substitute_variable
 from ..py3compat import StringIO
 
 if sys.version_info < (2, 7):
@@ -46,10 +46,14 @@ class TestSubstitute(unittest.TestCase):
         }
 
         # When
-        rendered = substitute_variables(data, variables)
+        rendered_standard = substitute_variables(data, variables)
+        rendered_curly_only = substitute_variables(
+            data, variables, template='curly_braces_only'
+        )
 
         # Then
-        self.assertEqual(rendered, r_data)
+        self.assertEqual(rendered_standard, r_data)
+        self.assertEqual(rendered_curly_only, r_data)
 
     def test_recursive(self):
         # Given
@@ -69,8 +73,149 @@ class TestSubstitute(unittest.TestCase):
         }
 
         # When
-        variables = substitute_variables(variables, variables)
-        rendered = substitute_variables(data, variables)
+        variables_standard = substitute_variables(variables, variables)
+        variables_curly_only = substitute_variables(
+            variables, variables
+        )
+        rendered_standard = substitute_variables(data, variables_standard)
+        rendered_curly_only = substitute_variables(
+            data, variables_curly_only, template='curly_braces_only'
+        )
+
+        # Then
+        self.assertEqual(rendered_standard, r_data)
+        self.assertEqual(rendered_curly_only, r_data)
+
+    def test_escape(self):
+        # Given
+        data = {
+            "foo": "$${yolo}",
+            "bar": "$${foo}/bin",
+        }
+
+        variables = {
+            "yolo": "/foo/bar",
+        }
+        variables.update(data)
+
+        r_data = {
+            "foo": "$${yolo}",
+            "bar": "$${foo}/bin",
+        }
+        r_foo_ignore_escape = "$${yolo}"
+        r_foo_escape = "${yolo}"
+
+        # When
+        variables = substitute_variables(
+            variables, variables, template="curly_braces_only"
+        )
+        rendered = substitute_variables(
+            data, variables, template="curly_braces_only"
+        )
+        render_foo_ignore_escape = substitute_variable(
+            data["foo"], variables, template="curly_braces_only",
+            ignore_escape=True
+        )
+        render_foo_escape = substitute_variable(
+            data["foo"], variables, template="curly_braces_only"
+        )
 
         # Then
         self.assertEqual(rendered, r_data)
+        self.assertEqual(render_foo_ignore_escape, r_foo_ignore_escape)
+        self.assertEqual(render_foo_escape, r_foo_escape)
+
+    def test_without_curly_braces(self):
+        # Given
+        data = {
+            "foo": "$yolo",
+            "bar": "$foo/bin",
+        }
+
+        variables = {
+            "yolo": "/foo/bar",
+        }
+        variables.update(data)
+
+        r_data = {
+            "foo": "$yolo",
+            "bar": "$foo/bin",
+        }
+
+        # When
+        variables = substitute_variables(
+            variables, variables, template="curly_braces_only"
+        )
+        rendered = substitute_variables(
+            data, variables, template="curly_braces_only"
+        )
+
+        # Then
+        self.assertEqual(rendered, r_data)
+
+    def test_empty_substitution(self):
+        # Given
+        # Empty variable name is invalid
+        data = {
+            "foo": "${}yolo",
+            "bar": "/bin",
+        }
+
+        variables = {
+            "yolo": "/foo/bar",
+        }
+        variables.update(data)
+
+        # When/Then
+        with self.assertRaises(ValueError):
+            variables = substitute_variables(
+                variables, variables, template="curly_braces_only"
+            )
+            substitute_variables(
+                data, variables, template="curly_braces_only"
+            )
+
+    def test_invalid_substitution(self):
+        # Given
+        # idpattern = r'[_a-z][_a-z0-9]*'
+        # Characters not matching idpattern are invalid
+        data = {
+            "foo": "${yo-lo}",
+            "bar": "/bin",
+        }
+
+        variables = {
+            "yo-lo": "/foo/bar",
+        }
+        variables.update(data)
+
+        # When/Then
+        with self.assertRaises(ValueError):
+            variables = substitute_variables(
+                variables, variables, template="curly_braces_only"
+            )
+            substitute_variables(
+                data, variables, template="curly_braces_only"
+            )
+
+    def test_key_error_substitution(self):
+        # Given
+        # Nonexistent variable name gives key error
+        data = {
+            "foo": "${nonexistent}yolo",
+            "bar": "/bin",
+        }
+
+        variables = {
+            "yolo": "/foo/bar",
+        }
+        variables.update(data)
+
+        # When/Then
+        with self.assertRaises(KeyError):
+            variables = substitute_variables(
+                variables, variables, template="curly_braces_only"
+            )
+            substitute_variables(
+                data, variables, template="curly_braces_only"
+            )