Run pip in isolated env by building zip

[uranusjr]

This puts pip into a zip file, and change the isolated environment to run that instead of the pip installation in site-packages. This avoids other packages in site-packages to be injected into the isolated environment.

Fix #8214.

As discussed in #8214 (comment), this has a ~1s overhead for each call, so an sdist build can be slowed down for up to 2s (one for pyproject.toml and another for get_requires_for_build_wheel). There are possible optimisations, e.g. creating only one pip.zip for each pip invocation, but the globally managed state would introduce much complexity, so I’m not getting into them unless we decide this is the way to go.

[pfmoore]

In get-pip, we do some clever stuff to extract the certificate bundle, because openssl needs it to be an actual file. Is that likely to be a problem here? I've never actually had an issue where that was necessary myself, so I don't know why it's important, maybe someone who worked on get-pip would know?

[uranusjr]

There are some tests failing due to unexpected temp files at $TEMPDIR/pip/_vendor/certifi/cacert.pem, which seems related.

[pfmoore]

Ah, looks like certifi these days uses importlib.resources to get the cacert.pem file, which means it will unpack it on demand if necessary...

Which probably also means we can simplify the get-pip bootstrap, and maybe even just ship pip as a pyz for people who want that...

[uranusjr]

I read the implementation and unfortunately the logic only works on Python 3.7. So unless we add importlib-resources to the vendor list, we’ll still need to do the get-pip trick for this to work.

[uranusjr]

I tried simply copying the whole tree into a temp directory, the overhead is about twice as much (slightly under 2s per distribution), which could be a bit much?

Since we already have a real-file certificate bundle (in the original installation), I’m thinking we probably can just point --cert to that file to work around the issue.

[pfmoore]

Since we already have a real-file certificate bundle (in the original installation), I’m thinking we probably can just point --cert to that file to work around the issue.

That sounds like a better idea. I'm not keen on a 2s overhead, by the time you start adding in things like virus checkers, slow non-SSD disks, temp on NFS disks, old hardware, etc, these things get a lot worse than we expect, very fast 🙁

[uranusjr]

Huh, the test failures are about forking bomb pervention. It seems to me (I’m not very familiar with this part of pip and may be wrong) the requirement tracker is not correctly communicated between the parent and child pip, but can’t tell why making the child a zip would affect this 😞

[uranusjr]

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.

I think the only way to work around this is to patch certifi or requests to make where() always return parent’s bundle without triggering importlib.resources. But that is very hacky.

[uranusjr]

It’s ugly, but it’s working.

[pradyunsg]

Hmm... I wonder if a better mechanism would be to have a variable somewhere that we patch for the child process?

That way we can have a branch that does things only in the subprocess.

[pfmoore]

So you're suggesting that the actual pip code carries a vendor patch here? That sounds reasonable, we could patch certifi to recognise when it's being run under an isolated pip. Maybe something like replacing

try:
    from importlib.resources import path as get_path, read_text
    ...
except ImportError:
    ...

with

class EarlyExit(Exception):
    """Hack to do an early exit from the try-except"""

try:
    if "PIP__ISOLATED__CERT__" in os.environ:
        def where():
            return os.environ["PIP__ISOLATED__CERT__"]
        raise EarlyExit

    from importlib.resources import path as get_path, read_text
    ...
except ImportError:
    ...
except EarlyExit:
    pass

The EarlyExit hack avoids us needing to re-indent basically the whole of the file, which would make the patch much harder to maintain...

Edit: Use an environment variable, so we don't need to hack pip's main function.

[pradyunsg]

    if "PIP__ISOLATED__CERT__" in os.environ:

I'd even start the environment variable with an underscore, to make it clearer that it's not meant for "external users".

[uranusjr]

Sounds like a good idea, let me try this out.

[uranusjr]

Tests seem to pass (finally).

[pradyunsg]

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?

[uranusjr]

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.

[piotr-dobrogost]

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

[pfmoore]

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?

[uranusjr]

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.

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!