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

Created on 25 Jun 2020  路  8Comments  路  Source: pypa/pip

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_

target

All 8 comments

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

Hi, FYI pip is already vendoring contextlib2.

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

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!

@pfmoore did you fix this yet? ;)

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 馃檨

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

Gosh, I knew I鈥檝e seen that CommandContextMixIn implementation before. I thought I was hallucinating or something.

@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...

Was this page helpful?
0 / 5 - 0 ratings