bpo-33185: Fix regression in pydoc CLI sys.path handling by ncoghlan · Pull Request #6419 · python/cpython · GitHub

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.

os.getcwd() or the starting directory?

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).

By "the current working directory" do you mean the empty string, not os.getcwd()?

The wording here looks slightly confusing.

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".

In what circumstances sys.path contains '' or '.'?

'' 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)

What if just remove the whole sys.path patching code? Both ./python -m pydoc and ./python Lib/pydoc.py looks working after this.

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.

Missing a period.

Single _.

Using double-underscore as my throwaway variable name is a habit for me these days: https://stackoverflow.com/a/5893946/597742

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.

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.

By "the current working directory" do you mean the empty string, not os.getcwd()?

The wording here looks slightly confusing.

What if just remove the whole sys.path patching code? Both ./python -m pydoc and ./python Lib/pydoc.py looks working after this.

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.

Missed period.

Shouldn't the reference to other issue be written as :issue:`33053`?

Bleh, this is the main downside of the switch to separate news snippets - no other surrounding entries to prompt styling fixes.

:option:`-m`?

Thanks @ncoghlan for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

GH-6476 is a backport of this pull request to the 3.7 branch.

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.
🐍🍒⛏🤖

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.

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>

Adds some working and markup fixes that I missed in the initial commit for this issue. (Follow-up to GH-6419)

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>

ncoghlan added the needs backport to 3.7 label Apr 8, 2018

ncoghlan requested a review from brettcannon April 8, 2018 12:56

the-knights-who-say-ni added the CLA signed label Apr 8, 2018

bedevere-bot added the awaiting merge label Apr 8, 2018

serhiy-storchaka reviewed Apr 10, 2018

View reviewed changes

brettcannon approved these changes Apr 13, 2018

View reviewed changes

serhiy-storchaka reviewed Apr 15, 2018

View reviewed changes

ncoghlan merged commit 82a9481 into python:master Apr 15, 2018

bedevere-bot removed the awaiting merge label Apr 15, 2018

bedevere-bot removed the needs backport to 3.7 label Apr 15, 2018

ncoghlan added the needs backport to 2.7 label Apr 15, 2018

ncoghlan added needs backport to 3.7 and removed needs backport to 2.7 labels Apr 15, 2018

ncoghlan mentioned this pull request Apr 15, 2018

bpo-33185: Improve wording and markup #6477

Merged

miss-islington mentioned this pull request Apr 15, 2018

[3.7] bpo-33185: Improve wording and markup (GH-6477) #6478

Merged

ncoghlan added a commit that referenced this pull request Apr 15, 2018

bpo-33185 : Improve wording and markup (GH-6477 )

1a5c4bd

Adds some working and markup fixes that I missed in the initial commit for this issue. (Follow-up to GH-6419)

ned-deily removed the needs backport to 3.7 label Apr 22, 2018

+                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).

+                      # 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)):

+                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).

+                      # 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)):

+                  # 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:

                   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

+              # 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

+                      return pydoc._get_revised_path(given_path, argv0)
+                  def _get_starting_path(self):
+                      # Get a copy of sys.path without the current directory

		@@ -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)

bpo-33185: Fix regression in pydoc CLI sys.path handling by ncoghlan · Pull Request #6419 · python/cpython · GitHub

Repository navigation

bpo-33185: Fix regression in pydoc CLI sys.path handling - #6419

bpo-33185: Fix regression in pydoc CLI sys.path handling#6419ncoghlan merged 1 commit intopython:masterpython/cpython:masterfrom ncoghlan:bpo-33185-fix-pydoc-path-manipulationncoghlan/cpython:bpo-33185-fix-pydoc-path-manipulationCopy head branch name to clipboard

Conversation

ncoghlan commented Apr 8, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncoghlan commented Apr 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Apr 15, 2018

Uh oh!

bedevere-bot commented Apr 15, 2018

Uh oh!

miss-islington commented Apr 15, 2018

Uh oh!

miss-islington commented Apr 15, 2018

Uh oh!

miss-islington commented Apr 15, 2018

Uh oh!

ncoghlan commented Apr 15, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bpo-33185: Fix regression in pydoc CLI sys.path handling#6419
ncoghlan merged 1 commit into
python:masterfrom
ncoghlan:bpo-33185-fix-pydoc-path-manipulation

ncoghlan commented Apr 8, 2018 •
edited by bedevere-bot
Loading