Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep empty in-project venv dir when recreating venv to allow for docker volumes #2064

Merged
merged 1 commit into from Jun 19, 2020

Conversation

TheButlah
Copy link
Contributor

@TheButlah TheButlah commented Feb 20, 2020

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:

  1. Build the image
  2. Run the development container with a volume mounted at ~/myproject/.venv and virtualenvs.in-project set to true
  3. Attempt to poetry install

This results in the following error:

The virtual environment found in /home/myuser/myproject/.venv seems to be broken.
Recreating virtualenv robotics in /home/myuser/myproject/.venv

[OSError]
[Errno 16] Device or resource busy: '/home/myuser/myproject/.venv'

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.

@TheButlah TheButlah changed the title Now keeping empty venv dir when recreating venv Keeping empty venv dir when recreating venv to allow for docker volumes Feb 20, 2020
@TheButlah
Copy link
Contributor Author

@TheButlah TheButlah commented Feb 20, 2020

Looks like I'm failing a test. I've fixed it by updating the mocked out functions in test_env.py to reflect the changes I made in EnvManager.

@TheButlah TheButlah changed the title Keeping empty venv dir when recreating venv to allow for docker volumes Keep empty in-project venv dir when recreating venv to allow for docker volumes Feb 20, 2020
@TheButlah
Copy link
Contributor Author

@TheButlah TheButlah commented Feb 20, 2020

Looks like my fix worked. However, the MacOS tests are failing - it appears to be a github actions issue unrelated to my changes.

@TheButlah TheButlah requested a review from finswimmer Feb 20, 2020
poetry/utils/env.py Outdated Show resolved Hide resolved
@TheButlah TheButlah force-pushed the master branch 2 times, most recently from cc7f246 to aa7eb50 Compare Feb 24, 2020
@TheButlah
Copy link
Contributor Author

@TheButlah TheButlah commented Feb 24, 2020

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 Path vs a str are really helpful in my experience.

@TheButlah
Copy link
Contributor Author

@TheButlah TheButlah commented Feb 28, 2020

Removed the type annotations I added

@TheButlah TheButlah force-pushed the master branch 11 times, most recently from 5c1c16a to 445fe5c Compare Mar 2, 2020
@TheButlah
Copy link
Contributor Author

@TheButlah TheButlah commented Mar 2, 2020

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

Copy link
Member

@finswimmer finswimmer left a 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.

poetry/utils/env.py Outdated Show resolved Hide resolved
@TheButlah
Copy link
Contributor Author

@TheButlah TheButlah commented Mar 3, 2020

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)?

@finswimmer
Copy link
Member

@finswimmer finswimmer commented Mar 3, 2020

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?

This is what I would do 😃

@TheButlah TheButlah reopened this Mar 21, 2020
@TheButlah TheButlah force-pushed the master branch 7 times, most recently from d769231 to 0864b64 Compare Mar 22, 2020
@TheButlah TheButlah requested a review from finswimmer Mar 22, 2020
@TheButlah
Copy link
Contributor Author

@TheButlah TheButlah commented Mar 22, 2020

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!

@TheButlah
Copy link
Contributor Author

@TheButlah TheButlah commented Apr 9, 2020

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

@TheButlah
Copy link
Contributor Author

@TheButlah TheButlah commented Apr 13, 2020

@sdispater @finswimmer any updates on this? I'm happy to help if there's anything further that this MR needs to do

@ATGardner
Copy link

@ATGardner ATGardner commented Jun 18, 2020

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.
Will it be merged anytime soon?

@TheButlah
Copy link
Contributor Author

@TheButlah TheButlah commented Jun 18, 2020

I gave up constantly rebasing and pinging because no one responded :(

@ATGardner
Copy link

@ATGardner ATGardner commented Jun 19, 2020

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.

Copy link
Member

@finswimmer finswimmer left a comment

Sorry for the delay. It's always good to ping us on a regular bases 👍

LGTM and thank you very much for your contribution.

@finswimmer finswimmer merged commit 92db1f2 into python-poetry:master Jun 19, 2020
16 checks passed
@TheButlah
Copy link
Contributor Author

@TheButlah TheButlah commented Jun 19, 2020

Great! Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants