Keep empty in-project venv dir when recreating venv to allow for docker volumes#2064
Keep empty in-project venv dir when recreating venv to allow for docker volumes#2064finswimmer merged 1 commit intopython-poetry:masterfrom TheButlah:master
Conversation
TheButlah
changed the title
|
Looks like I'm failing a test. I've fixed it by updating the mocked out functions in |
TheButlah
changed the title
|
Looks like my fix worked. However, the MacOS tests are failing - it appears to be a github actions issue unrelated to my changes. |
|
Looks like my type annotations broke the python 2.7 version. Should I remove them? I thought poetry used features that are only present on versions more recent than 2.7? Type annotations to distinguish the difference between a function taking a |
|
Removed the type annotations I added |
|
@finswimmer I resolved the comment so we can continue the discussion, if needed, here. I believe this PR is ready for final review after addressing your requested changes. |
finswimmer
left a comment
finswimmer
left a comment
There was a problem hiding this comment.
Hello @TheButlah,
sorry I was to fast with my opinion that empty the folder is always enough. :( The remove_venv method is also used, when one will remove a venv by poetry env remove. So this will leave behind an empty folder now.
What we can do is, to first try rmtree and only if this raises an OSError empty the folder.
Sorry again, that I haven't this in mind earlier.
|
Sounds good. Shall I write all the tests under the assumption that the directory will be successfully deleted, as before? And then just provide a new, separate test case that checks to ensure the directory remains but is empty in the event that an OSError is thrown? Or should I somehow test out both OSError and no error in all the existing tests (I'm not sure how I would go about that)? |
This is what I would do 😃 |
|
OK @finswimmer sorry for the delay. I've updated the code so that we check for the OSError and handle that case. Let me know if there's anything else do be done! |
|
@finswimmer any updates on this? It should be ready for a merge. The mac tests fail but I think thats a CI issue on your end - I ran the tests on my mac locally and it worked just fine. |
|
@sdispater @finswimmer any updates on this? I'm happy to help if there's anything further that this MR needs to do |
|
I suspect this is what currently blocks me from mounting the .venv folder as a separate mount point from the rest of my python project, inside a docker container. |
|
I gave up constantly rebasing and pinging because no one responded :( |
|
Yeah, I don't blame you. I'm just trying to raise awareness to this PR again, to hopefully get it in there. Seems like a simple and clean solution for that problem. Thank you for making it in the first place. |
|
Great! Thanks :) |
|
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. |
3 participants
In this PR, I have made it so that should a venv folder be rebuilt due to it being considered broken, the folder will be emptied but not deleted before repopulating it.
I've been running into issues when attempting to install poetry in a docker container with a mounted volume for the venv. My workflow is as follows:
~/myproject/.venvandvirtualenvs.in-projectset totruepoetry installThis results in the following error:
This is because when a docker volume is mounted to a folder, you cannot delete that folder. With this fix, no attempt to delete the folder will be made, instead only the contents will be deleted. This allows poetry to install the venv to a docker volume.