Bug 1730712: Remove external use of _run_pip() r=perftest-reviewers,ahal,sparky

As `_run_pip()` is being removed from `VirtualenvManager` in an upcoming
patch, its usages need to be removed. Besides, they're using an
"internal" function, which is a bit of a smell.

Note that this _could_ have been solved by exposing a public `run_pip()`
function. However, I felt like that was worse because:
* Friction here is good as we try to migrate the codebase to embrace the
  "requirements definition file" technique to install dependencies.
* There could be confusion about the relationship between
  `install_pip_package()` (only works if venv already activated)
  and `_run_pip()`, which works "in general".

Differential Revision: https://phabricator.services.mozilla.com/D130120
This commit is contained in:
Mitchell Hentges 2021-11-16 21:14:41 +00:00
parent ea484ce0b5
commit b7b1442242
7 changed files with 66 additions and 16 deletions

View File

@ -7,6 +7,7 @@ from __future__ import absolute_import, print_function, unicode_literals
import argparse
import logging
import os
import subprocess
import sys
import tempfile
from multiprocessing import cpu_count
@ -101,9 +102,17 @@ def python(
python_path = which("ipython", path=bindir)
if not python_path:
if not no_virtualenv:
# Use `_run_pip` directly rather than `install_pip_package` to bypass
# Use `pip` directly rather than `install_pip_package()` to bypass
# `req.check_if_exists()` which may detect a system installed ipython.
command_context.virtualenv_manager._run_pip(["install", "ipython"])
subprocess.check_call(
[
command_context.virtualenv_manager.python_path,
"-m",
"pip",
"install",
"ipython",
]
)
python_path = which("ipython", path=bindir)
if not python_path:

View File

@ -60,8 +60,11 @@ class VendorPython(MozbuildObject):
with TemporaryDirectory() as tmp:
# use requirements.txt to download archived source distributions of all packages
self.virtualenv_manager._run_pip(
subprocess.check_call(
[
self.virtualenv_manager.python_path,
"-m",
"pip",
"download",
"-r",
tmp_requirements_absolute,

View File

@ -3,6 +3,8 @@
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
import sys
from subprocess import CalledProcessError
import mozunit
from unittest import mock
import pytest
@ -80,8 +82,17 @@ def _req(package):
def test_install_package():
vem = mock.Mock()
vem.bin_path = "someplace"
assert install_package(vem, "foo")
vem._run_pip.assert_called()
with mock.patch("subprocess.check_call") as mock_check_call:
assert install_package(vem, "foo")
mock_check_call.assert_called_once_with(
[
vem.python_path,
"-m",
"pip",
"install",
"foo",
]
)
@mock.patch("pip._internal.req.constructors.install_req_from_line", new=_req)
@ -89,13 +100,12 @@ def test_install_package_failures():
vem = mock.Mock()
vem.bin_path = "someplace"
def run_pip(*args):
raise Exception()
def check_call(*args):
raise CalledProcessError(1, "")
vem._run_pip = run_pip
with pytest.raises(Exception):
install_package(vem, "foo")
with pytest.raises(CalledProcessError):
with mock.patch("subprocess.check_call", new=check_call):
install_package(vem, "foo")
# we can also absorb the error, and just return False
assert not install_package(vem, "foo", ignore_failure=True)

View File

@ -151,7 +151,9 @@ def install_package(virtualenv_manager, package, ignore_failure=False):
return True
with silence():
try:
virtualenv_manager._run_pip(["install", package])
subprocess.check_call(
[virtualenv_manager.python_path, "-m", "pip", "install", package]
)
return True
except Exception:
if not ignore_failure:

View File

@ -4,6 +4,7 @@ import json
import os
import re
import signal
import subprocess
import threading
import time
@ -32,7 +33,16 @@ def _install_package(virtualenv_manager, package):
if site_packages.startswith(venv_site_lib):
# already installed in this venv, we can skip
return
virtualenv_manager._run_pip(["install", package])
subprocess.check_call(
[
virtualenv_manager.python_path,
"-m",
"pip",
"install",
package,
]
)
def _kill_mozproxy(pid):

View File

@ -38,6 +38,7 @@ import logging
import os
import re
import stat
import subprocess
import sys
import time
@ -463,7 +464,15 @@ def activate_browsertime_virtualenv(command_context, *args, **kwargs):
# installing Python deps on the fly
for dep in ("Pillow==%s" % PILLOW_VERSION, "pyssim==%s" % PYSSIM_VERSION):
if _need_install(command_context, dep):
command_context.virtualenv_manager._run_pip(["install", dep])
subprocess.check_call(
[
command_context.virtualenv_manager.python_path,
"-m",
"pip",
"install",
dep,
]
)
def check(command_context):

View File

@ -5,6 +5,7 @@
from __future__ import absolute_import, print_function, unicode_literals
import argparse
import subprocess
from datetime import datetime, timedelta
import logging
from operator import itemgetter
@ -394,8 +395,14 @@ class PypiBasedTool:
print(release)
# there is one, so install it. Note that install_pip_package
# does not work here, so just run pip directly.
cmd.virtualenv_manager._run_pip(
["install", "%s==%s" % (self.pypi_name, release)]
subprocess.check_call(
[
cmd.virtualenv_manager.python_path,
"-m",
"pip",
"install",
f"{self.pypi_name}=={release}",
]
)
print(
"%s was updated to version %s. please"