Pip: When using --target, exceptions can leave a temp directory behind

[pfmoore]

Right, the problem with target_temp_dir is that it's created conditionally - so sometimes it's a context manager, and other times it's None. So we can't "just" enter it earlier, as you can't do with None.

When we drop Python 2, we could fix this with contextlib.ExitStack. This isn't worth vendoring the contextlib backport just for Python 2, though. We could also write our own MaybeContextManager class that wraps an optional context manager, but again that's a lot of work.

I'm going to make the call that this should be handled in a follow-up PR, and for now just allow left-over temp directories in the test.

_Originally posted by @pfmoore in https://github.com/pypa/pip/pull/8462#issuecomment-649349135_

[McSinyx]

When we drop Python 2, we could fix this with contextlib.ExitStack

Hi, FYI pip is already vendoring contextlib2.

[pfmoore]

Ooh, I didn't know that! Thanks for the pointer. In which case, I'll take a look at using ExitStack for this.

[pfmoore]

And actually, that led me to CommandContextMixIn and the fact that all of our commands have a built in exit stack - which is really cool, and makes fixing this a lot easier than I thought. I hope - just running the tests now 🙂

Thanks!

[pradyunsg]

@pfmoore did you fix this yet? ;)

[pfmoore]

No, I think I dropped the ball on this one. If I had a branch with a fix in, I'm not sure where it's gone 🙁

[McSinyx]

@pfmoore, I think you've already fixed it in f1622363606654ca18e27bbcfd13f56f6f40014b :smile:

[uranusjr]

Gosh, I knew I’ve seen that CommandContextMixIn implementation before. I thought I was hallucinating or something.

[pfmoore]

@pfmoore, I think you've already fixed it in f162236 😄

I thought it rang a bell 😄

Too many issues in the tracker, not enough cells in my brain...