Skip to content

Warning for user when Windows default Path Limit is exceeded#10046

Merged
pradyunsg merged 14 commits into
pypa:mainfrom
OBITORASU:long-paths-fix
Jun 11, 2021
Merged

Warning for user when Windows default Path Limit is exceeded#10046
pradyunsg merged 14 commits into
pypa:mainfrom
OBITORASU:long-paths-fix

Conversation

@OBITORASU
Copy link
Copy Markdown
Contributor

@OBITORASU OBITORASU commented Jun 8, 2021

Description

This PR generates a warning message for the end-user informing them about the potential errors which might be caused if they have Long Paths disabled on their Windows system. I have used:

from pip._internal.utils.compat import WINDOWS

as the means to check the host OS as suggested by the maintainers and added an if statement to check for the 260 default path length limit when Long Paths is disabled. The if statement will successfully trigger a warning using logger.warning in case the host OS is Windows and the error message length (which also contains the path) is greater than 260 characters.

Fixes Issue #10045

@OBITORASU OBITORASU changed the title Warning for user when Windows path limit is exceeded Warning for user when Windows default Path Limit is exceeded Jun 8, 2021
Copy link
Copy Markdown
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of thoughts:

  1. error is an exception, so you need to str() it first.
  2. We should probably check error.errno to make sure we’re catching a FileNotFoundError.
  3. The error message should include a link to some Microsoft documentation. I don’t except people having Long Paths off would understand what set LongPathsEnabled to 1 means.

@OBITORASU
Copy link
Copy Markdown
Contributor Author

OBITORASU commented Jun 8, 2021

Couple of thoughts:

1. `error` is an exception, so you need to `str()` it first.

2. We should probably check `error.errno` to make sure we’re catching a `FileNotFoundError`.

3. The error message should include a link to some Microsoft documentation. I don’t except people having Long Paths off would understand what _set LongPathsEnabled to 1_ means.

For the second suggestion. Can I go forward with a check like error.errno == errno.ENOENT? Somewhat like this check if error.errno == errno.EACCES: currently being used in the code.

@uranusjr
Copy link
Copy Markdown
Member

uranusjr commented Jun 8, 2021

Yeah, sounds right to me.

@OBITORASU
Copy link
Copy Markdown
Contributor Author

OBITORASU commented Jun 8, 2021

Yeah, sounds right to me.

And last question I had for the final suggestion, I am going forward with this microsoft documentation in particular: https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=cmd#enable-long-paths-in-windows-10-version-1607-and-later
Hope this is fine.

@uranusjr
Copy link
Copy Markdown
Member

uranusjr commented Jun 8, 2021

Damn, that’s a long one. I guess that’s the best we can have though, since it’s Microsoft.

@OBITORASU
Copy link
Copy Markdown
Contributor Author

OBITORASU commented Jun 8, 2021

Damn, that’s a long one. I guess that’s the best we can have though, since it’s Microsoft.

Yeah that is the only decent one I found which belonged exclusively to Microsoft. As of now I have updated the PR with all the requested changes, please take a look when it is possible for you. Thanks!

Comment thread src/pip/_internal/commands/install.py Outdated
Comment thread src/pip/_internal/commands/install.py Outdated
Comment thread src/pip/_internal/commands/install.py Outdated
Comment thread src/pip/_internal/commands/install.py Outdated
Comment thread src/pip/_internal/commands/install.py Outdated
Comment thread src/pip/_internal/commands/install.py Outdated
@pradyunsg pradyunsg added the type: enhancement Improvements to functionality label Jun 11, 2021
Copy link
Copy Markdown
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Happy to have this merged once CI is happy. :)

@pradyunsg
Copy link
Copy Markdown
Member

Thanks for your contribution @OBITORASU! ^>^

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

type: enhancement Improvements to functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants