Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run pip in isolated env by building zip #9689

Merged
merged 10 commits into from
Apr 3, 2021
2 changes: 2 additions & 0 deletions news/8214.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Prevent packages already-installed alongside with pip to be injected into an
isolated build environment during build-time dependency population.
51 changes: 48 additions & 3 deletions src/pip/_internal/build_env.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
"""Build Environment used for isolation during sdist building
"""

import contextlib
import logging
import os
import pathlib
import sys
import textwrap
import zipfile
from collections import OrderedDict
from sysconfig import get_paths
from types import TracebackType
from typing import TYPE_CHECKING, Iterable, List, Optional, Set, Tuple, Type
from typing import TYPE_CHECKING, Iterable, Iterator, List, Optional, Set, Tuple, Type

from pip._vendor.certifi import where
from pip._vendor.pkg_resources import Requirement, VersionConflict, WorkingSet

from pip import __file__ as pip_location
Expand Down Expand Up @@ -37,6 +41,29 @@ def __init__(self, path):
self.lib_dirs = get_prefixed_libs(path)


@contextlib.contextmanager
def _create_standalone_pip() -> Iterator[str]:
"""Create a "standalone pip" zip file.

The zip file's content is identical to the currently-running pip.
It will be used to install requirements into the build environment.
"""
source = pathlib.Path(pip_location).resolve().parent

# Return the current instance if it is already a zip file. This can happen
# if a PEP 517 requirement is an sdist itself.
if not source.is_dir() and source.parent.name == "__env_pip__.zip":
yield str(source)
return

with TempDirectory(kind="standalone-pip") as tmp_dir:
pip_zip = os.path.join(tmp_dir.path, "__env_pip__.zip")
with zipfile.ZipFile(pip_zip, "w") as zf:
for child in source.rglob("*"):
zf.write(child, child.relative_to(source.parent).as_posix())
yield os.path.join(pip_zip, "pip")


class BuildEnvironment:
"""Creates and manages an isolated environment to install build deps
"""
Expand Down Expand Up @@ -160,8 +187,25 @@ def install_requirements(
prefix.setup = True
if not requirements:
return
with _create_standalone_pip() as standalone_pip:
self._install_requirements(
standalone_pip,
finder,
requirements,
prefix,
message,
)

@staticmethod
def _install_requirements(
standalone_pip: str,
finder: "PackageFinder",
requirements: Iterable[str],
prefix: _Prefix,
message: str,
) -> None:
args = [
sys.executable, os.path.dirname(pip_location), 'install',
sys.executable, standalone_pip, 'install',
'--ignore-installed', '--no-user', '--prefix', prefix.path,
'--no-warn-script-location',
] # type: List[str]
Expand Down Expand Up @@ -190,8 +234,9 @@ def install_requirements(
args.append('--prefer-binary')
args.append('--')
args.extend(requirements)
extra_environ = {"_PIP_STANDALONE_CERT": where()}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, and I'm not suggesting that you try updating this PR, but I do wonder: can we use the --cert option that we have on the CLI to achieve this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works in practice, but some tests will fail because requests always triggers temp file creation even when the default cert is never used. The environ + certifi patch solution is the only way I can think of to prevent that temp file from being created entirely.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I managed to fix the fork bomb, but the tests are still failing due to unexpected temp files. This is because requests calls certifi.where() unconditionally, and a temporary file is created to read the cert bundle even if we pass in --cert explicitly.

From #9689 (comment) in this thread

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming that there's a reason we can't just mark these as "expected temp files" in the test suite, or a reason why the temp files are bad in real usage - and we're not introducing a complicated hack just to cover over a test issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are tests on temp files being correctly cleaned up after build, and allowing temp files would defeat their purpose. Another way to do this is to ensure the copied cert file is also cleaned up by the PEP 517 process, but that would require even more patching in certifi.

with open_spinner(message) as spinner:
call_subprocess(args, spinner=spinner)
call_subprocess(args, spinner=spinner, extra_environ=extra_environ)


class NoOpBuildEnvironment(BuildEnvironment):
Expand Down
16 changes: 16 additions & 0 deletions src/pip/_vendor/certifi/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,21 @@
"""
import os


class _PipPatchedCertificate(Exception):
pass


try:
# Return a certificate file on disk for a standalone pip zipapp running in
# an isolated build environment to use. Passing --cert to the standalone
# pip does not work since requests calls where() unconditionally on import.
_PIP_STANDALONE_CERT = os.environ.get("_PIP_STANDALONE_CERT")
if _PIP_STANDALONE_CERT:
def where():
return _PIP_STANDALONE_CERT
raise _PipPatchedCertificate()

from importlib.resources import path as get_path, read_text

_CACERT_CTX = None
Expand Down Expand Up @@ -38,6 +52,8 @@ def where():

return _CACERT_PATH

except _PipPatchedCertificate:
pass

except ImportError:
# This fallback will work for Python versions prior to 3.7 that lack the
Expand Down
34 changes: 31 additions & 3 deletions tools/vendoring/patches/certifi.patch
Original file line number Diff line number Diff line change
@@ -1,13 +1,41 @@
diff --git a/src/pip/_vendor/certifi/core.py b/src/pip/_vendor/certifi/core.py
index 5d2b8cd32..8987449f6 100644
index 5d2b8cd32..b8140cf1a 100644
--- a/src/pip/_vendor/certifi/core.py
+++ b/src/pip/_vendor/certifi/core.py
@@ -33,7 +33,7 @@ try:
@@ -8,7 +8,21 @@ This module returns the installation location of cacert.pem or its contents.
"""
import os

+
+class _PipPatchedCertificate(Exception):
+ pass
+
+
try:
+ # Return a certificate file on disk for a standalone pip zipapp running in
+ # an isolated build environment to use. Passing --cert to the standalone
+ # pip does not work since requests calls where() unconditionally on import.
+ _PIP_STANDALONE_CERT = os.environ.get("_PIP_STANDALONE_CERT")
+ if _PIP_STANDALONE_CERT:
+ def where():
+ return _PIP_STANDALONE_CERT
+ raise _PipPatchedCertificate()
+
from importlib.resources import path as get_path, read_text

_CACERT_CTX = None
@@ -33,11 +47,13 @@ try:
# We also have to hold onto the actual context manager, because
# it will do the cleanup whenever it gets garbage collected, so
# we will also store that at the global level as well.
- _CACERT_CTX = get_path("certifi", "cacert.pem")
+ _CACERT_CTX = get_path("pip._vendor.certifi", "cacert.pem")
_CACERT_PATH = str(_CACERT_CTX.__enter__())

return _CACERT_PATH

+except _PipPatchedCertificate:
+ pass

except ImportError:
# This fallback will work for Python versions prior to 3.7 that lack the