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

berkerpeksag · 2018-08-12T05:36:50Z

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

https://bugs.python.org/issue34384

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

serhiy-storchaka

LGTM. I have just few comments to tests.

serhiy-storchaka · 2018-08-12T06:43:58Z

Lib/test/test_os.py

@@ -2077,6 +2077,13 @@ def test_file_link(self):
        self.assertTrue(os.path.islink(self.filelink))
        self.check_stat(self.filelink, self.filelink_target)

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


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.

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.

serhiy-storchaka · 2018-08-12T06:46:10Z

Lib/test/test_os.py

@@ -2077,6 +2077,13 @@ def test_file_link(self):
        self.assertTrue(os.path.islink(self.filelink))
        self.check_stat(self.filelink, self.filelink_target)

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


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

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

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.

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.

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

serhiy-storchaka · 2018-08-12T06:47:48Z

Lib/test/test_os.py

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


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

serhiy-storchaka · 2018-08-12T06:50:39Z

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.

berkerpeksag · 2018-08-12T06:58:53Z

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!

berkerpeksag · 2018-08-12T09:44:34Z

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

zooba

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.

berkerpeksag · 2018-08-14T06:43:05Z

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.

berkerpeksag · 2018-08-14T07:39:28Z

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

serhiy-storchaka

Tests LGTM.

serhiy-storchaka · 2018-08-13T07:15:55Z

Lib/test/test_os.py

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


It should return the same type on all platforms.

serhiy-storchaka · 2018-08-14T08:12:43Z

Modules/posixmodule.c

 static PyObject *
 posix_readlink(PyObject *self, PyObject *args, PyObject *kwargs)
 {
    path_t path;
+#if defined(HAVE_READLINK)
    int dir_fd = DEFAULT_DIR_FD;


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

serhiy-storchaka · 2018-08-14T08:14:23Z

Modules/posixmodule.c

@@ -7414,28 +7414,41 @@ If dir_fd is not None, it should be a file descriptor open to a directory,\n\
  and path should be relative; path will then be relative to that directory.\n\
 dir_fd may not be implemented on your platform.\n\
  If it is unavailable, using it will raise a NotImplementedError.");
-#endif
-
-#ifdef HAVE_READLINK


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

It's already defined in line 7408.

serhiy-storchaka · 2018-08-14T08:16:10Z

Modules/posixmodule.c

    PyObject *return_value = NULL;
    static char *keywords[] = {"path", "dir_fd", NULL};

    memset(&path, 0, sizeof(path));
    path.function_name = "readlink";
    if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O&|$O&:readlink", keywords,
                          path_converter, &path,
-                          READLINKAT_DIR_FD_CONVERTER, &dir_fd))
+#if defined(HAVE_READLINK)


I think just READLINKAT_DIR_FD_CONVERTER can be used. It is expanded to dir_fd_unavailable on Windows.

serhiy-storchaka · 2018-08-14T08:24:55Z

Modules/posixmodule.c

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


This check is redundant.

berkerpeksag · 2018-08-14T10:07:46Z

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

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

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

add some debug prints

b834795

berkerpeksag added 2 commits August 12, 2018 10:34

use FakePath

980b9c4

increase test coverage

4c8b97e

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

merge posix and windows implementations

d42c9b5

adjust news entry

7ccd0ce

serhiy-storchaka reviewed Aug 14, 2018

View reviewed changes

address review comments

f3e6e2f

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

berkerpeksag commented Aug 12, 2018 •

edited by bedevere-bot

Loading

serhiy-storchaka left a comment

serhiy-storchaka Aug 12, 2018

berkerpeksag Aug 12, 2018

serhiy-storchaka Aug 12, 2018

berkerpeksag Aug 12, 2018

serhiy-storchaka Aug 12, 2018

berkerpeksag Aug 12, 2018

serhiy-storchaka Aug 12, 2018

serhiy-storchaka commented Aug 12, 2018

berkerpeksag commented Aug 12, 2018

berkerpeksag commented Aug 12, 2018

zooba left a comment

berkerpeksag commented Aug 14, 2018

berkerpeksag commented Aug 14, 2018

serhiy-storchaka left a comment

serhiy-storchaka Aug 13, 2018

serhiy-storchaka Aug 14, 2018

serhiy-storchaka Aug 14, 2018

berkerpeksag Aug 14, 2018

serhiy-storchaka Aug 14, 2018

serhiy-storchaka Aug 14, 2018

berkerpeksag commented Aug 14, 2018

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

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

Conversation

berkerpeksag commented Aug 12, 2018 • edited by bedevere-bot Loading

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka commented Aug 12, 2018

berkerpeksag commented Aug 12, 2018

berkerpeksag commented Aug 12, 2018

zooba left a comment

Choose a reason for hiding this comment

berkerpeksag commented Aug 14, 2018

berkerpeksag commented Aug 14, 2018

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkerpeksag commented Aug 14, 2018

berkerpeksag commented Aug 12, 2018 •

edited by bedevere-bot

Loading