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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support errors with two filenames for errno exceptions #64716
Comments
There are a bunch of functions provided by Python, e.g. PyErr_SetFromErrnoWithFilenameObject(), that allow specifying a filename associated with the error. But there are some errors that really need two filenames, like copy(), symlink(), and rename(). The error could be on only one file, but some errors could apply to either or both, and errno's error doesn't always provide enough context to tell which it would be. I propose that we add new APIs that allow specifying a second filename. We take all the *WithFilename* APIs and add the *WithFilenames* equivalent (e.g. PyErr_SetFromErrnoWithFilenameObjects()). Internally, oserror_parse_args() would now parse an extra "filename2" entry in the tuple, just after the "filename" entry (but before the possible "winerror" entry). Currently when formatting an error with a filename, the format string looks like |
"But there are some errors that really need two filenames, like copy(), symlink(), and rename()." I think *need* is too strong word in this case. I agree that two filenames is better than none. But I don't see anything wrong from omitting filenames in error messages for copy(), etc. If PHP and Perl are taking "omitting filenames" way, surely there is some merit in that way. I am in if we are taking "two filenames" way just like Ruby does. It's just isn't it too rush for Python 3.4? |
As release manager Larry has the right to add a new feature after feature freeze. |
Serhiy: I'm not sure if it's the language barrier, but that came across kind of mean. It's not that I can do whatever I want as release manager, but that I have the say in whether something is a "bug fix" or a "new feature", which is how we decide whether or not something is allowed into Python after feature freeze. This issue has been on my radar for a while (originally bpo-16074). But I wasn't paying strong attention to it. Nobody in that issue came up with a solution I liked. Finally when you posted your patch I said "ugh, can't we do better" and had to think about it before I realized we should just display both filenames. If somebody had posted a patch with that two months ago I would have happily accepted it and we wouldn't be having this conversation now. Vajrasky: My goal is that Python is nicer to use than PHP or Perl. And it's more than a month before 3.4 final is scheduled to be released. This patch is a pretty mechanical change--create new function, accept extra parameter, make the tuple one entry longer. I don't expect it to be destabilizing. However, I *was* hoping that one of the original authors of the code in question would come forth and say a) whether or not they think it's a good idea in general, and b) if they think the specific approach is fine. The patch is a bit stalled because of higher-priority Argument Clinic changes. I could post a partial patch if someone wanted to pick it up and finish it. |
Actually that was Vajrasky's patch, and that was extended version of your I agree that support errors with two filenames is better solution than remove |
Here's a first cut at a patch. With this applied Python passes the whole test suite. I was surprised at how ticklish the OSError object was about adding a fifth member, with this weird "exception tuples can only have two members" policy. But test_exceptions helped me find all the problems. |
Added a test checking that the error messages show up properly. |
Are *WithUnicodeFilenames() functions needed? Py_UNICODE API considered as deprecated and there is no need to support compatibility with older versions. |
There aren't any deprecation warnings in the code. |
But the PyErr_ functions that accept Py_UNICODE aren't marked deprecated. http://docs.python.org/3.4/c-api/exceptions.html#unicode-exception-objects |
Perhaps they should be. Note that all functions that accept Py_UNICODE are not In any case I don't think we should add *new* functions with deprecated API. |
I agree. |
Attached is patch #3. This one has been tested on Linux, Windows 7 64-bit, and Snow Leopard 64-bit. Windows Server 2008 32-bit and 64-bit Changes:
|
I'm not sure that PyErr_SetFromWindowsErrWithFilenames() and PyErr_SetExcFromWindowsErrWithFilenames() will be useful. "const char*" filenames are not recommended on Windows. Victor can say more. |
New patch incorporating Serhiy's suggestions. Thanks, Serhiy! Did more testing with the buildbots. Windows Server 2008 32-bit and 64-bit were both fine. So were ARMv7, OpenIndiana 64-bit, Gentoo 32-bit, FreeBSD 10 64-bit, and PowerLinux PPC 64-bit. (This was all run using diff #3, but diff #4 doesn't change any C code. It just changes the test and some docs.) Can I get a LGTM? |
One more tweak from Serhiy. |
Support of bytes filenames has ben deprecated on Windows, Unicode is really |
Okay. I have revived the Py_UNICODE functions I removed in patch #3. The patch now works fine on my Ubuntu 13.10 64-bit box, and at least compiled on a Windows buildbot. It's now building on nine buildbots. Assuming the buildbots look good, can I check this in? |
New changeset 081a9d8ba3c7 by Larry Hastings in branch 'default': |
It's in! And the buildbots look healthy. |
Why did you add so many versions of the same functions? Only PyErr_SetExcFromWindowsErrWithFilenameObjects() and PyErr_SetFromErrnoWithFilenameObjects() are used. The Py_UNICODE type is deprecated, you should not add new functions using it in Python 3.4. +PyObject *PyErr_SetFromWindowsErrWithUnicodeFilenames( And you should avoid passing raw bytes string to build an error message, you probably has the Python object version of the filename somewhere in your code. +PyAPI_FUNC(PyObject *) PyErr_SetFromErrnoWithFilenames( +PyObject *PyErr_SetExcFromWindowsErrWithFilenames( In Python 3.3, there are also too many functions to raise an OSError, I don't that you should so many new functions. Please remove:
Having two filenames in OSError is the best fix for functions like os.rename when we don't know which path raised the error. I remember that it was hard to guess if the source or the destination was the problem, so thanks for working on this. Note: When I wrote "Unicode is really the native type", I mean a PyObject* object which is a str, not Py_UNICODE* which is deprecated. |
The family of "PyErr_SetExcFrom..." functions was used when there were various kind of exceptions: select.error, mmap.error, OSError, IOError, socket.error, etc. The "PEP-3151: Reworking the OS and IO exception hierarchy" has been implemented in Python 3.3. I'm not sure that we need such function anymore, a function always raising OSError is probably enough: "PyErr_SetExcFromWindowsErrWithFilenameObjects" name should be just "PyErr_SetFromWindowsErrWithFilenames". I hate such long names :-( |
"And you should avoid passing raw bytes string to build an error message, you probably has the Python object version of the filename somewhere in your code." Oh, I remember the reason why char* must not be used to build an OSError: on Windows, you should never try to decode a bytes filename, because it may raise a UnicodeDecodeError while you are trying to build an OSError. See issue bpo-15478 for the rationale. Just pass the original PyObject* you get in path_t. There is even an unit test to ensure that OSError.filename is the original name: OSErrorTests.test_oserror_filename() in test_os. |
Talked it over with Victor in IRC. I agree it's best to only add the WithFilenameObjects functions, as best practice requires using the original PyObject * passed in when creating the OSError. The attached patch removes all the new WithFilenames and WithUnicodeFilenames functions. Note that I merely amend the existing NEWS entry rather than add a new one; the functions only lived in trunk for a couple of hours. |
I applied larry.oserror.remove.with.filenames.etc.1.diff on default: On Windows, the code compiles fine and test_os pass. |
New changeset 6343bdbb7085 by Larry Hastings in branch 'default': |
Buildbots are basically happy with it. It's checked in. This was the last checkin before 3.4.0rc1 was tagged! |
I think it is worth to mention this in Doc/whatsnew/3.4.rst, as this is a little incompatible change. Python 3.3: >>> x = OSError(2, 'No such file or directory', 'foo', 0, 'bar')
>>> x.args
(2, 'No such file or directory', 'foo', 0, 'bar') Python 3.4: >>> x = OSError(2, 'No such file or directory', 'foo', 0, 'bar')
>>> x.args
(2, 'No such file or directory') |
In 3.3: >>> x = OSError(2, 'No such file or directory', 'foo', 0, 'bar')
>>> str(x)
"(2, 'No such file or directory', 'foo', 0, 'bar')" So, I don't see this as a realistic backwards compatibility problem worthy of a porting note. |
OK then. |
I was going to wonder if the args thing was a bug, but I see that actually it continues the backward-compatibility tradition already established (python3.3): >>> x = OSError(2, 'No such file or directory', 'abc')
>>> str(x)
"[Errno 2] No such file or directory: 'abc'"
>>> x.args
(2, 'No such file or directory') |
(Ether that, or it is a long-standing bug.) |
Yeah, I admit I don't understand what problem that code was solving. But it looked Very Deliberate so I preserved the behavior. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: