GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-34384: Fix os.readlink() on Windows by berkerpeksag · Pull Request #8740 · python/cpython

Conversation

berkerpeksag commented Aug 12, 2018 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

os.readlink() now accepts path-like objects on Windows.

https://bugs.python.org/issue34384

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          bpo-34384: Fix os.readlink() on Windows

7a94c3c

os.readlink() now accepts path-like objects on Windows.

the-knights-who-say-ni added the CLA signed label

Aug 12, 2018

bedevere-bot added the awaiting merge label

Aug 12, 2018

berkerpeksag added 2 commits

August 12, 2018 08:45


          add news blurb

618243b


          fix build

35bb6b6

serhiy-storchaka added needs backport to 3.6 labels

Aug 12, 2018

serhiy-storchaka reviewed

Aug 12, 2018

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

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.

LGTM. I have just few comments to tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_os.py Outdated

		self.check_stat(self.filelink, self.filelink_target)

		@unittest.skipUnless(hasattr(os, 'readlink'), 'needs os.readlink()')
		def test_readlink_pathlike(self):

serhiy-storchaka Aug 12, 2018

Copy link

Copy Markdown

Member

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.

Seems there are no tests for readlink() with strings and bytes. Would be nice to test them too.

See also OSErrorTests.test_oserror_filename. Perhaps it can be simplified.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

berkerpeksag Aug 12, 2018

Copy link

Copy Markdown

Member Author

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.

Seems there are no tests for readlink() with strings and bytes. Would be nice to test them too.

Ok, I will create a separate ReadlinkTests test case and move test_readlink_pathlike there as well.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_os.py Outdated

		self.assertTrue(os.path.islink(self.filelink))
		self.check_stat(self.filelink, self.filelink_target)

		@unittest.skipUnless(hasattr(os, 'readlink'), 'needs os.readlink()')

serhiy-storchaka Aug 12, 2018

Copy link

Copy Markdown

Member

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.

Needs also os.symlink(), isn't?

You can test os.readlink() without involving os.symlink() by calling it with non-symlinks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

berkerpeksag Aug 12, 2018

Copy link

Copy Markdown

Member Author

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.

Win32SymlinkTests is already wrapped by @skip_unless_symlink, so I skipped it.

Your suggestion about using os.readlink() without os.symlink() is much better than mine, though.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Aug 12, 2018

Copy link

Copy Markdown

Member

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.

Ah, this is in Win32SymlinkTests? I have not found tests for os.readlink() on Posix. I think os.readlink() tests should be common for Posix and Windows.

Of course testing os.readlink() only without os.symlink() is not enough. But this can increase the test coverage of os.readlink() for the case when creating symlinks is not permitted for some reasons.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

berkerpeksag Aug 12, 2018

Copy link

Copy Markdown

Member Author

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.

Yes, unfortunately the test coverage of os.readlink() is pretty low. I'll try to improve it by creating a separate test case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_os.py Outdated

		def test_readlink_pathlike(self):
		import pathlib
		os.symlink(self.filelink_target, self.filelink)
		filelink = pathlib.Path(self.filelink)

serhiy-storchaka Aug 12, 2018

Copy link

Copy Markdown

Member

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.

FakePath is used for testing general support of the path protocol, not tied to pathlib.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          add some debug prints

b834795

serhiy-storchaka commented Aug 12, 2018

Copy link

Copy Markdown

Member

Since the arguments parsing code is now the same in win_readlink() and posix_readlink(), it may be worth to merge these functions. And it would be nice to convert them to Argument Clinic. But this can be done in a separate issue.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

berkerpeksag commented Aug 12, 2018

Copy link

Copy Markdown

Member Author

Since the arguments parsing code is now the same in win_readlink() and posix_readlink(), it may be worth to merge these functions.

I thought about that, but there is a chance that the readlink implementation for Windows can be more complicated in the future especially if we decide to implement issues like https://bugs.python.org/issue9949. That said, I will have a look at merging both implementation in posix_readlink().

I will address your other comments later today, thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

berkerpeksag added 2 commits

August 12, 2018 10:34


          use FakePath

980b9c4


          increase test coverage

4c8b97e

berkerpeksag commented Aug 12, 2018

Copy link

Copy Markdown

Member Author

@serhiy-storchaka I've just added some tests for os.readlink().

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          skip bytes tests on Windows

09c4335

serhiy-storchaka removed needs backport to 3.6 labels

Aug 13, 2018

berkerpeksag added 2 commits

August 13, 2018 20:12


          try to implement bytes support on Windows

3b51dda


          document new feature

eb30636

berkerpeksag requested a review from zooba

August 13, 2018 18:18

zooba approved these changes

Aug 13, 2018

View reviewed changes

zooba left a comment

Copy link

Copy Markdown

Member

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.

Looks good to me. Serhiy's suggestion of merging callsites also sounds good, but I won't be the one to force you to do it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          merge posix and windows implementations

d42c9b5

berkerpeksag commented Aug 14, 2018

Copy link

Copy Markdown

Member Author

Thanks for the review! d42c9b5 merges POSIX and Windows implementations. I tried to make the diff less noisy, so let me know what do you think.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          adjust news entry

7ccd0ce

berkerpeksag commented Aug 14, 2018

Copy link

Copy Markdown

Member Author

@serhiy-storchaka do you have any further comments about tests?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reviewed

Aug 14, 2018

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

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.

Tests LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_os.py Outdated


		@support.skip_unless_symlink
		@unittest.skipIf(sys.platform == 'win32',
		'os.readlink() always returns str on Windows')

serhiy-storchaka Aug 13, 2018

Copy link

Copy Markdown

Member

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.

It should return the same type on all platforms.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/posixmodule.c

		{
		path_t path;
		#if defined(HAVE_READLINK)
		int dir_fd = DEFAULT_DIR_FD;

serhiy-storchaka Aug 14, 2018

Copy link

Copy Markdown

Member

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.

dir_fd can be common for both implementations, isn't?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/posixmodule.c

		If it is unavailable, using it will raise a NotImplementedError.");
		#endif

		#ifdef HAVE_READLINK

serhiy-storchaka Aug 14, 2018

Copy link

Copy Markdown

Member

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.

#if defined(HAVE_READLINK) || defined(MS_WINDOWS)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

berkerpeksag Aug 14, 2018

Copy link

Copy Markdown

Member Author

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.

It's already defined in line 7408.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Modules/posixmodule.c Outdated

		if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O&\|$O&:readlink", keywords,
		path_converter, &path,
		READLINKAT_DIR_FD_CONVERTER, &dir_fd))
		#if defined(HAVE_READLINK)

serhiy-storchaka Aug 14, 2018

Copy link

Copy Markdown

Member

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 just READLINKAT_DIR_FD_CONVERTER can be used. It is expanded to dir_fd_unavailable on Windows.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/posixmodule.c Outdated

		rdb->SymbolicLinkReparseBuffer.PrintNameLength / sizeof(wchar_t));
		if (path.narrow) {
		Py_SETREF(return_value, PyUnicode_EncodeFSDefault(return_value));
		if (!return_value) {

serhiy-storchaka Aug 14, 2018

Copy link

Copy Markdown

Member

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 check is redundant.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          address review comments

f3e6e2f

berkerpeksag commented Aug 14, 2018

Copy link

Copy Markdown

Member Author

@serhiy-storchaka I've addressed your review comments.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

berkerpeksag merged commit e0b5b20 into python:master

Aug 15, 2018

bedevere-bot removed the awaiting merge label

Aug 15, 2018

berkerpeksag deleted the 34384-readlink branch

August 15, 2018 10:03

berkerpeksag mentioned this pull request

Aug 15, 2018

Remove redundant checks in posixmodule.c #8772

Closed

girtsf mentioned this pull request

Aug 17, 2018

bpo-30618: add readlink to pathlib.Path #8285

Merged

carljm added a commit to carljm/cpython that referenced this pull request

Aug 19, 2018


          Merge branch 'master' into cached-property

977ece0

* master: (107 commits)
  bpo-22057: Clarify eval() documentation (pythonGH-8812)
  bpo-34318: Convert deprecation warnings to errors in assertRaises() etc. (pythonGH-8623)
  bpo-22602: Raise an exception in the UTF-7 decoder for ill-formed sequences starting with "+". (pythonGH-8741)
  bpo-34415: Updated logging.Formatter docstring. (pythonGH-8811)
  bpo-34432: doc Mention complex and decimal.Decimal on str.format not about locales (pythonGH-8808)
  bpo-34381: refer to 'Running & Writing Tests' in README.rst (pythonGH-8797)
  Improve error message when mock.assert_has_calls fails (pythonGH-8205)
  Warn not to set SIGPIPE to SIG_DFL (python#6773)
  bpo-34419: selectmodule.c does not compile on HP-UX due to bpo-31938 (pythonGH-8796)
  bpo-34418: Fix HTTPErrorProcessor documentation (pythonGH-8793)
  bpo-34391: Fix ftplib test for TLS 1.3 (pythonGH-8787)
  bpo-34217: Use lowercase for windows headers (pythonGH-8472)
  bpo-34395: Fix memory leaks caused by incautious usage of PyMem_Resize(). (pythonGH-8756)
  bpo-34405: Updated to OpenSSL 1.1.0i for Windows builds. (pythonGH-8775)
  bpo-34384: Fix os.readlink() on Windows (pythonGH-8740)
  closes bpo-34400: Fix undefined behavior in parsetok(). (pythonGH-4439)
  bpo-34399: 2048 bits RSA keys and DH params (python#8762)
  Make regular expressions in test_tasks.py raw strings. (pythonGH-8759)
  smtplib documentation fixes (pythonGH-8708)
  Fix misindented yaml in logging how to example (pythonGH-8604)
  ...

Reviewers

serhiy-storchaka

serhiy-storchaka left review comments

zooba

zooba approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

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

Uh oh!

Uh oh!

serhiy-storchaka left a comment

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!

serhiy-storchaka commented Aug 12, 2018

Uh oh!

berkerpeksag commented Aug 12, 2018

Uh oh!

berkerpeksag commented Aug 12, 2018

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

berkerpeksag commented Aug 14, 2018

Uh oh!

berkerpeksag commented Aug 14, 2018

Uh oh!

serhiy-storchaka left a comment

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!

berkerpeksag commented Aug 14, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

berkerpeksag commented Aug 12, 2018 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.