bpo-33185: Fix regression in pydoc CLI sys.path handling by ncoghlan · Pull Request #6419 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation25 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
The pydoc CLI assumed -m pydoc would add the empty string
to sys.path, and hence got confused when it switched to
adding the full initial working directory instead.
This refactors the pydoc CLI path manipulation to be
more testable, and ensures it won't accidentally
remove the standard library directory containing
pydoc itself from sys.path.
https://bugs.python.org/issue33185
The pydoc CLI assumed -m pydoc would add the empty string to sys.path, and hence got confused when it switched to adding the full initial working directory instead.
This refactors the pydoc CLI path manipulation to be more testable, and ensures it won't accidentally remove the standard library directory containing pydoc itself from sys.path.
@brettcannon The context here is that converting the -m switch over to adding the full starting directory path to sys.path confused pydoc's sys.path manipulation to the extent that it could end up removing the standard lib directory from sys.path.
Rather than just fixing the regression, I figured it made sense to also make that manipulation code more testable, and less likely to ever remove the standard library from sys.path.
| rather than the current working directory. Any programs that are checking for |
|---|
| the empty string in :data:`sys.path`, or otherwise relying on the previous |
| behaviour, will need to be updated accordingly (e.g. by checking for |
| ``os.getcwd()`` in addition to checking for the empty string). |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.getcwd() or the starting directory?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on why you're looking for the empty string (in the case of pydoc for example, it knows it hasn't changed the working directory, so the starting directory and the current directory are the same thing).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "the current working directory" do you mean the empty string, not os.getcwd()?
The wording here looks slightly confusing.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I tried to make it clearer in the last set of updates, but it's hard to concisely explain the difference between "current working directory at application startup" and "current working directory at time of import resolution".
| # Accordingly, if the current directory is already present, don't make |
| # any changes to the given_path |
| if '' in given_path or os.curdir in given_path or os.getcwd() in given_path: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what circumstances sys.path contains '' or '.'?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'' will be present for -c and the interactive prompt, '.' will only be present if you added it explicitly (originally it was added by pydoc.cli() itself, but I changed that as part of this PR)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if just remove the whole sys.path patching code? Both ./python -m pydoc and ./python Lib/pydoc.py looks working after this.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem the path manipulation solves is that pydoc itself needs to be able to find modules in the current directory (where the user's own code is likely to be), even when run via a wrapper script installed as /usr/bin/pydoc (or the Windows equivalent).
I'm not sure there's a regression test for that, though.
| @@ -2614,18 +2614,50 @@ def browse(port=0, *, open_browser=True, hostname='localhost'): |
|---|
| def ispath(x): |
| return isinstance(x, str) and x.find(os.sep) >= 0 |
| def _get_revised_path(given_path, argv0): |
| """Ensures current directory is on returned path, and argv0 directory is not |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a period.
| # Note: the tests only cover _get_revised_path, not _adjust_cli_path itself |
| def _adjust_cli_sys_path(): |
| """Ensures current directory is on sys.path, and __main__ directory is not |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a period.
| return pydoc._get_revised_path(given_path, argv0) |
|---|
| def _get_starting_path(self): |
| # Get a copy of sys.path without the current directory |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a period.
| # Get a copy of sys.path without the current directory |
|---|
| clean_path = sys.path.copy() |
| for spelling in self.curdir_spellings: |
| for __ in range(clean_path.count(spelling)): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single _.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is hard to distinguish __ from _ or ___. And since names starting with underscores have special meaning, it looks as a false signal that there is is something special here. I would use i here.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually went to PEP 8 to see if it stated a preference for throwaway variable names, but it's currently silent on the topic.
| rather than the current working directory. Any programs that are checking for |
|---|
| the empty string in :data:`sys.path`, or otherwise relying on the previous |
| behaviour, will need to be updated accordingly (e.g. by checking for |
| ``os.getcwd()`` in addition to checking for the empty string). |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "the current working directory" do you mean the empty string, not os.getcwd()?
The wording here looks slightly confusing.
| # Accordingly, if the current directory is already present, don't make |
| # any changes to the given_path |
| if '' in given_path or os.curdir in given_path or os.getcwd() in given_path: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if just remove the whole sys.path patching code? Both ./python -m pydoc and ./python Lib/pydoc.py looks working after this.
| # Get a copy of sys.path without the current directory |
|---|
| clean_path = sys.path.copy() |
| for spelling in self.curdir_spellings: |
| for __ in range(clean_path.count(spelling)): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is hard to distinguish __ from _ or ___. And since names starting with underscores have special meaning, it looks as a false signal that there is is something special here. I would use i here.
| @@ -0,0 +1,5 @@ |
|---|
| Fixed regression when running pydoc with the ``-m`` switch. (The regression |
| was introduced in 3.7.0b3 by the resolution of bpo-33053) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed period.
Shouldn't the reference to other issue be written as :issue:`33053`?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bleh, this is the main downside of the switch to separate news snippets - no other surrounding entries to prompt styling fixes.
| @@ -0,0 +1,5 @@ |
|---|
| Fixed regression when running pydoc with the ``-m`` switch. (The regression |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:option:`-m`?
Thanks @ncoghlan for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
The pydoc CLI assumed -m pydoc would add the empty string to sys.path, and hence got confused when it switched to adding the full initial working directory instead.
This refactors the pydoc CLI path manipulation to be more testable, and ensures it won't accidentally remove the standard library directory containing pydoc itself from sys.path. (cherry picked from commit 82a9481)
Co-authored-by: Nick Coghlan ncoghlan@gmail.com
Thanks @ncoghlan for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖
Sorry, @ncoghlan, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.cherry_picker 82a948105920100ca2ec5c2340bc3890adcfe778 2.7
Thanks @ncoghlan for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖
miss-islington added a commit that referenced this pull request
The pydoc CLI assumed -m pydoc would add the empty string to sys.path, and hence got confused when it switched to adding the full initial working directory instead.
This refactors the pydoc CLI path manipulation to be more testable, and ensures it won't accidentally remove the standard library directory containing pydoc itself from sys.path. (cherry picked from commit 82a9481)
Co-authored-by: Nick Coghlan ncoghlan@gmail.com
sigh And now I notice that git was sitting prompting for my ssh passphrase, and so my last round of edits weren't in the version I merged.
Ah well, I needed to do another push to fix the markup in the NEWS entry anyway.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
Adds some working and markup fixes that I missed in the initial commit for this issue.
(Follow-up to pythonGH-6419) (cherry picked from commit 1a5c4bd)
Co-authored-by: Nick Coghlan ncoghlan@gmail.com
ncoghlan added a commit that referenced this pull request
Adds some working and markup fixes that I missed in the initial commit for this issue.
(Follow-up to GH-6419)
miss-islington added a commit that referenced this pull request
Adds some working and markup fixes that I missed in the initial commit for this issue.
(Follow-up to GH-6419) (cherry picked from commit 1a5c4bd)
Co-authored-by: Nick Coghlan ncoghlan@gmail.com