-
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.
Co-authored-by: Richard Si <[email protected]>
357ee36
to
95fe31e
Compare
Hi @ichard26, sorry for the delay! Thank you so much for your thoughtful suggestions and the time you took to review them, really appreciate it. I've addressed all the requested changes and also added a platform check to skip the relevant test cases when not running on Windows. Since the issue is Windows-specific, I opted for this approach to keep things scoped and avoid duplicating test cases. |
I'll take another look when I boot up my Windows machine, but in the meanwhile, I'm going to ask @pfmoore for a quick review for his Windows expertise as I haven't used Windows as my day to day OS in many years. (Paul, I'm mostly asking that you confirm the long path mode applicability—or lack of thereof for certain errors—on Windows. I can double check our logic to detect the various errors is right myself.) Thanks for the updates! |
Thanks a lot! That sounds great. Let me know if there's anything I can do to assist. |
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.
This looks OK to me. I confirmed that EINVAL is reported if you try to create a file with a part more than 255 characters long, so the check seems good to me. I can't say much about long path mode - I'm pretty sure I have long path mode enabled (I'm sure I did that at some point a long time ago) so I guess the 255 limit applies even then, as stated. But I can't do much more without disabling long path mode, and I'm reluctant to do that as I don't know the implications for existing files.
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.
The changes look great now, thanks!
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.
Wait, hang on.
But I can't do much more without disabling long path mode, and I'm reluctant to do that as I don't know the implications for existing files.
I turned off Long Paths (and ... now I realize I forgot to turn them back on, whoops) and it seems that Windows raises the ENOENT
error in both situations.
>>> Path(f"P:/test/{'a'*200}").touch()
>>> Path(f"P:/test/{'a'*300}").touch()
Traceback (most recent call last):
File "<python-input-9>", line 1, in <module>
Path(f"P:/test/{'a'*300}").touch()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "C:\Users\richa\AppData\Local\Programs\Python\Python313\Lib\pathlib\_local.py", line 714, in touch
fd = os.open(self, flags, mode)
FileNotFoundError: [Errno 2] No such file or directory: 'P:\\test\\aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
>>> import sys, errno
>>> errno.errorcode[sys.last_exc.errno]
'ENOENT'
So I guess that we should be checking for ENOENT
or EINVAL
. As long as any segment of the path exceeds the hard 255 character limit, it doesn't matter which exact error code was raised, we should warn the user that they may be encountering the OS limit. I would ping Steve Dower, but they're on vacation right now.
Thanks for the thorough testing and clarification, @pfmoore and @ichard26! @ichard26 — I’ll go ahead and apply the changes you suggested. I’ll update the code to check for both Please let me know if there’s anything else you’d like to adjust. |
Sorry, I wasn't clear enough. I think it's still worth keeping the error messages for both scenarios separate. If any segment of the path exceeds the 255 character limit, then it's simply not going to work and enabling Long Paths won't help. This should be reflected in the error message. The error message from the previous version of this PR did. However, if the path overall exceeds 260 characters, then it's possible that Long Paths aren't enabled but should be. That possibility should be included in the error message. A good error message offers a hint for what to do, but it's also important to not lead the user down a rabbit hole of trying a solution we know isn't going to help. In other words, when I made that suggestion, I simply wanted you to tweak the Does that make sense? Sorry again, I don't mean to make you do a bunch of work. I'm just trying to guide/coach you through the reasoning process since you did ask for feedback. |
Hi @ichard26, that makes total sense, thank you so much for taking the time to explain the distinction so clearly, and no worries at all! I really appreciate the guidance, it helps me learn a lot through this process. I’ll go ahead and make that change to handle both |
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.