gh-92675: venv: Fix ensure_directories() to again accept a Path for env_dir #92676
Conversation
Lib/venv/__init__.py
Outdated
| @@ -116,7 +116,7 @@ def create_if_needed(d): | |||
| elif os.path.islink(d) or os.path.isfile(d): | |||
| raise ValueError('Unable to create directory %r' % d) | |||
|
|
|||
| if os.pathsep in env_dir: | |||
| if isinstance(env_dir, str) and os.pathsep in env_dir: | |||
There was a problem hiding this comment.
It looks like the intent here is to enforce that env_dir is only a single path segment. Should we preserve that behavior for path-likes too? For example, we could call env_dir = os.fspath(env_dir) first.
I'm not sure why we need to check for this at all though. What is the reason we have this check?
There was a problem hiding this comment.
It's checking for os.pathsep, not os.sep: it was a fix for bpo-43218. I agree, we should retain the behaviour for path-likes too - otherwise the problem originally solved for will come back.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @vsajip: please review the changes made to this pull request. |
|
Thanks for the effort fixing this issue! Any update on its status? |
Thanks to JelleZijlstra for the feedback! Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
|
@JelleZijlstra thanks for the feedback! Did apply your suggestion. |
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
|
@davidfstr you can fix the other CI failure by adding a NEWS entry using https://blurb-it.herokuapp.com. I think it probably does need a NEWS entry, since this is a bugfix :) |
I'll plan to look at this tomorrow. I was previously thinking that no NEWS entry would be required since I'm fixing a bug that only affects a pre-release version of Python (i.e. Python 3.11 beta 1). |
The bug is nonetheless present in a released version of Python, even if it's not a production-ready release :) The changelog needs to reflect changes that took place in between Beta 1 and Beta 2, and this was a user-visible bug in Beta 1. |
AlexWaygood
added
type-bug
|
Thanks! Once the news entry is added, I think this should be good to go. Hopefully @vsajip will be able to give another review; if not I can merge the fix, since his previous concerns have been addressed and it seems important that the bug fix makes it into beta 2. |
|
(Beta 2 is scheduled for May 30, less than 2 weeks away: PEP-664.) |
|
Yes, it's fine by me to merge once the NEWS item is added. |
|
NEWS item has now been added. |
Misc/NEWS.d/next/Library/2022-05-19-13-33-18.gh-issue-92675.ZeerMZ.rst
Outdated
Show resolved
Hide resolved
…erMZ.rst Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
|
Thanks @davidfstr for the PR, and @JelleZijlstra for merging it |
|
GH-92961 is a backport of this pull request to the 3.11 branch. |
… for env_dir (pythonGH-92676) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> (cherry picked from commit 30deeac64925effe46cb5f1cd091ccb4c850ce83) Co-authored-by: David Foster <david@dafoster.net>
Fixes: #92675