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

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

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

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

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

Removed the type annotations I added

@TheButlah
Copy link
Contributor Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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 closed this Mar 21, 2020
@TheButlah TheButlah reopened this Mar 21, 2020
@TheButlah
Copy link
Contributor Author

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

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

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

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

@ATGardner
Copy link

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@TheButlah
Copy link
Contributor Author

Great! Thanks :)

Copy link

github-actions bot commented Mar 1, 2024

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants