-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fix: poetry env
broken on Windows (#4615)
#4682
Conversation
@sdispater - this is a trivial fix and the problem is quite severe on Windows. |
@tom-bowles can you rebase this so the cirrus tests can have a rerun please? |
@abn - Thanks for taking a look! I've rebased now. |
@tom-bowles should have mentioned this earlier, any cance we can add in a test case for this so we do not regress? |
@abn - no worries, I'll have a crack at that today |
@abn - Added test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM -- we just need the formatting fixed (run pre-commit run --all-files
) by black to merge.
|
||
venv = VirtualEnv(venv_path) | ||
|
||
assert Path(venv.python) == base_dir_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this test fails on Python 3.6 on Windows, so I spoke a little too soon. Can we take a look at that failure and get it corrected?
(Also includes correction for non-mingw Windows from python-poetry#4682)
Looks like this was fixed incidentally at #5007. Apparently that one was allowed through without testcases so I suppose it could just about be worth keeping this open if anyone is motivated, more than a year later, to add a unit test. But I expect this should now be closed. |
Resolves: #4615
Looks like this if statement was copied from further up in the function and only one of the branches updated to use self._path.