Repository: pypa/virtualenv

Issue:
20.0.19 does not install pip on Python 3.4
**Issue**

In Travis with Python 3.4, virtualenv 20.0.19 does not seem to install a pip into a newly created virtualenv when creating it while a virtualenv is already acrive. "which pip" returns the pip from the first virtualenv after the new one is created and active. It fails only on Python 3.4 (the errors with pypy in the Travis logs below have a different cause). virtualenv before 20.0.19 does not have that behavior and succeeds on Python 3.4.

Version and location of Python and Pip after having activated the second virtualenv: Note that Python is shown to come from the correct second virtualenv while Pip is shown to come from the incorrect first virtualenv that Travis had set up:
* Python version: Python 3.4.8 from /home/travis/build/pywbem/pywbem/tmp_installtest/virtualenvs/pywbem_test_test1/bin/python
* Pip version: pip 19.1.1 from /home/travis/virtualenv/python3.4.8/lib/python3.4/site-packages/pip (python 3.4)

To see this in the Travis logs, search for "Creating virtualenv:" case sensitively (around line 2500):
* Travis build with virtualenv 20.0.19, showing the issue: https://travis-ci.org/github/pywbem/pywbem/builds/682738409
* Travis build with virtualenv 20.0.18, showing success: https://travis-ci.org/github/pywbem/pywbem/builds/682740925

**Environment**

Provide at least:
- OS: MacOS 10.14.6
- ``pip list`` of the host python where ``virtualenv`` is installed:
  see Travis logs

**Output of the virtual environment creation**

Make sure to run the creation with `-vvv --with-traceback`:

see Travis logs

Discussion:

Patch:
diff --git a/docs/changelog/1809.bugfix.rst b/docs/changelog/1809.bugfix.rst
new file mode 100644
index 00000000..cbd1b063
--- /dev/null
+++ b/docs/changelog/1809.bugfix.rst
@@ -0,0 +1,1 @@
+Fix download fails with python 3.4 - by :user:`gaborbernat`.
diff --git a/docs/changelog/1813.bugfix.rst b/docs/changelog/1813.bugfix.rst
new file mode 100644
index 00000000..7a894bb4
--- /dev/null
+++ b/docs/changelog/1813.bugfix.rst
@@ -0,0 +1,1 @@
+Fix download is ``True`` by default - by :user:`gaborbernat`.
diff --git a/docs/changelog/1814.bugfix.rst b/docs/changelog/1814.bugfix.rst
new file mode 100644
index 00000000..f4195c4a
--- /dev/null
+++ b/docs/changelog/1814.bugfix.rst
@@ -0,0 +1,1 @@
+Fail ``app-data`` seed operation when wheel download fails and better error message - by :user:`gaborbernat`.
diff --git a/src/virtualenv/seed/embed/base_embed.py b/src/virtualenv/seed/embed/base_embed.py
index 1067a7b7..e98a72d1 100644
--- a/src/virtualenv/seed/embed/base_embed.py
+++ b/src/virtualenv/seed/embed/base_embed.py
@@ -45,13 +45,6 @@ class BaseEmbed(Seeder):
     @classmethod
     def add_parser_arguments(cls, parser, interpreter, app_data):
         group = parser.add_mutually_exclusive_group()
-        group.add_argument(
-            "--download",
-            dest="download",
-            action="store_true",
-            help="pass to enable download of the latest {} from PyPI".format("/".join(cls.packages)),
-            default=False,
-        )
         group.add_argument(
             "--no-download",
             "--never-download",
@@ -60,6 +53,13 @@ class BaseEmbed(Seeder):
             help="pass to disable download of the latest {} from PyPI".format("/".join(cls.packages)),
             default=True,
         )
