retire temporary_directory()

[dimbleby]

removing poetry's own temporary_directory() implementation, prefer tempfile.TemporaryDirectory().

There are already examples of TemporaryDirectory() in the codebase so this isn't breaking new ground. I assume the other was legacy of python2 support or somesuch.

[abn]

Note the force implementation in remove_directory. The reason why it exists is twofolds.

1. Git directories can be read only causing issues on windows. Issue with installing GitPython on Windows 10 / Python 3.7 #1243
2. Certain sdists can have similar issues. Poetry fails to install p4python due to read-only files #520.

Will probably need to verify that the temfile implementation handles that well.

[dimbleby]

I'd have much more confidence in the language implementation of TemporaryDirectory() than in a home-grown version when it comes to dealing with edge cases!

And indeed it looks to do all that remove_directory() does and more: https://github.com/python/cpython/blob/e91dee87edcf6dee5dd78053004d76e5f05456d4/Lib/tempfile.py#L808-L836

[abn]

I agree the system implementation is preferred. However, I'd want to make sure we are not regressing here somehow.

[dimbleby]

Well, there's a testcase over in poetry-core for this. So the pipeline from python-poetry/poetry-core#337 should tell us.

[dimbleby]

Colour me surprised, recent releases behave as expected but Windows and python 3.7 do indeed go wrong.

So I guess this will have to wait until such time as python 3.7 is out of support,

[dimbleby]

https://bugs.python.org/issue26660

[abn]

We should add some comments around it and possibly move it into _compat to signal it's intent .

[neersighted]

I ran into this when ripping out Python 2.7 support -- which is why it got left behind, but without comments evidently. I would suggest making this a wrapper for stdlib version that only uses the home-grown version on Python 3.7. That way the Python version can be grepped in the future and we can robustly test the stdlib version for future inclusion.

[neersighted]

Going to merge this in Poetry and link my suggestion in the PR where it actually matters.

[dimbleby]

@neersighted I don't think this wanted merging yet - apologies for leaving it in a state where this was not clear

• While poetry.core.utils.helpers.temporary_directory still exists, after merging this MR there are no users of it outside of the poetry-core unit tests.
• even though this repository had no unit test proving the point, I suppose it still had a reason for avoidingTemporaryDirectory (which is faulty on Windows python 3.7)

I agree that a single implementation of temporary_directory() in just one of the repositories would be enough: but that implementation does want using, until we can drop python 3.7

[neersighted]

Hmm, I'm not fully convinced that we need it in Poetry given our particular uses of TemporaryDirectory -- can we replicate the failing test on Python 3.7 in Poetry itself? I was rather under the impression that we don't trigger the bug given our usage in this repo.

[dimbleby]

I'm afraid that trying to do that is more trouble than I can be bothered with!

I don't know what would have changed to stop poetry from hitting this since all the previous reports. eg _get_info_from_wheel() and _get_info_from_sdist() can still potentially download and extract archives containing read-only things.

If you're somehow convinced that it's not possible for poetry to hit this problem any more then I guess that's fine by me, but I'd think that it's sensible to err on the side of caution, we can tidy up this nonsense in a year or so when dropping support for python 3.7.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.