Auto merge of #9415 - adamncasey:mach-run-win32-fix, r=larsbergstrom

Fix ./mach run on Windows

This branch also includes the commit being reviewed in https://github.com/servo/servo/pull/9408 . Is it OK to just wait for that one to merge, or should I separate them?

This patch adds the BIN_SUFFIX to the servo executable name when doing ./mach run.

However just doing this caused subprocess to error due to some unicode symbols in PATH.

To fix this, I pulled a peice of code from mozilla-central which fixes this problem there. Source: https://dxr.mozilla.org/mozilla-central/source/python/mach/mach/mixin/process.py#108

Revisiting this now (originally developed this a few weeks ago), perhaps we should be using https://github.com/servo/servo/blob/master/python/mach/mach/mixin/process.py instead of subprocess to give us all of this crossplatform process execution support. I think that should be a nice to have though - this patch at least gets a necessary development command working on windows.

Tested on Windows and Ubuntu.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9415)
<!-- Reviewable:end -->

2 files changed, 39 insertions, 7 deletions

diff --git a/python/mach_bootstrap.py b/python/mach_bootstrap.py
index e78132d21d2..a700b4c2419 100644
--- a/python/mach_bootstrap.py
+++ b/python/mach_bootstrap.py
@@ -80,6 +80,13 @@ def _get_exec(*names):
     return None
 
 
+def _get_virtualenv_script_dir():
+    # Virtualenv calls its scripts folder "bin" on linux/OSX/MSYS64 but "Scripts" on Windows
+    if os.name == "nt" and os.path.sep != "/":
+        return "Scripts"
+    return "bin"
+
+
 # Possible names of executables, sorted from most to least specific
 PYTHON_NAMES = ["python-2.7", "python2.7", "python2", "python"]
 VIRTUALENV_NAMES = ["virtualenv-2.7", "virtualenv2.7", "virtualenv2", "virtualenv"]
@@ -92,8 +99,7 @@ def _activate_virtualenv(topdir):
     if python is None:
         sys.exit("Python is not installed. Please install it prior to running mach.")
 
-    # Virtualenv calls its scripts folder "bin" on linux/OSX but "Scripts" on Windows, detect which one then use that
-    script_dir = "Scripts" if os.name == "nt" else "bin"
+    script_dir = _get_virtualenv_script_dir()
     activate_path = os.path.join(virtualenv_path, script_dir, "activate_this.py")
     if not (os.path.exists(virtualenv_path) and os.path.exists(activate_path)):
         virtualenv = _get_exec(*VIRTUALENV_NAMES)
diff --git a/python/servo/command_base.py b/python/servo/command_base.py
index ad411b3de53..e8164acd6d8 100644
--- a/python/servo/command_base.py
+++ b/python/servo/command_base.py
@@ -71,9 +71,33 @@ def call(*args, **kwargs):
     return subprocess.call(*args, shell=sys.platform == 'win32', **kwargs)
 
 
+def normalize_env(env):
+    # There is a bug in subprocess where it doesn't like unicode types in
+    # environment variables. Here, ensure all unicode are converted to
+    # binary. utf-8 is our globally assumed default. If the caller doesn't
+    # want UTF-8, they shouldn't pass in a unicode instance.
+    normalized_env = {}
+    for k, v in env.items():
+        if isinstance(k, unicode):
+            k = k.encode('utf-8', 'strict')
+
+        if isinstance(v, unicode):
+            v = v.encode('utf-8', 'strict')
+
+        normalized_env[k] = v
+
+    return normalized_env
+
+
 def check_call(*args, **kwargs):
-    """Wrap `subprocess.check_call`, printing the command if verbose=True."""
+    """Wrap `subprocess.check_call`, printing the command if verbose=True.
+
+    Also fix any unicode-containing `env`, for subprocess """
     verbose = kwargs.pop('verbose', False)
+
+    if 'env' in kwargs:
+        kwargs['env'] = normalize_env(kwargs['env'])
+
     if verbose:
         print(' '.join(args[0]))
     # we have to use shell=True in order to get PATH handling
@@ -175,10 +199,13 @@ class CommandBase(object):
 
     def get_binary_path(self, release, dev, android=False):
         base_path = self.get_target_dir()
+
         if android:
             base_path = path.join(base_path, self.config["android"]["target"])
-        release_path = path.join(base_path, "release", "servo")
-        dev_path = path.join(base_path, "debug", "servo")
+
+        binary_name = "servo" + BIN_SUFFIX
+        release_path = path.join(base_path, "release", binary_name)
+        dev_path = path.join(base_path, "debug", binary_name)
 
         # Prefer release if both given
         if release and dev:
@@ -246,8 +273,7 @@ class CommandBase(object):
                 path.join(self.config["tools"]["cargo-root"], "bin")]
 
         if extra_path:
-            env["PATH"] = "%s%s%s" % (
-                os.pathsep.join(extra_path), os.pathsep, env["PATH"])
+            env["PATH"] = "%s%s%s" % (env["PATH"], os.pathsep, os.pathsep.join(extra_path))
 
         if "CARGO_HOME" not in env:
             env["CARGO_HOME"] = self.config["tools"]["cargo-home-dir"]