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

ncoghlan · 2018-04-08T12:56:25Z

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.

ncoghlan · 2018-04-08T13:00:35Z

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

serhiy-storchaka · 2018-04-10T05:35:23Z

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


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

serhiy-storchaka · 2018-04-10T05:40:53Z

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


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.

brettcannon · 2018-04-13T22:17:57Z

    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


Missing a period.

brettcannon · 2018-04-13T22:18:34Z

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


Missing a period.

brettcannon · 2018-04-13T22:19:05Z

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


Missing a period.

brettcannon · 2018-04-13T22:19:17Z

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


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.

serhiy-storchaka · 2018-04-15T11:33:17Z

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


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

The wording here looks slightly confusing.

serhiy-storchaka · 2018-04-15T11:37:16Z

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


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

serhiy-storchaka · 2018-04-15T11:41:04Z

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


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.

serhiy-storchaka · 2018-04-15T11:46:12Z

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


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.

serhiy-storchaka · 2018-04-15T11:46:31Z

@@ -0,0 +1,5 @@
+Fixed regression when running pydoc with the ``-m`` switch. (The regression


:option:`-m`?

miss-islington · 2018-04-15T11:52:59Z

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

bedevere-bot · 2018-04-15T11:53:27Z

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 <[email protected]>

miss-islington · 2018-04-15T11:54:09Z

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

miss-islington · 2018-04-15T11:54:12Z

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

miss-islington · 2018-04-15T11:54:36Z

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 <[email protected]>

ncoghlan · 2018-04-15T12:35:37Z

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 <[email protected]>

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 <[email protected]>

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

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

Uh oh!

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ncoghlan commented Apr 8, 2018 •

edited by bedevere-bot

Loading