From 16d213ffc80ee7fe4fcaf23e6c4f42a58c6cc67c Mon Sep 17 00:00:00 2001 From: John Sirois Date: Fri, 23 Feb 2024 14:28:18 -0800 Subject: [PATCH] Fix `--pip` for pre-existing venvs w/o Pip. Expand test coverage and improve option help. --- pex/cli/commands/lock.py | 28 ++-- .../cli/commands/test_lock_sync.py | 133 +++++++++++++++--- 2 files changed, 135 insertions(+), 26 deletions(-) diff --git a/pex/cli/commands/lock.py b/pex/cli/commands/lock.py index e0ad2d266..dc3e2823b 100644 --- a/pex/cli/commands/lock.py +++ b/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_( diff --git a/tests/integration/cli/commands/test_lock_sync.py b/tests/integration/cli/commands/test_lock_sync.py index 17c5d60fd..1a149f849 100644 --- a/tests/integration/cli/commands/test_lock_sync.py +++ b/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",