Feature/1269 formalize paths in package commands

src/rez/build_system.py

@@ -7,6 +7,7 @@
 from rez.build_process import BuildType
 from rez.exceptions import BuildSystemError
 from rez.packages import get_developer_package
+from rez.rex_bindings import VariantBinding
 
 
 def get_buildsys_types():
@@ -234,10 +235,9 @@ def build(self, context, variant, build_path, install_path, install=False,
         raise NotImplementedError
 
     @classmethod
-    def get_standard_vars(cls, context, variant, build_type, install,
+    def set_standard_vars(cls, executor, context, variant, build_type, install,
                           build_path, install_path=None):
-        """Returns a standard set of environment variables that can be set
-        for the build system to use
+        """Set some standard env vars that all build systems can rely on.
         """
         from rez.config import config
 
@@ -251,16 +251,18 @@ def get_standard_vars(cls, context, variant, build_type, install,
 
         vars_ = {
             'REZ_BUILD_ENV': 1,
-            'REZ_BUILD_PATH': build_path,
+            'REZ_BUILD_PATH': executor.normalize_path(build_path),
             'REZ_BUILD_THREAD_COUNT': package.config.build_thread_count,
             'REZ_BUILD_VARIANT_INDEX': variant.index or 0,
             'REZ_BUILD_VARIANT_REQUIRES': ' '.join(variant_requires),
-            'REZ_BUILD_VARIANT_SUBPATH': variant_subpath,
+            'REZ_BUILD_VARIANT_SUBPATH': executor.normalize_path(variant_subpath),
             'REZ_BUILD_PROJECT_VERSION': str(package.version),
             'REZ_BUILD_PROJECT_NAME': package.name,
             'REZ_BUILD_PROJECT_DESCRIPTION': (package.description or '').strip(),
             'REZ_BUILD_PROJECT_FILE': package.filepath,
-            'REZ_BUILD_SOURCE_PATH': os.path.dirname(package.filepath),
+            'REZ_BUILD_SOURCE_PATH': executor.normalize_path(
+                os.path.dirname(package.filepath)
+            ),
             'REZ_BUILD_REQUIRES': ' '.join(
                 str(x) for x in context.requested_packages(True)
             ),
@@ -272,14 +274,16 @@ def get_standard_vars(cls, context, variant, build_type, install,
         }
 
         if install_path:
-            vars_['REZ_BUILD_INSTALL_PATH'] = install_path
+            vars_['REZ_BUILD_INSTALL_PATH'] = executor.normalize_path(install_path)
 
         if config.rez_1_environment_variables and \
                 not config.disable_rez_1_compatibility and \
                 build_type == BuildType.central:
             vars_['REZ_IN_REZ_RELEASE'] = 1
 
-        return vars_
+        # set env vars
+        for key, value in vars_.items():
+            executor.env[key] = value
 
     @classmethod
     def add_pre_build_commands(cls, executor, variant, build_type, install,
@@ -292,16 +296,29 @@ def add_pre_build_commands(cls, executor, variant, build_type, install,
         build_ns = {
             "build_type": build_type.name,
             "install": install,
-            "build_path": build_path,
-            "install_path": install_path
+            "build_path": executor.normalize_path(build_path),
+            "install_path": executor.normalize_path(install_path)
         }
 
         # execute pre_build_commands()
+        # note that we need to wrap variant in a VariantBinding so that any refs
+        # to (eg) 'this.root' in pre_build_commands() will get the possibly
+        # normalized path.
+        #
         pre_build_commands = getattr(variant, "pre_build_commands")
 
+        # TODO I suspect variant root isn't correctly set to the cached root
+        # when pkg caching is enabled (see use of VariantBinding in
+        # ResolvedContext._execute).
+        #
+        bound_variant = VariantBinding(
+            variant,
+            interpreter=executor.interpreter
+        )
+
         if pre_build_commands:
             with executor.reset_globals():
-                executor.bind("this", variant)
+                executor.bind("this", bound_variant)
                 executor.bind("build", ROA(build_ns))
                 executor.execute_code(pre_build_commands)
 
@@ -311,14 +328,12 @@ def add_standard_build_actions(cls, executor, context, variant, build_type,
         """Perform build actions common to every build system.
         """
         # set env vars
-        env_vars = cls.get_standard_vars(
+        cls.set_standard_vars(
+            executor=executor,
             context=context,
             variant=variant,
             build_type=build_type,
             install=install,
             build_path=build_path,
             install_path=install_path
         )
-
-        for var, value in env_vars.items():
-            executor.env[var] = value

src/rez/config.py

@@ -344,6 +344,7 @@ def _parse_env_var(self, value):
     "resetting_variables":                          StrList,
     "release_hooks":                                StrList,
     "context_tracking_context_fields":              StrList,
+    "pathed_env_vars":                              StrList,
     "prompt_release_message":                       Bool,
     "critical_styles":                              OptionalStrList,
     "error_styles":                                 OptionalStrList,

src/rez/resolved_context.py

@@ -1948,19 +1948,25 @@ def _get_pre_resolve_bindings(self):
 
     @pool_memcached_connections
     def _execute(self, executor):
-        # bind various info to the execution context
+        """Bind various info to the execution context
+        """
+        def normalized(path):
+            return executor.normalize_path(path)
+
         resolved_pkgs = self.resolved_packages or []
         ephemerals = self.resolved_ephemerals or []
 
         request_str = ' '.join(str(x) for x in self._package_requests)
         implicit_str = ' '.join(str(x) for x in self.implicit_packages)
         resolve_str = ' '.join(x.qualified_package_name for x in resolved_pkgs)
-        package_paths_str = os.pathsep.join(self.package_paths)
         req_timestamp_str = str(self.requested_timestamp or 0)
+        package_paths_str = executor.interpreter.pathsep.join(
+            normalized(x) for x in self.package_paths
+        )
 
         header_comment(executor, "system setup")
 
-        executor.setenv("REZ_USED", self.rez_path)
+        executor.setenv("REZ_USED", normalized(self.rez_path))
         executor.setenv("REZ_USED_VERSION", self.rez_version)
         executor.setenv("REZ_USED_TIMESTAMP", str(self.timestamp))
         executor.setenv("REZ_USED_REQUESTED_TIMESTAMP", req_timestamp_str)
@@ -1981,7 +1987,7 @@ def _execute(self, executor):
                 not config.disable_rez_1_compatibility:
             request_str_ = " ".join([request_str, implicit_str]).strip()
             executor.setenv("REZ_VERSION", self.rez_version)
-            executor.setenv("REZ_PATH", self.rez_path)
+            executor.setenv("REZ_PATH", normalized(self.rez_path))
             executor.setenv("REZ_REQUEST", request_str_)
             executor.setenv("REZ_RESOLVE", resolve_str)
             executor.setenv("REZ_RAW_REQUEST", request_str_)
@@ -2005,7 +2011,9 @@ def _execute(self, executor):
             else:
                 cached_root = None
 
-            variant_binding = VariantBinding(pkg, cached_root=cached_root)
+            variant_binding = VariantBinding(
+                pkg, cached_root=cached_root, interpreter=executor.interpreter
+            )
             variant_bindings[pkg.name] = variant_binding
 
         # binds objects such as 'request', which are accessible before a resolve
@@ -2038,17 +2046,17 @@ def _execute(self, executor):
             executor.setenv(prefix + "_MAJOR_VERSION", major_version)
             executor.setenv(prefix + "_MINOR_VERSION", minor_version)
             executor.setenv(prefix + "_PATCH_VERSION", patch_version)
-            executor.setenv(prefix + "_BASE", pkg.base)
+            executor.setenv(prefix + "_BASE", normalized(pkg.base))
 
             variant_binding = variant_bindings[pkg.name]
 
             if variant_binding._is_in_package_cache():
                 # set to cached payload rather than original
                 executor.setenv(prefix + "_ROOT", variant_binding.root)
                 # store extra var to indicate that root retarget occurred
-                executor.setenv(prefix + "_ORIG_ROOT", pkg.root)
+                executor.setenv(prefix + "_ORIG_ROOT", normalized(pkg.root))
             else:
-                executor.setenv(prefix + "_ROOT", pkg.root)
+                executor.setenv(prefix + "_ROOT", normalized(pkg.root))
 
         # package commands
         for attr in ("pre_commands", "commands", "post_commands"):
@@ -2067,7 +2075,7 @@ def _execute(self, executor):
                 executor.bind('this', variant_binding)
                 executor.bind("version", VersionBinding(pkg.version))
                 executor.bind('root', variant_binding.root)
-                executor.bind('base', pkg.base)
+                executor.bind('base', normalized(pkg.base))
 
                 exc = None
                 commands.set_package(pkg)

src/rez/rex.py

@@ -8,6 +8,7 @@
 import sys
 import re
 import traceback
+from fnmatch import fnmatch
 from contextlib import contextmanager
 from string import Formatter
 
@@ -232,7 +233,7 @@ def get_public_methods(self):
             ('undefined', self.undefined)]
 
     def _env_sep(self, name):
-        return self._env_sep_map.get(name, os.pathsep)
+        return self._env_sep_map.get(name, self.interpreter.pathsep)
 
     def _is_verbose(self, command):
         if isinstance(self.verbose, (list, tuple)):
@@ -472,6 +473,11 @@ class ActionInterpreter(object):
     """
     expand_env_vars = False
 
+    # Path separator. There are cases (eg gitbash - git for windows) where the
+    # path separator does not match the system (ie os.pathsep)
+    #
+    pathsep = os.pathsep
+
     # RegEx that captures environment variables (generic form).
     # Extend/override to regex formats that can capture environment formats
     # in other interpreters like shells if needed
@@ -537,22 +543,63 @@ def shebang(self):
 
     # --- other
 
-    def escape_string(self, value):
+    def escape_string(self, value, is_path=False):
         """Escape a string.
 
         Escape the given string so that special characters (such as quotes and
         whitespace) are treated properly. If `value` is a string, assume that
         this is an expandable string in this interpreter.
 
+        Note that `is_path` provided because of the special case where a
+        path-like envvar is set. In this case, path normalization, if it needs
+        to occur, has to be part of the string escaping process.
+
         Note:
             This default implementation returns the string with no escaping
             applied.
 
         Args:
             value (str or `EscapedString`): String to escape.
+            is_path (bool): True if the value is path-like.
+
+        Returns:
+            str: The escaped string.
         """
         return str(value)
 
+    @classmethod
+    def _is_pathed_key(cls, key):
+        return any(fnmatch(key, x) for x in config.pathed_env_vars)
+
+    def normalize_path(self, path):
+        """Normalize a path.
+
+        Change `path` to a valid filepath representation for this interpreter.
+
+        IMPORTANT: Because var references like ${THIS} might be passed to funcs
+        like appendvar, `path` might be in this form. You need to take that
+        into account (ie, ensure normalization doesn't break such a var reference).
+
+        Args:
+            path (str): A filepath which may be in posix format, or windows
+                format, or some combination of the two. For eg, a string like
+                `{root}/bin` on windows will evaluate to `C:\\.../bin` - in this
+                case, the `cmd` shell would want to normalize this and convert
+                to all forward slashes.
+
+        Returns:
+            str: The normalized path.
+        """
+        return path
+
+    def normalize_paths(self, value):
+        """Normalize value if it's a path(s).
+        Note that `value` may be more than one pathsep-delimited paths.
+        """
+        paths = value.split(self.pathsep)
+        paths = [self.normalize_path(x) for x in paths]
+        return self.pathsep.join(paths)
+
     # --- internal commands, not exposed to public rex API
 
     def _saferefenv(self, key):
@@ -620,7 +667,7 @@ def setenv(self, key, value):
         if self.update_session:
             if key == 'PYTHONPATH':
                 value = self.escape_string(value)
-                sys.path = value.split(os.pathsep)
+                sys.path = value.split(self.pathsep)
 
     def unsetenv(self, key):
         pass
@@ -1284,12 +1331,14 @@ def reset_globals(self):
     def append_system_paths(self):
         """Append system paths to $PATH."""
         from rez.shells import Shell, create_shell
-        sh = self.interpreter if isinstance(self.interpreter, Shell) \
-            else create_shell()
 
-        paths = sh.get_syspaths()
-        paths_str = os.pathsep.join(paths)
-        self.env.PATH.append(paths_str)
+        if isinstance(self.interpreter, Shell):
+            sh = self.interpreter
+        else:
+            sh = create_shell()
+
+        for path in sh.get_syspaths():
+            self.env.PATH.append(path)
 
     def prepend_rez_path(self):
         """Prepend rez path to $PATH."""
@@ -1301,6 +1350,14 @@ def append_rez_path(self):
         if system.rez_bin_path:
             self.env.PATH.append(system.rez_bin_path)
 
+    def normalize_path(self, path):
+        """Normalize a path.
+        Note that in many interpreters this will be unchanged.
+        Returns:
+            str: The normalized path.
+        """
+        return self.interpreter.normalize_path(path)
+
     @classmethod
     def compile_code(cls, code, filename=None, exec_namespace=None):
         """Compile and possibly execute rex code.

src/rez/rex_bindings.py

@@ -19,7 +19,8 @@
 
 
 class Binding(object):
-    """Abstract base class."""
+    """Abstract base class.
+    """
     def __init__(self, data=None):
         self._data = data or {}
 
@@ -111,10 +112,13 @@ def __iter__(self):
 
 
 class VariantBinding(Binding):
-    """Binds a packages.Variant object."""
-    def __init__(self, variant, cached_root=None):
+    """Binds a packages.Variant object.
+    """
+    def __init__(self, variant, cached_root=None, interpreter=None):
         doc = dict(version=VersionBinding(variant.version))
         super(VariantBinding, self).__init__(doc)
+
+        self.__interpreter = interpreter
         self.__variant = variant
         self.__cached_root = cached_root
 
@@ -125,7 +129,12 @@ def root(self):
         such as 'resolve.mypkg.root' resolve to the cached payload location,
         if the package is cached.
         """
-        return self.__cached_root or self.__variant.root
+        root = self.__cached_root or self.__variant.root
+
+        if self.__interpreter:
+            root = self.__interpreter.normalize_path(root)
+
+        return root
 
     def __getattr__(self, attr):
         try:

src/rez/rezconfig.py

@@ -524,6 +524,15 @@
     "DOXYGEN_TAGFILES": " ",
 }
 
+# This setting identifies path-like environment variables. This is required
+# because some shells need to apply path normalization. For example, the command
+# `env.PATH.append("{root}/bin")` will be normalized to (eg) `C:\...\bin` in a
+# `cmd` shell on Windows. Note that wildcards are supported. If this setting is
+# not correctly configured, then your shell may not work correctly.
+pathed_env_vars = [
+    "*PATH"
+]
+
 # Defines what suites on $PATH stay visible when a new rez environment is resolved.
 # Possible values are:
 # - "never":            Don"t attempt to keep any suites visible in a new env