bpo-34384: Fix os.readlink() on Windows by berkerpeksag · Pull Request #8740 · python/cpython (original) (raw)
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 our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation22 Commits12 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
os.readlink() now accepts path-like objects on Windows.
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.
| @@ -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): |
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.
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.
| @@ -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()') |
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.
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.
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.
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.
| def test_readlink_pathlike(self): |
|---|
| import pathlib |
| os.symlink(self.filelink_target, self.filelink) |
| filelink = pathlib.Path(self.filelink) |
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.
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.
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!
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests LGTM.
| @support.skip_unless_symlink |
| @unittest.skipIf(sys.platform == 'win32', |
| 'os.readlink() always returns str on Windows') |
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.
| static PyObject * |
|---|
| posix_readlink(PyObject *self, PyObject *args, PyObject *kwargs) |
| { |
| path_t path; |
| #if defined(HAVE_READLINK) |
| int dir_fd = DEFAULT_DIR_FD; |
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?
| @@ -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 |
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)
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.
| 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) |
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.
| rdb->SymbolicLinkReparseBuffer.PrintNameLength / sizeof(wchar_t)); |
|---|
| if (path.narrow) { |
| Py_SETREF(return_value, PyUnicode_EncodeFSDefault(return_value)); |
| if (!return_value) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is redundant.
carljm added a commit to carljm/cpython that referenced this pull request
- 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) ...