Account for symlinked .venv when removing#3375
Conversation
|
|
||
| def err_on_rm_venv_only(path, *args, **kwargs): | ||
| if path == str(venv_path): | ||
| raise OSError(20, "Test error") # ERRNO 16: Device or resource busy |
There was a problem hiding this comment.
Looks like this comment is incorrect.
|
|
||
| # Delete all files and folders but the toplevel one. This is because sometimes | ||
| # the venv folder is mounted by the OS, such as in a docker volume. In such | ||
| # the venv folder is mounted or symlinked by the OS, such as in a docker volume, codebuild, etc. In such |
There was a problem hiding this comment.
Please reformat this comment block to have a uniform width.
| except OSError as e: | ||
| # Continue only if e.errno == 16 | ||
| if e.errno != 16: # ERRNO 16: Device or resource busy | ||
| if ( |
There was a problem hiding this comment.
I'd like to see the errno module's named constants used here. A construct like e.errno in {errno.ENOTDIR, errno.EBUSY} would be more obvious and maintainable.
| @@ -834,13 +834,16 @@ def remove_venv(cls, path): # type: (Union[Path,str]) -> None | |||
| return | |||
| except OSError as e: | |||
| # Continue only if e.errno == 16 | |||
| # the venv folder is mounted or symlinked by the OS, such as in a docker volume, codebuild, etc. In such | ||
| # cases, an attempt to delete the folder itself will result in an `OSError`. | ||
| # See https://github.com/python-poetry/poetry/pull/2064 | ||
| # See https://bugs.python.org/issue1669 |
There was a problem hiding this comment.
I don't think this issue is relevant -- it describes a bug that is long-resolved, and not the behavior of throwing an exception when called on a symlink.
| m.side_effect = original_rmtree # Avoid teardown using `err_on_rm_venv_only` | ||
|
|
||
|
|
||
| def test_remove_keeps_dir_if_not_deleteable_os_err_20( |
There was a problem hiding this comment.
Let's parameterize this whole test for different errno constants instead of doing this copy-and-paste.
|
Stale. Closing as this has not had any activity for a while, and the error the change attempts to fix is no longer reproducible. |
|
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
Pull Request Check List
Resolves: Sometimes the
.venvis a symlink, such as in caching strategies for CI/CD environments (codebuild, etc)Related: #2064
We are already asserting the file path is a directory, so if OS Error 20 is triggered, which is "Not a directory" we simply delete everything but the top level folder.