venv: Use tagged zipfiles for downloaded wheels (#1650)

* Use the MU_LOG_TO_STDOUT envvar to turn on stdout logging and to disable the crash handler
Use version-tagged zips for the downloaded packages

* Refactor slightly partly to support easier testing

* Test the right exception if the download fails

* Pushing latest changes although incomplete

-- Still tests failing and some coverage missing after refactor

* Iterate over the files in the folder, not the name of the folder(!)

* Ensure we can use glob

* Tidy

* Ignore generated zip files

* Refactor baseline install test to allow for the fact that we now install from zips, not from files

* Tweak tests for logging setup to allow for MU_LOG_TO_STDOUT env var

* Tidy

* Disable the pip version check when downloading wheels
Always add the temp download area to the search patch for downloading wheels. This should work for the MacOS shim where we have to download a signed wheel for pygame and then be sure to use that in preference to the PyPI version

* Add more useful function comments
Remove (again) `upgrade_pip`
+ Tidy

.gitignore

@@ -88,3 +88,4 @@ target/
 /baseline_packages.json
 venv-pup/
 /local-scripts
+/mu/wheels/*.zip

mu/app.py

@@ -224,14 +224,15 @@ def setup_logging():
     log = logging.getLogger()
     log.setLevel(logging.DEBUG)
     log.addHandler(handler)
-    sys.excepthook = excepthook
 
     # Only enable on-screen logging if the MU_LOG_TO_STDOUT env variable is set
     if "MU_LOG_TO_STDOUT" in os.environ:
         stdout_handler = logging.StreamHandler()
         stdout_handler.setFormatter(formatter)
         stdout_handler.setLevel(logging.DEBUG)
         log.addHandler(stdout_handler)
+    else:
+        sys.excepthook = excepthook
 
 
 def setup_modes(editor, view):

mu/virtual_environment.py

@@ -5,7 +5,9 @@
 import glob
 import logging
 import subprocess
+import tempfile
 import time
+import zipfile
 
 from PyQt5.QtCore import (
     QObject,
@@ -93,7 +95,7 @@ def __init__(self):
         self.environment.insert("PYTHONIOENCODING", "utf-8")
 
     def _set_up_run(self, **envvars):
-        """Run the process with the command and args"""
+        """Set up common elements of a QProcess run"""
         self.process = QProcess()
         environment = QProcessEnvironment(self.environment)
         for k, v in envvars.items():
@@ -102,6 +104,11 @@ def _set_up_run(self, **envvars):
         self.process.setProcessChannelMode(QProcess.MergedChannels)
 
     def run_blocking(self, command, args, wait_for_s=30.0, **envvars):
+        """Run `command` with `args` via QProcess, passing `envvars` as
+        environment variables for the process.
+
+        Wait `wait_for_s` seconds for completion and return any stdout/stderr
+        """
         logger.info(
             "About to run blocking %s with args %s and envvars %s",
             command,
@@ -113,6 +120,8 @@ def run_blocking(self, command, args, wait_for_s=30.0, **envvars):
         return self.wait(wait_for_s=wait_for_s)
 
     def run(self, command, args, **envvars):
+        """Run `command` asynchronously with `args` via QProcess, passing `envvars`
+        as environment variables for the process."""
         logger.info(
             "About to run %s with args %s and envvars %s",
             command,
@@ -127,6 +136,12 @@ def run(self, command, args, **envvars):
         QTimer.singleShot(1, partial)
 
     def wait(self, wait_for_s=30.0):
+        """Wait for the process to complete, optionally timing out.
+        Return any stdout/stderr.
+
+        If the process fails to complete in time or returns an error, raise a
+        VirtualEnvironmentError
+        """
         finished = self.process.waitForFinished(1000 * wait_for_s)
         exit_status = self.process.exitStatus()
         exit_code = self.process.exitCode()
@@ -174,6 +189,7 @@ def wait(self, wait_for_s=30.0):
         return output
 
     def data(self):
+        """Return all the data from the running process, converted to unicode"""
         output = self.process.readAll().data()
         return output.decode(sys.stdout.encoding, errors="replace")
 
@@ -228,7 +244,7 @@ def run(
             result = self.process.run_blocking(
                 self.executable, params, wait_for_s=wait_for_s
             )
-            logger.debug("Process output: %s", result.strip())
+            logger.debug("Process output: %s", compact(result.strip()))
             return result
         else:
             if slots.started:
@@ -392,7 +408,7 @@ def __str__(self):
     @staticmethod
     def _generate_dirpath():
         """
-        Construct a unique virtual environment folder
+        Construct a unique virtual environment folder name
 
         To avoid clashing with previously-created virtual environments,
         construct one which includes the Python version and a timestamp
@@ -432,6 +448,9 @@ def run_subprocess(self, *args, **kwargs):
         return process.returncode == 0, output
 
     def reset_pip(self):
+        """To avoid a problem where the same Pip process is executed by different
+        threads, recreate the Pip process on demand
+        """
         self.pip = Pip(self.pip_executable)
 
     def relocate(self, dirpath):
@@ -506,6 +525,12 @@ def _directory_is_venv(self):
         return False
 
     def quarantine_venv(self, reason="FAILED"):
+        """Rename an existing virtual environment folder out of the way to make
+        it clearer which is the current one.
+
+        NB if this fails (eg because of file locking) it won't matter: the folder
+        will not be used and will simply be a little distracting
+        """
         error_dirpath = self.path + "." + reason
         try:
             os.rename(self.path, error_dirpath)
@@ -545,8 +570,9 @@ def recreate(self):
         #
         # Now reinstall the original user packages
         #
-        logger.debug("About to reinstall user packages: %s", user_packages)
-        self.install_user_packages(user_packages)
+        if user_packages:
+            logger.debug("About to reinstall user packages: %s", user_packages)
+            self.install_user_packages(user_packages)
 
     def ensure_and_create(self, emitter=None):
         """Check whether we have a valid virtual environment in place and, if not,
@@ -625,10 +651,6 @@ def ensure_and_create(self, emitter=None):
                 else:
                     raise
             finally:
-                logger.debug(
-                    "Emitter: %s; Splash Handler; %s"
-                    % (emitter, splash_handler)
-                )
                 if emitter and splash_handler:
                     logger.removeHandler(splash_handler)
 
@@ -752,7 +774,7 @@ def create(self):
 
     def create_venv(self):
         """
-        Create a new virtualenv at the referenced path.
+        Create a new virtualenv
         """
         logger.info("Creating virtualenv: {}".format(self.path))
         logger.info("Virtualenv name: {}".format(self.name))
@@ -780,26 +802,10 @@ def create_venv(self):
                 % (sys.executable, self.path, compact(output))
             )
 
-    def upgrade_pip(self):
-        logger.debug(
-            "About to upgrade pip; interpreter %s %s",
-            self.interpreter,
-            "exists" if os.path.exists(self.interpreter) else "doesn't exist",
-        )
-        ok, output = self.run_subprocess(
-            self.interpreter, "-m", "pip", "install", "--upgrade", "pip"
-        )
-        if ok:
-            logger.info("Upgraded pip")
-        else:
-            raise VirtualEnvironmentCreateError(
-                "Unable to upgrade pip\n%s" % compact(output)
-            )
-
     def install_jupyter_kernel(self):
         """
         Install a Jupyter kernel for Mu (the name of the kernel indicates this
-        is a Mu related kernel).
+        is a Mu-related kernel).
         """
         kernel_name = self.name.replace(" ", "-")
         display_name = '"Python/Mu ({})"'.format(kernel_name)
@@ -822,6 +828,34 @@ def install_jupyter_kernel(self):
                 "Unable to install kernel\n%s" % compact(output)
             )
 
+    def install_from_zipped_wheels(self, zipped_wheels_filepath):
+        """Take a zip containing wheels, unzip it and install the wheels into
+        the current virtualenv
+        """
+        with tempfile.TemporaryDirectory() as unpacked_wheels_dirpath:
+            #
+            # The wheel files are shipped in Mu-version-specific zip files to avoid
+            # cross-contamination when a Mu version change happens and we still have
+            # wheels from the previous installation.
+            #
+            with zipfile.ZipFile(zipped_wheels_filepath) as zip:
+                zip.extractall(unpacked_wheels_dirpath)
+
+            self.reset_pip()
+            #
+            # The wheels are installed one at a time as they reduces the possibility
+            # of the process installing them breaching its timeout
+            #
+            for wheel in glob.glob(
+                os.path.join(unpacked_wheels_dirpath, "*.whl")
+            ):
+                logger.info(
+                    "About to install from wheel: {}".format(
+                        os.path.basename(wheel)
+                    )
+                )
+                self.pip.install(wheel, deps=False, index=False)
+
     def install_baseline_packages(self):
         """
         Install all packages needed for non-core activity.
@@ -832,44 +866,20 @@ def install_baseline_packages(self):
         no network access is needed. But if the wheels aren't found, because
         we're not running from an installer, then just pip install in the
         usual way.
-
-        --upgrade is currently used with a thought to upgrade-releases of Mu.
         """
         logger.info("Installing baseline packages.")
-        logger.info(
-            "%s %s",
-            wheels_dirpath,
-            "exists" if os.path.isdir(wheels_dirpath) else "does not exist",
-        )
-        #
-        # This command should install the baseline packages, picking up the
-        # precompiled wheels from the wheels path
         #
-        # For dev purposes (where we might not have the wheels) warn where
-        # the wheels are not already present and download them
+        # TODO: Add semver check to ensure filepath is safe
         #
-        wheel_filepaths = glob.glob(os.path.join(wheels_dirpath, "*.whl"))
-        if not wheel_filepaths:
-            logger.warning(
-                "No wheels found in %s; downloading...", wheels_dirpath
-            )
-            try:
-                wheels.download(interpreter=self.interpreter, logger=logger)
-            except wheels.WheelsDownloadError as exc:
-                raise VirtualEnvironmentCreateError(exc.message)
-            else:
-                wheel_filepaths = glob.glob(
-                    os.path.join(wheels_dirpath, "*.whl")
-                )
+        zipped_wheels_filepath = os.path.join(
+            wheels_dirpath, "%s.zip" % mu_version
+        )
+        logger.info("Expecting zipped wheels at %s", zipped_wheels_filepath)
+        if not os.path.exists(zipped_wheels_filepath):
+            logger.warning("No zipped wheels found; downloading...")
+            wheels.download(zipped_wheels_filepath)
 
-        if not wheel_filepaths:
-            raise VirtualEnvironmentCreateError(
-                "No wheels in %s; try `python -mmu.wheels`" % wheels_dirpath
-            )
-        self.reset_pip()
-        for wheel in wheel_filepaths:
-            logger.info("About to install from wheels: {}".format(wheel))
-            self.pip.install(wheel, deps=False, index=False)
+        self.install_from_zipped_wheels(zipped_wheels_filepath)
 
     def register_baseline_packages(self):
         """