Auto merge of #13151 - Wafflespeanut:mach_cleanup, r=Ms2ger

Make sure that mach gets the correct paths of executables in virtualenv

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #13141 (github issue number if applicable).

<!-- Either: -->
- [x] These changes do not require tests because it's a cleanup related to `mach`

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13151)
<!-- Reviewable:end -->

mach

@@ -12,11 +12,10 @@
 from __future__ import print_function, unicode_literals
 
 import os
-from os import path
 import sys
 
 def main(args):
-    topdir = path.abspath(path.dirname(sys.argv[0]))
+    topdir = os.path.abspath(os.path.dirname(sys.argv[0]))
     sys.path.insert(0, os.path.join(topdir, "python"))
     import mach_bootstrap
     mach = mach_bootstrap.bootstrap(topdir)

python/mach_bootstrap.py

@@ -6,9 +6,9 @@ from __future__ import print_function, unicode_literals
 
 import os
 import platform
-import subprocess
 import sys
 from distutils.spawn import find_executable
+from subprocess import PIPE, Popen
 
 SEARCH_PATHS = [
     os.path.join("python", "tidy"),
@@ -26,7 +26,6 @@ MACH_MODULES = [
     os.path.join('python', 'servo', 'devenv_commands.py'),
 ]
 
-
 CATEGORIES = {
     'bootstrap': {
         'short': 'Bootstrap Commands',
@@ -76,55 +75,53 @@ CATEGORIES = {
     }
 }
 
+# Possible names of executables
+PYTHON_NAMES = ["python-2.7", "python2.7", "python2", "python"]
+VIRTUALENV_NAMES = ["virtualenv-2.7", "virtualenv2.7", "virtualenv2", "virtualenv"]
+PIP_NAMES = ["pip-2.7", "pip2.7", "pip2", "pip"]
 
-def _get_exec(*names):
+
+def _get_exec_path(names, is_valid_path=lambda _path: True):
     for name in names:
         path = find_executable(name)
-        if path is not None:
+        if path and is_valid_path(path):
             return path
     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 != "/":
+    if os.name == "nt" and os.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"]
-PIP_NAMES = ["pip-2.7", "pip2.7", "pip2", "pip"]
-
-
 def _activate_virtualenv(topdir):
     virtualenv_path = os.path.join(topdir, "python", "_virtualenv")
-    python = _get_exec(*PYTHON_NAMES)
+    check_exec_path = lambda path: path.startswith(virtualenv_path)
-    if python is None:
+    python = _get_exec_path(PYTHON_NAMES)   # If there was no python, mach wouldn't have run at all!
-        sys.exit("Python is not installed. Please install it prior to running mach.")
+    if not python:
+        sys.exit('Failed to find python executable for starting virtualenv.')
 
     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)
+        virtualenv = _get_exec_path(VIRTUALENV_NAMES)
-        if virtualenv is None:
+        if not virtualenv:
             sys.exit("Python virtualenv is not installed. Please install it prior to running mach.")
 
-        process = subprocess.Popen(
+        process = Popen([virtualenv, "-p", python, virtualenv_path], stdout=PIPE, stderr=PIPE)
-            [virtualenv, "-p", python, virtualenv_path],
-            stdout=subprocess.PIPE,
-            stderr=subprocess.PIPE)
         process.wait()
         if process.returncode:
-            sys.exit("Python virtualenv failed to execute properly: {}"
+            out, err = process.communicate()
-                     .format(process.communicate()[1]))
+            print('Python virtualenv failed to execute properly:')
+            sys.exit('Output: %s\nError: %s' % (out, err))
 
     execfile(activate_path, dict(__file__=activate_path))
 
-    python = find_executable("python")
+    python = _get_exec_path(PYTHON_NAMES, is_valid_path=check_exec_path)
-    if python is None or not python.startswith(virtualenv_path):
+    if not python:
-        sys.exit("Python virtualenv failed to activate.")
+        sys.exit("Python executable in virtualenv failed to activate.")
 
     # TODO: Right now, we iteratively install all the requirements by invoking
     # `pip install` each time. If it were the case that there were conflicting
@@ -138,28 +135,28 @@ def _activate_virtualenv(topdir):
         os.path.join("tests", "wpt", "harness", "requirements_firefox.txt"),
         os.path.join("tests", "wpt", "harness", "requirements_servo.txt"),
     ]
+
     for req_rel_path in requirements_paths:
         req_path = os.path.join(topdir, req_rel_path)
         marker_file = req_rel_path.replace(os.path.sep, '-')
         marker_path = os.path.join(virtualenv_path, marker_file)
+
         try:
             if os.path.getmtime(req_path) + 10 < os.path.getmtime(marker_path):
                 continue
         except OSError:
             pass
 
-        pip = _get_exec(*PIP_NAMES)
+        pip = _get_exec_path(PIP_NAMES, is_valid_path=check_exec_path)
-        if pip is None:
+        if not pip:
-            sys.exit("Python pip is not installed. Please install it prior to running mach.")
+            sys.exit("Python pip is either not installed or not found in virtualenv.")
 
-        process = subprocess.Popen(
+        process = Popen([pip, "install", "-q", "-r", req_path], stdout=PIPE, stderr=PIPE)
-            [pip, "install", "-q", "-r", req_path],
-            stdout=subprocess.PIPE,
-            stderr=subprocess.PIPE)
         process.wait()
         if process.returncode:
-            sys.exit("Pip failed to execute properly: {}"
+            out, err = process.communicate()
-                     .format(process.communicate()[1]))
+            print('Pip failed to execute properly:')
+            sys.exit('Output: %s\nError: %s' % (out, err))
 
         open(marker_path, 'w').close()
 
@@ -199,8 +196,7 @@ def bootstrap(topdir):
         sys.exit(1)
 
     # Ensure we are running Python 2.7+. We put this check here so we generate a
-    # user-friendly error message rather than a cryptic stack trace on module
+    # user-friendly error message rather than a cryptic stack trace on module import.
-    # import.
     if not (3, 0) > sys.version_info >= (2, 7):
         print('Python 2.7 or above (but not Python 3) is required to run mach.')
         print('You are running Python', platform.python_version())
@@ -221,8 +217,7 @@ def bootstrap(topdir):
     mach.populate_context_handler = populate_context
 
     for category, meta in CATEGORIES.items():
-        mach.define_category(category, meta['short'], meta['long'],
+        mach.define_category(category, meta['short'], meta['long'], meta['priority'])
-                             meta['priority'])
 
     for path in MACH_MODULES:
         mach.load_commands_from_file(os.path.join(topdir, path))