bpo-28334: fix netrc not working when $HOME is not set #123

dmerejkowsky · 2017-02-15T20:32:47Z

https://bugs.python.org/issue28334

the-knights-who-say-ni · 2017-02-15T20:32:48Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

If you don't have an account on b.p.o, please create one
Make sure your GitHub username is listed in "Your Details" at b.p.o
If you have not already done so, please sign the PSF contributor agreement
If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

dmerejkowsky · 2017-02-15T20:36:11Z

As discussed during patch review, no tests were added, but doc was updated.

dmerejkowsky · 2017-02-15T20:40:15Z

@the-knights-who-say-ni :
Added my github username on bugs.python.org as requested.

berkerpeksag

Thanks for the PR, but like I said in review we can't accept this without a test case.

It should be easy to test at least the FileNotFoundError case by changing $HOME env variable.

berkerpeksag · 2017-02-19T14:48:37Z

Doc/library/netrc.rst

@@ -32,6 +32,10 @@ the Unix :program:`ftp` program and other FTP clients.

   .. versionchanged:: 3.4 Added the POSIX permission check.

+   .. versionchanged:: 3.7 Now uses :func:`os.path.expanduser`, so if


Style nit:

.. versionchanged:: 3.7 Now uses :func:`os.path.expanduser`, so if [...]

dmerejkowsky · 2017-02-20T19:15:54Z

Rebased, added a test, and fix doc style as requested.

zhangyangyu · 2017-03-08T09:26:42Z

Lib/test/test_netrc.py

@@ -123,6 +123,15 @@ def test_security(self):
            os.chmod(fn, 0o622)
            self.assertRaises(netrc.NetrcParseError, netrc.netrc)

+    def test_when_not_found(self):


I would like the name just test_file_not_found.

zhangyangyu · 2017-03-08T09:28:57Z

Lib/test/test_netrc.py

+        with support.EnvironmentVarGuard() as environ:
+            environ.set('HOME', d)
+            self.assertRaises(FileNotFoundError, netrc.netrc)
+


When you explicitly pass a not exist file, it could also raise the error.

OK. I wrote an other test for this. (I like to follow the 'one assert per test' rule)

zhangyangyu · 2017-03-13T03:32:40Z

LGTM. But I'd wait for others' reviews for some time. And I want to treat this as an enhancement though it looks like a bug on Windows.

berkerpeksag

Thanks! Looks good to me, just left some minor comments. Also, since this a behavior change we need to add a note to Misc/NEWS.

berkerpeksag · 2017-03-15T14:23:16Z

Doc/library/netrc.rst

@@ -32,6 +32,11 @@ the Unix :program:`ftp` program and other FTP clients.

   .. versionchanged:: 3.4 Added the POSIX permission check.

+   .. versionchanged:: 3.7
+      Now uses :func:`os.path.expanduser`, so if :envvar:`HOME` is not


I think the "if :envvar:HOME is not set [...]" part should be moved to the main documentation block and the versionchanged annotation should only mention that it now uses expanduser() to determine the location of the .netrc file if *file* is not passed.

Makes sense.

berkerpeksag · 2017-03-15T14:23:16Z

Doc/library/netrc.rst

@@ -32,6 +32,11 @@ the Unix :program:`ftp` program and other FTP clients.

   .. versionchanged:: 3.4 Added the POSIX permission check.

+   .. versionchanged:: 3.7
+      Now uses :func:`os.path.expanduser`, so if :envvar:`HOME` is not
+      set *and* ``~/.netrc`` not found,  will raise


Double spaces before "will".

oups, sorry :)

berkerpeksag · 2017-03-15T14:23:16Z

Lib/test/test_netrc.py

+            self.assertRaises(FileNotFoundError, netrc.netrc)
+
+    def test_file_not_found_explicit(self):
+        unlikely_file = "unlikely_netrc"


No need to use unlikely_file here. Just pass "unlikely_netrc" to the constructor.

dmerejkowsky · 2017-03-24T09:56:09Z

@berkerpeksag rebased and conformed with your latest comments

zhangyangyu · 2017-04-10T10:01:31Z

Doc/library/netrc.rst

-   no argument is given, the file :file:`.netrc` in the user's home directory will
-   be read.  Parse errors will raise :exc:`NetrcParseError` with diagnostic
+   no argument is given, the file :file:`.netrc` in the user's home directory --
+   as returned by :func:`os.path.expanduser` -- will be read. If the file is not found, will raise


Two spaces before "If the file ...".

zhangyangyu · 2017-04-10T10:08:07Z

Misc/NEWS

@@ -1046,6 +1046,8 @@ Core and Builtins
 Library
 -------

+- bpo-28334: fix netrc not working when $HOME is not set


Uppercase please and colon at end. And instead of fixing, I'd rather something like "Use os.path.expanduser rather than barely checking $HOME is set or not when no argument is given to netrc.netrc().". Ignore my wording, I am not a native speaker. :-(

Also done. It's a bit long, but I hope it documents precisely the new behavior

dmerejkowsky · 2017-04-10T11:30:41Z

@zhangyangyu Thanks for the suggestions! It's worth making sure the documentation is as best as possible :)
Rebased and changed as requested.

zhangyangyu · 2017-04-10T15:25:34Z

Misc/NEWS

@@ -1125,6 +1125,11 @@ Core and Builtins
 Library
 -------

+- bpo-28334: Use os.path.expanduser() to find the ~/.netrc file in netrc.netrc().


Too wordy. Just

bpo-28334: Use os.path.expanduser() to find the ~/.netrc file in netrc.netrc().
If the file does not exist, a FileNotFoundError is raised.

serhiy-storchaka · 2017-04-10T16:46:22Z

Doc/library/netrc.rst

-   be read.  Parse errors will raise :exc:`NetrcParseError` with diagnostic
+   no argument is given, the file :file:`.netrc` in the user's home directory --
+   as returned by :func:`os.path.expanduser` -- will be read.  If the file is
+   not found, will raise :exc:`FileNotFoundError`.


There are many causes why the file can't be read. It can be a directory or broken symlink, or user can not have permissions to read it. Different OSError subclasses are raised in these cases. I think no need to specify this is explicitly. Exceptions that can be raised by the function are rarely documented in Python docs.

Fair enough. I removed the sentence.

serhiy-storchaka · 2017-04-10T16:46:22Z

Lib/test/test_netrc.py

+        os.mkdir(d)
+        self.addCleanup(support.rmtree, d)
+        with support.EnvironmentVarGuard() as environ:
+            environ.set('HOME', d)


This doesn't test a new behavior. Needed a test for the case when HOME is not set.

This doesn't test a new behavior

I think it does. It checks the type of the exception raised, which is the whole point of this patch,
getting rid of the weird 'OSError' about HOME not set ...

Needed a test for the case when HOME is not set.

Yeah I was not sure about this one. I've added a test where I monkeypatch os.path.expanduser, but I'm not sure it's useful. My guts tell me such a test is too close to the implementation.

Serhiy is right that we need a test for this case and unless I'm missing something you can do it without doing any mocking:

with support.EnvironmentVarGuard() as environ: environ.unset('HOME') self.assertRaises(FileNotFoundError, netrc.netrc)

dmerejkowsky · 2017-04-11T19:47:35Z

Wrote a test monkeypatching os.path.expanduser. Maybe we can keep it after all.
You tell me ;)

dmerejkowsky · 2017-04-26T13:26:05Z

@serhiy-storchaka @berkerpeksag @terryjreedy gentle reminder :)

louisom · 2017-04-26T15:02:09Z

@dmerejkowsky For reminding, you don't need to squash your commit every change makes, all commit will be squash when the PR is merged. See devguide last paragraph.

berkerpeksag · 2017-04-26T15:07:44Z

Lib/test/test_netrc.py

+        os.chmod(fake_netrc_path, 0o600)
+        with support.EnvironmentVarGuard() as environ:
+            environ.unset('HOME')
+            with mock.patch('os.path.expanduser') as mock_expanduser:


Mocking os.path.expanduser is not needed here in my opinion. If you can change the test to

def test_home_not_set(self): with support.EnvironmentVarGuard() as environ: environ.unset('HOME') self.assertRaises(FileNotFoundError, netrc.netrc)

then we are good to go! :)

I'm afraid not. If we unset HOME and ~/.netrc does exist on the developer box, os.path.expanduser will read /etc/passwd using pwd (see [1]) and no exception will be raised. I'd prefer the tests to not depend on what's on the home directory of the user running the tests.

[1]: https://github.com/python/cpython/blob/master/Lib/posixpath.py#L245-L247

berkerpeksag · 2017-04-26T15:08:57Z

Misc/NEWS

@@ -1125,6 +1125,9 @@ Core and Builtins
 Library
 -------

+- bpo-28334: Use os.path.expanduser() to find the ~/.netrc file in netrc.netrc().


~/.netrc -> ``~/.netrc``

(otherwise our doc build will complain)

berkerpeksag · 2017-04-26T15:12:44Z

@dmerejkowsky and just one more comment: Please add your name to Misc/ACKS.

serhiy-storchaka · 2017-04-27T16:00:47Z

Doc/library/netrc.rst

-   no argument is given, the file :file:`.netrc` in the user's home directory will
-   be read.  Parse errors will raise :exc:`NetrcParseError` with diagnostic
+   no argument is given, the file :file:`.netrc` in the user's home directory --
+   as returned by :func:`os.path.expanduser` -- will be read.


Maybe "as determined"? expanduser() itself doesn't return the user's home directory.

Or "as returned by os.path.expanduser('~') ", but this exposes more implementation details and is harder to format as a reference.

I like 'determined', thanks :)

serhiy-storchaka · 2017-04-27T16:00:47Z

Lib/netrc.py

-                file = os.path.join(os.environ['HOME'], ".netrc")
-            except KeyError:
-                raise OSError("Could not find .netrc: $HOME is not set") from None
+            file = os.path.expanduser("~/.netrc")


'/' is Posix specific pathname separator. Windows accepts it too, but it is better to use the native pathname separator, os.sep. Or use os.path.join():

os.path.expanduser(os.path.join("~", ".netrc"))

or

os.path.join(os.path.expanduser("~"), ".netrc")

serhiy-storchaka · 2017-04-27T16:00:47Z

Doc/library/netrc.rst

@@ -32,6 +33,10 @@ the Unix :program:`ftp` program and other FTP clients.

   .. versionchanged:: 3.4 Added the POSIX permission check.

+   .. versionchanged:: 3.7
+      Now uses :func:`os.path.expanduser` to determine the location
+      of ``~/.netrc`` if *file* is not passed as argument.


~/.netrc is Posix-specific syntax. Windows users of netrc can even not know what ~/ means. Maybe "of the file :file:'.netrc'" of "of the default netrc file"?

serhiy-storchaka · 2017-04-27T16:00:47Z

Lib/test/test_netrc.py

+            environ.unset('HOME')
+            with mock.patch('os.path.expanduser') as mock_expanduser:
+                mock_expanduser.return_value = fake_netrc_path
+                nrc = netrc.netrc()


Check the content of nrc for testing that the configuration is read from the right file. Add a random data in the fake .netrc.

serhiy-storchaka · 2017-04-27T16:00:47Z

Misc/NEWS

@@ -1125,6 +1125,9 @@ Core and Builtins
 Library
 -------

+- bpo-28334: Use os.path.expanduser() to find the ``~/.netrc`` file in netrc.netrc().


Too long line.

Add "Patch by Dimitri Merejkowsky."

serhiy-storchaka · 2017-04-27T16:00:47Z

Misc/NEWS

@@ -1125,6 +1125,9 @@ Core and Builtins
 Library
 -------

+- bpo-28334: Use os.path.expanduser() to find the ``~/.netrc`` file in netrc.netrc().
+  If the file does not exist, a FileNotFoundError is raised.


Is this a new behavior?

Nope, fixed

serhiy-storchaka · 2017-04-27T16:00:47Z

Lib/test/test_netrc.py

+        os.chmod(fake_netrc_path, 0o600)
+        with support.EnvironmentVarGuard() as environ:
+            environ.unset('HOME')
+            with mock.patch('os.path.expanduser') as mock_expanduser:


This is too fragile. If rather than os.path.expanduser("~/.netrc") use one of variants suggested by me, the test will be broken.

I would monkey-patch expanduser with a code like the following:

orig_expanduser = os.path.expanduser called = [] def fake_expanduser(s): called.append(s) environ.set('HOME', d) result = orig_expanduser(s) environ.unset('HOME') return result with test.support.swap_attr(os.path, 'expanduser', fake_expanduser): nrc = netrc.netrc() self.assertTrue(called)

You're right, test is less likely to break this way.

That's a Python bug: http://bugs.python.org/issue28334 Most likely it will be fixed in Python 3.7: python/cpython#123

During interpreter shutdown Stackless kills tasklets on daemon threads, if they have a non-trivial C-stack. This caused an assertion violation. https://bitbucket.org/stackless-dev/stackless/issues/123 (grafted from 9fd4f6a9d266250bef5baa692b89516e73b00e62)

berkerpeksag · 2017-11-24T08:26:41Z

I've fixed merge conflicts, created NEWS entry with blurb and made some cosmetic changes. Is there anything left to do here?

serhiy-storchaka · 2017-11-24T08:48:08Z

Lib/test/test_netrc.py

+        self.assertRaises(FileNotFoundError, netrc.netrc,
+                          file='unlikely_netrc')
+
+    def test_home_not_set(self):


Sorry, I don't understand how this test is related to the case when HOME is not set. Why not use the simple test suggested by @berkerpeksag above?

My suggested test won't work, because of the following code in expanduser:

if 'HOME' not in os.environ: import pwd userhome = pwd.getpwuid(os.getuid()).pw_dir

I have a .netrc file in my HOME directory so the test won't pass:

def test_foo(self): with support.EnvironmentVarGuard() as environ: environ.unset('HOME') self.assertRaises(FileNotFoundError, netrc.netrc)

====================================================================== FAIL: test_foo (test.test_netrc.NetrcTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/berker/projects/cpython/master/Lib/test/test_netrc.py", line 34, in test_foo self.assertRaises(FileNotFoundError, netrc.netrc) AssertionError: FileNotFoundError not raised by netrc

Thanks, now I see that this is the simplest platform-independent test.

Also add more tests to check that proper exceptions are raised

berkerpeksag · 2017-11-24T09:12:08Z

For some reason, Travis CI didn't pick up latest changes:

/home/travis/build/python/cpython/Doc/library/pyexpat.rst:872:Footnote [#] is not referenced.

Serhiy fixed it a while ago and I don't get any warnings in my development environment.

berkerpeksag · 2017-11-24T09:27:57Z

Ok, this looks like a Travis bug to me. I've opened PR #4537 and documentation build passed there.

@dmerejkowsky could you please create a new PR? If I merge PR #4537, GitHub overwrite author information and set me as author.

dmerejkowsky · 2017-11-24T18:15:49Z

@dmerejkowsky could you please create a new PR? If I merge PR #4537, GitHub overwrite author information and set me as author.

It's not a problem for me :) It was very nice of you to finish up my pull request, so by all means just merge your own and let GitHub set you as author of the patch, you deserve it.

What matters to me is to be listed in the ACKS file 😛

berkerpeksag · 2017-11-25T10:38:51Z

I've just merged PR #4537. Thank you for your work again, @dmerejkowsky. It took more than a year but we eventually fixed the bug :)

the-knights-who-say-ni added the CLA not signed label Feb 15, 2017

Mariatta removed the CLA not signed label Feb 15, 2017

the-knights-who-say-ni added the CLA signed label Feb 15, 2017

dmerejkowsky mentioned this pull request Feb 18, 2017

bpo-28806: Improve the netrc library #127

Open

berkerpeksag requested changes Feb 19, 2017

View changes

dmerejkowsky force-pushed the dmerejkowsky:netrc-home branch from f9e303b to cb9777b Feb 20, 2017

zhangyangyu self-requested a review Feb 22, 2017

zhangyangyu reviewed Mar 8, 2017

View changes

zhangyangyu requested a review from terryjreedy Mar 8, 2017

dmerejkowsky force-pushed the dmerejkowsky:netrc-home branch from cb9777b to 7119217 Mar 11, 2017

zhangyangyu approved these changes Mar 13, 2017

View changes

berkerpeksag requested changes Mar 15, 2017

View changes

dmerejkowsky force-pushed the dmerejkowsky:netrc-home branch from 7119217 to 62ff357 Mar 15, 2017

dmerejkowsky force-pushed the dmerejkowsky:netrc-home branch from 62ff357 to 86a9333 Mar 24, 2017

zhangyangyu reviewed Apr 10, 2017

View changes

dmerejkowsky force-pushed the dmerejkowsky:netrc-home branch 2 times, most recently from df1f1bc to 09abcaf Apr 10, 2017

zhangyangyu reviewed Apr 10, 2017

View changes

serhiy-storchaka requested changes Apr 10, 2017

View changes

dmerejkowsky force-pushed the dmerejkowsky:netrc-home branch from 09abcaf to 1d544f5 Apr 11, 2017

berkerpeksag reviewed Apr 26, 2017

View changes

serhiy-storchaka requested changes Apr 27, 2017

View changes

akruis pushed a commit to akruis/cpython that referenced this pull request Sep 9, 2017

merge 3.4-slp (Stackless python#123)

5d88da3

akruis pushed a commit to akruis/cpython that referenced this pull request Sep 9, 2017

merge 3.3-slp (Stackless python#123)

2619d22

Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017

berkerpeksag approved these changes Nov 24, 2017

View changes

bedevere-bot added the awaiting merge label Nov 24, 2017

serhiy-storchaka reviewed Nov 24, 2017

View changes

bpo-28334: fix netrc not working when $HOME is not set

Loading status checks…

e8dde32

Also add more tests to check that proper exceptions are raised

berkerpeksag force-pushed the dmerejkowsky:netrc-home branch from 4605b07 to e8dde32 Nov 24, 2017

serhiy-storchaka approved these changes Nov 24, 2017

View changes

berkerpeksag closed this Nov 25, 2017

		@@ -32,6 +32,10 @@ the Unix :program:`ftp` program and other FTP clients.

		.. versionchanged:: 3.4 Added the POSIX permission check.

		.. versionchanged:: 3.7 Now uses :func:`os.path.expanduser`, so if

python / cpython

Sponsor python/cpython

bpo-28334: fix netrc not working when $HOME is not set #123

bpo-28334: fix netrc not working when $HOME is not set #123

Conversation

dmerejkowsky commented Feb 15, 2017 • edited by bedevere-bot

the-knights-who-say-ni commented Feb 15, 2017

dmerejkowsky commented Feb 15, 2017

dmerejkowsky commented Feb 15, 2017

berkerpeksag left a comment

This comment has been minimized.

berkerpeksag Feb 19, 2017 Member

dmerejkowsky commented Feb 20, 2017

This comment has been minimized.

zhangyangyu Mar 8, 2017 Member

This comment has been minimized.

dmerejkowsky Mar 11, 2017 Author

This comment has been minimized.

zhangyangyu Mar 8, 2017 Member

This comment has been minimized.

dmerejkowsky Mar 11, 2017 Author

zhangyangyu commented Mar 13, 2017

berkerpeksag left a comment

This comment has been minimized.

berkerpeksag Mar 15, 2017 Member

This comment has been minimized.

dmerejkowsky Mar 15, 2017 Author

This comment has been minimized.

berkerpeksag Mar 15, 2017 Member

This comment has been minimized.

dmerejkowsky Mar 15, 2017 Author

This comment has been minimized.

berkerpeksag Mar 15, 2017 Member

This comment has been minimized.

dmerejkowsky Mar 15, 2017 Author

dmerejkowsky commented Mar 24, 2017

This comment has been minimized.

zhangyangyu Apr 10, 2017 Member

This comment has been minimized.

dmerejkowsky Apr 10, 2017 Author

This comment has been minimized.

zhangyangyu Apr 10, 2017 Member

This comment has been minimized.

dmerejkowsky Apr 10, 2017 Author

dmerejkowsky commented Apr 10, 2017 • edited

This comment has been minimized.

zhangyangyu Apr 10, 2017 Member

This comment has been minimized.

serhiy-storchaka Apr 10, 2017 Member

This comment has been minimized.

dmerejkowsky Apr 11, 2017 Author

This comment has been minimized.

serhiy-storchaka Apr 10, 2017 Member

This comment has been minimized.

dmerejkowsky Apr 11, 2017 Author

This comment has been minimized.

berkerpeksag Apr 26, 2017 • edited Member

dmerejkowsky commented Apr 11, 2017

dmerejkowsky commented Apr 26, 2017

louisom commented Apr 26, 2017

This comment has been minimized.

berkerpeksag Apr 26, 2017 Member

This comment has been minimized.

dmerejkowsky Apr 27, 2017 Author

This comment has been minimized.

berkerpeksag Apr 26, 2017 Member

This comment has been minimized.

dmerejkowsky Apr 27, 2017 Author

berkerpeksag commented Apr 26, 2017

This comment has been minimized.

serhiy-storchaka Apr 27, 2017 Member

This comment has been minimized.

dmerejkowsky Apr 28, 2017 Author

This comment has been minimized.

serhiy-storchaka Apr 27, 2017 Member

This comment has been minimized.

dmerejkowsky Apr 28, 2017 • edited Author

This comment has been minimized.

serhiy-storchaka Apr 27, 2017 Member

This comment has been minimized.

dmerejkowsky commented Feb 15, 2017 •

edited by bedevere-bot

berkerpeksag Feb 19, 2017
Member

zhangyangyu Mar 8, 2017
Member

dmerejkowsky Mar 11, 2017
Author

zhangyangyu Mar 8, 2017
Member

dmerejkowsky Mar 11, 2017
Author

berkerpeksag Mar 15, 2017
Member

dmerejkowsky Mar 15, 2017
Author

berkerpeksag Mar 15, 2017
Member

dmerejkowsky Mar 15, 2017
Author

berkerpeksag Mar 15, 2017
Member

dmerejkowsky Mar 15, 2017
Author

zhangyangyu Apr 10, 2017
Member

dmerejkowsky Apr 10, 2017
Author

zhangyangyu Apr 10, 2017
Member

dmerejkowsky Apr 10, 2017
Author

dmerejkowsky commented Apr 10, 2017 •

edited

zhangyangyu Apr 10, 2017
Member

serhiy-storchaka Apr 10, 2017
Member

dmerejkowsky Apr 11, 2017
Author

serhiy-storchaka Apr 10, 2017
Member

dmerejkowsky Apr 11, 2017
Author

berkerpeksag Apr 26, 2017 •

edited

Member

berkerpeksag Apr 26, 2017
Member

dmerejkowsky Apr 27, 2017
Author

berkerpeksag Apr 26, 2017
Member

dmerejkowsky Apr 27, 2017
Author

serhiy-storchaka Apr 27, 2017
Member

dmerejkowsky Apr 28, 2017
Author

serhiy-storchaka Apr 27, 2017
Member

dmerejkowsky Apr 28, 2017 •

edited

Author

serhiy-storchaka Apr 27, 2017
Member

dmerejkowsky Apr 28, 2017
Author

serhiy-storchaka Apr 27, 2017
Member

serhiy-storchaka Apr 27, 2017
Member

dmerejkowsky Apr 28, 2017
Author

serhiy-storchaka Apr 27, 2017
Member

dmerejkowsky Apr 28, 2017
Author

serhiy-storchaka Apr 27, 2017
Member

dmerejkowsky Apr 28, 2017
Author

serhiy-storchaka Nov 24, 2017
Member

berkerpeksag Nov 24, 2017 •

edited

Member

serhiy-storchaka Nov 24, 2017
Member