-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix: Windows long filename error #13454
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
base: main
Are you sure you want to change the base?
Conversation
Hi @sepehr-rs, thanks for your PR, please be aware it may take some time for pip maintainers to review or respond to this PR as all maintainers are currently volunteers. Though please let me know if you need any assistance fixing the pre-commit errors. |
Hi @notatallshaw, thanks for the update! Understand the delay — I really appreciate the work the maintainers do. |
a04c3c2
to
2cfe45a
Compare
2cfe45a
to
51dfb4e
Compare
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.
Hello,
This looks generally right, but I have suggestions to tweak the changelog and comments. I also ask that you add a unit test to verify that this works properly. This function is tested here:
pip/tests/unit/test_command_install.py
Line 125 in e22047b
def test_create_os_error_message( |
You simply need to add another case for this specific error.
Thanks for your interest in improving pip! I look forward to merging your contribution once it's ready.
# Windows raises EINVAL when a file or folder name exceeds 255 characters, | ||
# even if long path support is enabled. Add a hint for that case. |
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.
# Windows raises EINVAL when a file or folder name exceeds 255 characters, | |
# even if long path support is enabled. Add a hint for that case. | |
# Windows raises EINVAL when a file*name* or path segment exceeds 255 | |
# characters, even if long path support is enabled. |
It wasn't immediately obvious to me what the difference between this check and the pre-existing hint was. That's my bad, but we can make this clearer :)
@@ -0,0 +1 @@ | |||
This change will improve error hint for long filenames on Windows (EINVAL) |
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.
Provide a hint if a system error is raised involving long filenames or path segments on Windows.
The changelog entry should be worded for the user. No need for "this change will [...]" as that implied.
WINDOWS | ||
and error.errno == errno.EINVAL | ||
and error.filename | ||
and any(len(part) > 255 for part in Path(error.filename).parts) |
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.
Nice use of pathlib!
Fixes #13346
This is my first attempt at addressing this issue. I’d appreciate any feedback or suggestions on how to improve the approach or make it more concise.