Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.4k
bpo-28334: fix netrc not working when $HOME is not set#123
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
the-knights-who-say-ni commentedFeb 15, 2017
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 thePSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username onbugs.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:
Thanks again to your contribution and we look forward to looking at it! |
dmerejkowsky commentedFeb 15, 2017
As discussed duringpatch review, no tests were added, but doc was updated. |
dmerejkowsky commentedFeb 15, 2017
@the-knights-who-say-ni : |
berkerpeksag left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 theFileNotFoundError case by changing$HOME env variable.
Doc/library/netrc.rst Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Style nit:
..versionchanged::3.7 Now uses:func:`os.path.expanduser`, so if [...]
dmerejkowsky commentedFeb 20, 2017
Rebased, added a test, and fix doc style as requested. |
Lib/test/test_netrc.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would like the name justtest_file_not_found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
agreed
Lib/test/test_netrc.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
When you explicitly pass a not exist file, it could also raise the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK. I wrote an other test for this. (I like to follow the 'one assert per test' rule)
zhangyangyu commentedMar 13, 2017
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 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks! Looks good to me, just left some minor comments. Also, since this a behavior change we need to add a note toMisc/NEWS.
Doc/library/netrc.rst Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Makes sense.
Doc/library/netrc.rst Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Double spaces before "will".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oups, sorry :)
Lib/test/test_netrc.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No need to useunlikely_file here. Just pass"unlikely_netrc" to the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
agreed
dmerejkowsky commentedMar 24, 2017
@berkerpeksag rebased and conformed with your latest comments |
Doc/library/netrc.rst Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Two spaces before "If the file ...".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
Misc/NEWS Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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. :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also done. It's a bit long, but I hope it documents precisely the new behavior
df1f1bc to09abcafComparedmerejkowsky commentedApr 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@zhangyangyu Thanks for the suggestions! It's worth making sure the documentation is as best as possible :) |
Misc/NEWS Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Doc/library/netrc.rst Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fair enough. I removed the sentence.
Lib/test/test_netrc.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This doesn't test a new behavior. Needed a test for the case when HOME is not set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This doesn't test a new behavior
I think it does. It checks thetype 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 monkeypatchos.path.expanduser, but I'm not sure it's useful. My guts tell me such a test is too close to the implementation.
berkerpeksagApr 26, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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:
withsupport.EnvironmentVarGuard()asenviron:environ.unset('HOME')self.assertRaises(FileNotFoundError,netrc.netrc)
dmerejkowsky commentedApr 11, 2017
Wrote a test monkeypatching |
dmerejkowsky commentedApr 26, 2017
@serhiy-storchaka@berkerpeksag@terryjreedy gentle reminder :) |
louisom commentedApr 26, 2017
@dmerejkowsky For reminding, you don't need to squash your commit every change makes, all commit will be squash when the PR is merged. Seedevguide last paragraph. |
Lib/test/test_netrc.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Mockingos.path.expanduser is not needed here in my opinion. If you can change the test to
deftest_home_not_set(self):withsupport.EnvironmentVarGuard()asenviron:environ.unset('HOME')self.assertRaises(FileNotFoundError,netrc.netrc)
then we are good to go! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm afraid not. If we unset HOME and~/.netrcdoes exist on the developer box,os.path.expanduser will read/etc/passwd usingpwd(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
Misc/NEWS Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
~/.netrc -> ``~/.netrc``
(otherwise our doc build will complain)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
berkerpeksag commentedApr 26, 2017
@dmerejkowsky and just one more comment: Please add your name to |
Doc/library/netrc.rst Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe "as determined"?expanduser() itself doesn't return the user's home directory.
Or "as returned byos.path.expanduser('~') ", but this exposes more implementation details and is harder to format as a reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like 'determined', thanks :)
Lib/netrc.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
'/' is Posix specific pathname separator. Windows accepts it too, but it is better to use the native pathname separator,os.sep. Or useos.path.join():
os.path.expanduser(os.path.join("~",".netrc"))
or
os.path.join(os.path.expanduser("~"),".netrc")
dmerejkowskyApr 28, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok.
Doc/library/netrc.rst Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
~/.netrc is Posix-specific syntax. Windows users ofnetrc can even not know what~/ means. Maybe "of the file :file:'.netrc'" of "of the default netrc file"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
agreed
Lib/test/test_netrc.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Check the content of nrc for testing that the configuration is read from the right file. Add a random data in the fake.netrc.
Misc/NEWS Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Too long line.
Add "Patch by Dimitri Merejkowsky."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
Misc/NEWS Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this a new behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nope, fixed
Lib/test/test_netrc.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is too fragile. If rather thanos.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.expandusercalled= []deffake_expanduser(s):called.append(s)environ.set('HOME',d)result=orig_expanduser(s)environ.unset('HOME')returnresultwithtest.support.swap_attr(os.path,'expanduser',fake_expanduser):nrc=netrc.netrc()self.assertTrue(called)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're right, test is less likely to break this way.
That's a Python bug:http://bugs.python.org/issue28334Most likely it will be fixed in Python 3.7:python/cpython#123
* [cbsinteractive] fix extractor* [cbsinteractive] update test cases* [cbsinteractive] extract formats with `CBSIE`* [extractor/common] Fix rtmp and rtsp formats' URLs in _extract_wowza_formats* [vier] Extract more infoExtract the `episode_number` and `upload_date`. Also extract the real`description`.* [vier] Relax regexes and extract more metadata (closes #12539)* [jsinterp] Add support for quoted names and indexers (closes #13123, closes #13130)* [ChangeLog] Actualize* release 2017.05.18* [ChangeLog] Fix typo* [jsinterp] Fix typo and cleanup regexes (closes #13134)* [ChangeLog] Actualize* release 2017.05.18.1* [mitele] Update app key regex* [hitbox] Add support for smashcast.tv (closes #13154)* [njpwworld] Fix extraction (closes #13162)* [toypics] Fix extraction* [toypics] Improve and modernize* [adobepass] Add support for Brighthouse MSO* [toggle] Relax _VALID_URL (closes #13172)* [youtube] Fix DASH manifest signature decryption (closes #8944)* [youtube] Modernize* [streamcz] Add support for subtitles* [downloader/external] Pass -loglevel to ffmpeg downloader (closes #13183)* Credit@zurfyx for atresplayer improvements (#12548)* Credit@mphe for streamango (#12643)* Credit@fredbourni for noovo (#12792)* [ChangeLog] Actualize* release 2017.05.23* Credit@timendum for rai (#11790) and mediaset (#12964)* Credit@gritstub for vevo fix (#12879)* [cbsnews] fix extraction for 60 Minutes videos* [vimeo] Fix formats' sorting (closes #13189)* [postprocessor/ffmpeg] Fix metadata filename handling on Python 2Fixes #13182* [udemy] Fix extraction for outputs' format entries without URL (closes #13192)* [youku] Fix extraction (closes #13191)* [utils] Recognize more patterns in strip_jsonp()Used in Youku Show pages* [youku:show] Fix extraction* [tudou] Merge into youku extractor (fixes #12214)Also, there are no tudou playlists anymore. All playlist URLs points to youkuplaylists.* [bbc] Add support for authentication* Revert "[youtube] Don't use the DASH manifest from 'get_video_info' if 'use_cipher_signature' is True (#5118)"This reverts commit87dc451.* [ChangeLog] Update after the fix for #11381* [ChangeLog] Actualize* release 2017.05.26* [cbsnews] Fix extraction (closes #13205)* [youku] Extract more metadata (closes #10433)* [adn] fix formats extraction* [utils] Drop an compatibility wrapper for Python < 2.6addinfourl.getcode is added since Python 2.6a1. As youtube-dl nowrequires 2.6+, this is no longer necessary.Seepython/cpython@9b0d46d* [cbsinteractive] Relax _VALID_URL (closes #13213)* [beam:vod] Add extractor* [beam] Improve and add support for mixer.com (closes #13032)* [dvtv] Parse adaptive formats as wellThe old code hit an error when it attempted to parse the string"adaptive" for video height. Actually parsing the returned playlists isa good idea because it adds more output formats, including someaudio-only-ones.* [dvtv] Improve and fix playlists support (closes #13063)* [medialaan] Fix videos with missing videoUrlA rough trick to get around the two different json styles medialaan seems to be using.Fix for these example videos:https://vtmkzoom.be/video?aid=45724https://vtmkzoom.be/video?aid=45425* [medialaan] PEP 8 (closes #12774)* [gaskrank] Fix extraction* [gaskrank] Improve (closes #12493)* [abcnews] Add support for embed URLs* [abcnews] Improve and remove duplicate test (closes #12851)* [xhamster] Extract categories (closes #11728)* [xhamster] Fix author and like/dislike count extraction* [xhamster] Simplify (closes #13216)* [youtube] Parse player_url if format URLs are encrypted or DASH MPDs are requestedFixes #13211* [ChangeLog] Actualize* release 2017.05.29* [README.md] Add an example for how to use .netrc on WindowsThat's a Python bug:http://bugs.python.org/issue28334Most likely it will be fixed in Python 3.7:python/cpython#123* [README.md] Mention http_dash_segments protocol* [packtpub] Fix authentication(closes #13240)* [drbonanza] Fix extraction (closes #13231)* [francetv] Relax _VALID_URL* [1tv] Lower preference for http formats (closes #13246)* [youtube] Improve chapters extraction (closes #13247)* [safari] Fix typo (closes #13252)* [YoutubeDL] Don't emit ANSI escape codes on Windows* [godtv] Remove extractor (closes #13175)* [pornhub:playlist] Fix extraction (closes #13281)* [pornhub:uservideos] Add missing raise* [bandcamp:weekly] Add extractor* [bandcamp:weekly] Improve and extract more metadata (closes #12758)* Credit@adamvoss for bandcamp:weekly (#12758)* Credit@mikf for beam:vod (#13032)* Credit@jktjkt for dvtv formats (#13063)* [ChangeLog] Actualize* release 2017.06.05* [tvplayer] Fix extraction (closes #13291)* [rtlnl] Improve _VALID_URL (closes #13295)* [streamango] Make title optional* [streamango] Skip download for test (closes #13292)* [README.md] Clarify output template references (closes #13316)* [README.md] Improve man page formatting* [YoutubeDL] Sanitize more fields (#13313)* [liveleak] Ensure height is int (closes #13313)* [safari] Improve authentication detection (closes #13319)* [sohu] Fix numeric fields* [flickr] Ensure format id is string* [foxgay] Ensure height is int* [gfycat] Ensure filesize is int* [golem] Ensure format id is string* [jove] Ensure comment count is int* [sexu] Ensure height is int* [turbo] Ensure format id is string* [extractor/common] Return unicode string from _match_id* [extractor/generic] Ensure format id is unicode string* [msn] Fix formats extraction* [newgrounds] Improve formats and uploader extraction (closes #13346)* [newgrounds:playlist] Add extractor (closes #10611)* [utils] Improve unified_timestamp* [newgrounds] Extract more metadata (closes #13232)* [rutv] Add support for testplayer.vgtrk.com (closes #13347)* [xfileshare] Modernize and pass referrer* [xfileshare] Add support for rapidvideo (closes #13348)* [compat] Introduce compat_HTMLParseError* [utils] Handle HTMLParseError in extract_attributes (closes #13349)* [xfileshare] PEP 8* [ChangeLog] Actualize* release 2017.06.12* [compat] Add compat_HTMLParseError to __all__* [corus] Add support for history.ca (closes #13359)* [corus] Add support for showcase.ca
During interpreter shutdown Stackless kills tasklets on daemon threads, if theyhave a non-trivial C-stack. This caused an assertion violation.https://bitbucket.org/stackless-dev/stackless/issues/123(grafted from 9fd4f6a9d266250bef5baa692b89516e73b00e62)
berkerpeksag commentedNov 24, 2017
I've fixed merge conflicts, created NEWS entry with blurb and made some cosmetic changes. Is there anything left to do here? |
Lib/test/test_netrc.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
berkerpeksagNov 24, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My suggested test won't work, because of the following code inexpanduser:
if'HOME'notinos.environ:importpwduserhome=pwd.getpwuid(os.getuid()).pw_dir
I have a.netrc file in myHOME directory so the test won't pass:
deftest_foo(self):withsupport.EnvironmentVarGuard()asenviron: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 netrcThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks, now I see that this is the simplest platform-independent test.
Also add more tests to check that proper exceptions are raised
berkerpeksag commentedNov 24, 2017
For some reason, Travis CI didn't pick up latest changes: Serhiy fixed it a while ago and I don't get any warnings in my development environment. |
berkerpeksag commentedNov 24, 2017
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 commentedNov 24, 2017
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 commentedNov 25, 2017
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 :) |
Uh oh!
There was an error while loading.Please reload this page.
https://bugs.python.org/issue28334