Fix --pip for pre-existing venvs w/o Pip.

Expand test coverage and improve option help.

pex/cli/commands/lock.py

@@ -133,6 +133,8 @@ def __call__(self, parser, namespace, value, option_str=None):
 
 @attr.s(frozen=True)
 class SyncTarget(object):
+    _PIP_PROJECT_NAME = ProjectName("pip")
+
     @classmethod
     def resolve_command(
         cls,
@@ -213,10 +215,16 @@ def sync(
         existing_distributions_by_project_name = {
             dist.metadata.project_name: dist for dist in self.venv.iter_distributions()
         }  # type: Dict[ProjectName, Distribution]
+        installed_pip = existing_distributions_by_project_name.get(
+            self._PIP_PROJECT_NAME
+        )  # type: Optional[Distribution]
 
+        resolved_pip = None  # type: Optional[Distribution]
         to_remove = []  # type: List[Distribution]
         to_install = []  # type: List[Distribution]
         for distribution in distributions:
+            if self._PIP_PROJECT_NAME == distribution.metadata.project_name:
+                resolved_pip = distribution
             existing_distribution = existing_distributions_by_project_name.pop(
                 distribution.metadata.project_name, None
             )
@@ -226,7 +234,7 @@ def sync(
                 to_remove.append(existing_distribution)
                 to_install.append(distribution)
         if retain_pip:
-            existing_distributions_by_project_name.pop(ProjectName("pip"), None)
+            existing_distributions_by_project_name.pop(self._PIP_PROJECT_NAME, None)
         to_remove.extend(existing_distributions_by_project_name.values())
 
         to_unlink_by_pin = (
@@ -282,6 +290,9 @@ def sync(
                 ):
                     os.rmdir(parent_dir)
 
+        if retain_pip and not resolved_pip and not installed_pip:
+            self.venv.install_pip(upgrade=True)
+
         if to_install:
             for distribution in to_install:
                 for src, dst in InstalledWheel.load(distribution.location).reinstall_venv(
@@ -679,11 +690,14 @@ def _add_sync_arguments(cls, sync_parser):
             action=HandleBoolAction,
             default=False,
             help=(
-                "When creating a venv that doesn't exist, install pip in it. When syncing an "
-                "existing venv, retain its pip, if any, even if pip is not present in the lock "
-                "file being synced. N.B.: When in `--no-retain-pip` mode (the default) only remove "
-                "pip if it is both present in the venv and not present in the lock file being "
-                "synced."
+                "When syncing a venv in the default `--no-pip`/`--no-retain-pip` mode, new venvs "
+                "will be created without Pip installed in them and existing venvs will have Pip "
+                "removed unless the lock being synced specifies a locked Pip, in which case that "
+                "locked Pip version will be ensured. When syncing a venv in the "
+                "`--pip`/`--retain-pip` mode, new venvs will be created with Pip installed in them "
+                "and existing venvs will have Pip installed. The version of Pip installed will be "
+                "taken from the lock being synced is present, and will be the latest compatible "
+                "with the venv interpreter otherwise."
             ),
         )
         sync_parser.add_argument(
@@ -1446,8 +1460,6 @@ def _sync(self):
                         )
                     interpreter = interpreters[0]
                 venv = Virtualenv.create(self.options.venv, interpreter=interpreter)
-                if self.options.pip:
-                    venv.install_pip(upgrade=True)
             sync_target = SyncTarget.resolve_command(venv=venv, command=self.passthrough_args)
         elif self.passthrough_args:
             sync_target = try_(

tests/integration/cli/commands/test_lock_sync.py

@@ -284,10 +284,9 @@ def test_sync_noop(
         path_mappings,
         expected_pins=[pin("cowsay", "5.0"), pin("foo", "1"), pin("bar", "1")],
     )
-    result = run_sync(
+    run_sync(
         "cowsay", "foo", "bar", "--lock", initial_lock, *(repo_args + path_mapping_args)
-    )
-    result.assert_success(
+    ).assert_success(
         expected_output_re=NO_OUTPUT,
         expected_error_re=re_exact(
             "No updates for lock generated by {platform}.".format(
@@ -316,10 +315,9 @@ def test_sync_update(
         expected_pins=[pin("cowsay", "5.0"), pin("foo", "1"), pin("bar", "1")],
     )
 
-    result = run_sync(
+    run_sync(
         "cowsay", "foo>1", "bar", "--lock", initial_lock, *(repo_args + path_mapping_args)
-    )
-    result.assert_success(
+    ).assert_success(
         expected_output_re=NO_OUTPUT,
         expected_error_re=re_exact(
             dedent(
@@ -353,10 +351,9 @@ def test_sync_add(
         expected_pins=[pin("cowsay", "5.0"), pin("foo", "1"), pin("bar", "1")],
     )
 
-    result = run_sync(
+    run_sync(
         "cowsay", "foo", "bar", "baz", "--lock", initial_lock, *(repo_args + path_mapping_args)
-    )
-    result.assert_success(
+    ).assert_success(
         expected_output_re=NO_OUTPUT,
         expected_error_re=re_exact(
             dedent(
@@ -390,8 +387,9 @@ def test_sync_remove(
         expected_pins=[pin("cowsay", "5.0"), pin("foo", "1"), pin("bar", "1")],
     )
 
-    result = run_sync("cowsay", "foo", "--lock", initial_lock, *(repo_args + path_mapping_args))
-    result.assert_success(
+    run_sync(
+        "cowsay", "foo", "--lock", initial_lock, *(repo_args + path_mapping_args)
+    ).assert_success(
         expected_output_re=NO_OUTPUT,
         expected_error_re=re_exact(
             dedent(
@@ -426,10 +424,9 @@ def test_sync_complex(
     )
 
     # N.B.: Update foo, remove bar, add baz.
-    result = run_sync(
+    run_sync(
         "cowsay", "foo<3", "baz<2", "--lock", initial_lock, *(repo_args + path_mapping_args)
-    )
-    result.assert_success(
+    ).assert_success(
         expected_output_re=NO_OUTPUT,
         expected_error_re=re_exact(
             dedent(
@@ -804,8 +801,48 @@ def test_sync_venv_dry_run(
     )
 
 
+def test_sync_venv_run_no_retain_pip_preinstalled(
+    tmpdir,  # type: Any
+    repo_args,  # type: List[str]
+    path_mappings,  # type: PathMappings
+):
+    # type: (...) -> None
+
+    venv_dir = os.path.join(str(tmpdir), "venv")
+    venv = Virtualenv.create(venv_dir)
+    venv.install_pip()
+    pip = find_distribution("pip", search_path=venv.sys_path)
+    assert pip is not None
+
+    subprocess.check_call(args=[venv.bin_path("pip"), "install", "cowsay==5.0"] + repo_args)
+    assert_cowsay5(venv)
+
+    lock = os.path.join(str(tmpdir), "lock.json")
+    run_sync(
+        *(
+            repo_args
+            + [
+                "--no-retain-pip",
+                "--yes",
+                "cowsay==5.0",
+                "--lock",
+                lock,
+                "--",
+                venv.bin_path("cowsay"),
+                "-t",
+                "Moo Two!",
+            ]
+        )
+    ).assert_success(
+        expected_output_re=r".*| Moo Two! |.*", expected_error_re=NO_OUTPUT, re_flags=re.DOTALL
+    )
+    assert_lock_matches_venv(
+        lock=lock, venv=venv, path_mappings=path_mappings, expected_pins=[pin("cowsay", "5.0")]
+    )
+
+
 @skip_cowsay6_for_python27
-def test_sync_venv_run_retain_pip(
+def test_sync_venv_run_retain_pip_preinstalled(
     tmpdir,  # type: Any
     repo_args,  # type: List[str]
     path_mappings,  # type: PathMappings
@@ -823,7 +860,7 @@ def test_sync_venv_run_retain_pip(
     assert_cowsay5(venv)
 
     lock = os.path.join(str(tmpdir), "lock.json")
-    result = run_sync(
+    run_sync(
         *(
             repo_args
             + [
@@ -838,9 +875,9 @@ def test_sync_venv_run_retain_pip(
                 "A New Moo!",
             ]
         )
+    ).assert_success(
+        expected_output_re=r".*| A New Moo! |.*", expected_error_re=NO_OUTPUT, re_flags=re.DOTALL
     )
-    result.assert_success(expected_output_re=r".*| A New Moo! |.*", re_flags=re.DOTALL)
-    assert not result.error
 
     assert_lock(lock, path_mappings, expected_pins=[pin("cowsay", "6.0")])
     assert_venv(venv_dir, expected_pins=[pin("cowsay", "6.0"), pip_pin])
@@ -853,6 +890,66 @@ def test_sync_venv_run_retain_pip(
     assert_cowsay5(venv)
 
 
+def test_sync_venv_run_retain_pip_no_pip_preinstalled(
+    tmpdir,  # type: Any
+    repo_args,  # type: List[str]
+    path_mappings,  # type: PathMappings
+):
+    # type: (...) -> None
+
+    venv_dir = os.path.join(str(tmpdir), "venv")
+    lock = os.path.join(str(tmpdir), "lock.json")
+    run_sync(
+        *(
+            repo_args
+            + [
+                "--no-pip",
+                "cowsay==5.0",
+                "--lock",
+                lock,
+                "--venv",
+                venv_dir,
+                "--",
+                "cowsay",
+                "Moo!",
+            ]
+        )
+    ).assert_success(
+        expected_output_re=r".*| Moo! |.*", expected_error_re=NO_OUTPUT, re_flags=re.DOTALL
+    )
+    lock_file = assert_lock_matches_venv(
+        lock=lock, path_mappings=path_mappings, venv=venv_dir, expected_pins=[pin("cowsay", "5.0")]
+    )
+
+    venv = Virtualenv(venv_dir)
+    run_sync(
+        *(
+            repo_args
+            + [
+                "--pip",
+                "cowsay==5.0",
+                "--lock",
+                lock,
+                "--",
+                venv.bin_path("cowsay"),
+                "Moo Two!",
+            ]
+        )
+    ).assert_success(
+        expected_output_re=r".*| Moo Two! |.*",
+        expected_error_re=re_exact(
+            "No updates for lock generated by {platform}.".format(
+                platform=lock_file.locked_resolves[0].platform_tag
+            )
+        ),
+        re_flags=re.DOTALL,
+    )
+    assert_lock(lock, path_mappings, expected_pins=[pin("cowsay", "5.0")])
+
+    pip = find_distribution("pip", search_path=venv.sys_path, rescan=True)
+    assert pip is not None
+
+
 @skip_cowsay6_for_python27
 @pytest.mark.parametrize(
     "retain_pip_args",