+        group.add_argument(
+            "--download",
+            dest="download",
+            action="store_true",
+            help="pass to enable download of the latest {} from PyPI".format("/".join(cls.packages)),
+            default=False,
+        )
         parser.add_argument(
             "--extra-search-dir",
             metavar="d",
diff --git a/src/virtualenv/seed/embed/wheels/acquire.py b/src/virtualenv/seed/embed/wheels/acquire.py
index 72ab4a62..28ac6a77 100644
--- a/src/virtualenv/seed/embed/wheels/acquire.py
+++ b/src/virtualenv/seed/embed/wheels/acquire.py
@@ -21,7 +21,16 @@ from . import BUNDLE_SUPPORT, MAX
 BUNDLE_FOLDER = Path(os.path.abspath(__file__)).parent
 
 
-def get_wheels(for_py_version, wheel_cache_dir, extra_search_dir, download, packages, app_data):
+class WheelDownloadFail(ValueError):
+    def __init__(self, packages, for_py_version, exit_code, out, err):
+        self.packages = packages
+        self.for_py_version = for_py_version
+        self.exit_code = exit_code
+        self.out = out.strip()
+        self.err = err.strip()
+
+
+def get_wheels(for_py_version, wheel_cache_dir, extra_search_dir, packages, app_data, download):
     # not all wheels are compatible with all python versions, so we need to py version qualify it
     processed = copy(packages)
     # 1. acquire from bundle
@@ -147,11 +156,11 @@ def download_wheel(packages, for_py_version, to_folder, app_data):
     cmd.extend(to_download)
     # pip has no interface in python - must be a new sub-process
 
-    with pip_wheel_env_run("{}{}".format(*sys.version_info[0:2]), app_data) as env:
-        process = Popen(cmd, env=env, stdout=subprocess.PIPE)
-        process.communicate()
+    with pip_wheel_env_run("{}.{}".format(*sys.version_info[0:2]), app_data) as env:
+        process = Popen(cmd, env=env, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
+        out, err = process.communicate()
         if process.returncode != 0:
-            raise RuntimeError("failed to download wheels")
+            raise WheelDownloadFail(packages, for_py_version, process.returncode, out, err)
 
 
 @contextmanager
diff --git a/src/virtualenv/seed/via_app_data/via_app_data.py b/src/virtualenv/seed/via_app_data/via_app_data.py
index 0e3fb993..3d4b7c72 100644
--- a/src/virtualenv/seed/via_app_data/via_app_data.py
+++ b/src/virtualenv/seed/via_app_data/via_app_data.py
@@ -3,11 +3,12 @@ from __future__ import absolute_import, unicode_literals
 
 import logging
 from contextlib import contextmanager
+from functools import partial
 from threading import Lock, Thread
 
 from virtualenv.info import fs_supports_symlink
 from virtualenv.seed.embed.base_embed import BaseEmbed
-from virtualenv.seed.embed.wheels.acquire import get_wheels
+from virtualenv.seed.embed.wheels.acquire import WheelDownloadFail, get_wheels
 from virtualenv.util.path import safe_delete
 
 from .pip_install.copy import CopyPipInstall
@@ -63,26 +64,54 @@ class FromAppData(BaseEmbed):
             if wheels_to.exists():
                 safe_delete(wheels_to)
             wheels_to.mkdir(parents=True, exist_ok=True)
-            name_to_whl, lock = {}, Lock()
+            name_to_whl, lock, fail = {}, Lock(), {}
 
             def _get(package, version):
-                result = get_wheels(
+                wheel_loader = partial(
+                    get_wheels,
                     creator.interpreter.version_release_str,
                     wheels_to,
                     self.extra_search_dir,
-                    self.download,
                     {package: version},
                     self.app_data,
                 )
-                with lock:
-                    name_to_whl.update(result)
+                failure, result = None, None
+                # fallback to download in case the exact version is not available
+                for download in [True] if self.download else [False, True]:
+                    failure = None
+                    try:
+                        result = wheel_loader(download)
+                        if result:
+                            break
+                    except Exception as exception:
+                        failure = exception
+                if failure:
+                    if isinstance(failure, WheelDownloadFail):
+                        msg = "failed to download {}".format(package)
+                        if version is not None:
+                            msg += " version {}".format(version)
+                        msg += ", pip download exit code {}".format(failure.exit_code)
+                        output = failure.out + failure.err
+                        if output:
+                            msg += "\n"
+                            msg += output
+                    else:
+                        msg = repr(failure)
+                    logging.error(msg)
+                    with lock:
+                        fail[package] = version
+                else:
+                    with lock:
+                        name_to_whl.update(result)
 
-            threads = list(Thread(target=_get, args=(pkg, v)) for pkg, v in self.package_version().items())
+            package_versions = self.package_version()
+            threads = list(Thread(target=_get, args=(pkg, v)) for pkg, v in package_versions.items())
             for thread in threads:
                 thread.start()
             for thread in threads:
                 thread.join()
-
+            if fail:
+                raise RuntimeError("seed failed due to failing to download wheels {}".format(", ".join(fail.keys())))
             yield name_to_whl
 
     def installer_class(self, pip_version):


Test patch:
diff --git a/tests/unit/seed/test_base_embed.py b/tests/unit/seed/test_base_embed.py
new file mode 100644
index 00000000..379211c6
--- /dev/null
+++ b/tests/unit/seed/test_base_embed.py
@@ -0,0 +1,11 @@
+import pytest
+
+from virtualenv.run import session_via_cli
+
+
+@pytest.mark.parametrize(
+    "args, download", [([], False), (["--no-download"], False), (["--never-download"], False), (["--download"], True)]
+)
+def test_download_cli_flag(args, download, tmp_path):
+    session = session_via_cli(args + [str(tmp_path)])
+    assert session.seeder.download is download