bpo-41530: Handle unhandled IsADirectoryError and PermissionError in zoneinfo.ZoneInfo #21839
Conversation
Additionally handles `IsADirectoryError`, `PermissionError`, and `ValueError` raised when accessing specific resource names.
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). Recognized GitHub usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
ValueError is documented as an exception raised elsewhere and as such need not be handled.
|
Why did you close your PR? |
|
I'm aware of this ignoring the Unsure as to what would be a good plan of attack to solve that issue. one crude solution could be to assume only TZif files lack extensions however there's obviously large room for future issues derived from that soltution. |
Lib/zoneinfo/_common.py
Outdated
| import importlib.util | ||
|
|
||
| package_spec = importlib.util.find_spec(package_name) | ||
| resource_path = os.path.join(os.path.dirname(package_spec.origin), key) |
There was a problem hiding this comment.
imports+two lines of code is a risk of raising a new exception. You may do that before and use the path to load the resource. I'm not sure.
Is it possible that tzdata is in a ZIP file, or in memory but not on disk?
Maybe it's overcomplicated and maybe it's better to convert any OSError into a ZoneInfoNotFoundError. I let @pganssle decides what is the best.
There was a problem hiding this comment.
Yeah, I don't like this at all. In importlib_resources there's a way to determine if something is a directory or not using the importlib.resources.files API.
That will complicate things a bit for the backport, though, which doesn't currently require importlib_resources in Python 3.8.
I'm inclined to go the simple way and catch all OSErrors instead of the more narrowly scoped ones. I realize this may frustrate some people debugging in unusual environments by disguising the source of some errors, but I don't love that the standard ZoneInfo constructor is exposing details about the specifics of the underlying file system.
There was a problem hiding this comment.
@jaraco Do you know if there's an equivalent of importlib.resources.files('my.package').joinpath('resource').isdir() that works for Python 3.8's version of importlib.resources?
There was a problem hiding this comment.
In Python 3.8 and earlier, importlib.resources didn't support directories as resources - all resources were required to be files directly in a package.
There was a problem hiding this comment.
@jaraco Well, we're not really concerned with using a directory as a resource, the problem is that it appears that importlib.resources raises PermissionError on Windows when a directory is specified, and we're trying to distinguish between the case where a purported resource exists but is a directory and the case of a legitimate permissions error.
Is there no way to do that in the Python 3.8 importlib.resources?
There was a problem hiding this comment.
swapped to using importlib.resources.files to locate the resource however inclined to agree that catching all OSError derivatives might prove less problematic.
There was a problem hiding this comment.
@pganssle Apologies for having dropped the ball on this one.
I don't believe there's a reliable in the older importlib.resources API to detect that a directory is present. It was meant to be the case that a directory was indistinguishable from 'not a resource'.
What I would recommend though is to use importlib.resources here, but in the zoneinfo backport, use importlib_resources, which implements the files() API even on Python 2.
Other OSError derivatives should be ignored.
There was a problem hiding this comment.
I'm still not convinced about whether this is the right solution, but regardless of how we solve the issue, it does need tests.
Again I do think it would be better in some ways to start this as a PR against backports.zoneinfo to take advantage of the improved CI setup there, then migrate the code into CPython wholesale, but if you don't want to do that I will just make sure to backport the code before this is merged.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Happy to start a PR against backports, won't be able to for about 10 hours or so though. |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @pganssle: please review the changes made to this pull request. |
Handles
IsADirectoryErrorandPermissionErrorraised when accessing specific resource names.https://bugs.python.org/issue41530