GNOME Bugzilla – Bug 341113
pygtk.require uses asserts rather than real error checking
Last modified: 2018-08-17 13:41:10 UTC
pygtk.require uses the assert keyword to check if the requested version is available, and doesn't conflict with a previous requested one. However, assert statements in Python are stripped when using python -O, and so can't be used for real run-time sanity checks. They should be replaced by conditionals that raise appropriate error types (probably ValueError, or a custom subclass of it). Example: piman@toybox:~$ python -O Python 2.3.5 (#2, Mar 6 2006, 10:12:24) [GCC 4.0.3 20060304 (prerelease) (Debian 4.0.2-10)] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> import gtk >>> import pygtk >>> pygtk.require("1.2") # Violates "pygtk before gtk" rule >>> pygtk.require("2.0") # Violates "one required version" rule >>>
Created attachment 65343 [details] [review] Raise a custom. compatible error explicitly, rather than using assert.
There's no need to subclass AssertionError here because the current api here is that nothing is raised in -O mode. That said, I'm not completing sold on this needing to change. Developers shouldn't use -O (at least usually) and end users should be protected by a friendly installer / package management system. Or do you have an example where this needs to be raised when executing with -O?
Consider the following case: 1. A user has only PyGTK 1.2 installed. 2. For whatever reason (user preference, distribution defaults) python -O is being used. 3. A program does pygtk.require("2.0"). This program will continue blindly forward and give a relatively obscure error when the programmer first tries to use a GTK 2.0 feature, rather than on the require(), which would raise a nice "version is not available" message. (This is actually more likely in the GStreamer case, since the GStreamer Python API changed very little compared to the PyGTK one, and so could continue pretty far before running into a problem.) I object to the claim that "the current API is that nothing is raised in -O mode." The current *behavior* is that nothing is raised in -O mode. Why is that desirable, correct behavior? If the developer has explicitly asked for a version, and PyGTK can't provide it, that should always be an error. End users should be protected by their packaging system, but for whatever reason (new software, new versions of software, crappy packaging) they're not always. I think the burden should be to show that ignoring the error is desirable.
The statement about the current api (or lack of one) is primarily to say that the custom exception class that multiply inherits isn't needed. No one should be expecting an AssertionError in -O mode now, so just use a ValueError. I'll buy that a ValueError is needed because of crappy packaging in this case because it is done once and before anything else. I don't think that argument applies to other asserts in pygtk, though. And I still think people developing software shouldn't have -O turned on, except for performance work.
Um, there are no other asserts in PyGTK.
I'm thinking more of the C code. Also, more functionality may be written in python in the future, with more potential for asserts.
But python -O doesn't (and can't) remove C assertions. And if Python C code raises an AssertionError, it will need to be handled even with -O. In other words, there's no way a C assertion can lead to the kind of behavior changes that this relates bug to. Which is one of the reasons Python's assert statement should just be avoided.
I should point out the reason that I subclassed AssertionError is because PyGTK examples (e.g. http://www.async.com.br/faq/pygtk/index.py?req=show&file=faq02.006.htp) give instructions for developers to catch it. So those won't break with this patch.
I disagree that python's assert statement should be avoided because it communicates clearly that something is expected to always be true and that code depends on it. In any case, we've probably discussed this enough. I'm -1 on adding hacky code to raise a subclass of AssertError and +0 on changing this to a ValueError.
pygst merged this patch almost verbatim. If we're doing some voting thing, I'm +1 on preserving documented API and fixing a bug (which my patch does), and -1 on doing anything stupider. Though I would hope it's not possible to vote in API incompatibility, no matter how "hacky" the *single line* is.
I'll leave this up to others to decide if the "documented api" is to raise an assertion error in -O mode. I have a bit of trouble with that formulation because asserts don't fire in -O mode. The referenced FAQ is wrong -- it never has worked in -O mode. Possibly the thing to do is to leave the assert for backward compatibility when __debug__ is true and add a raise ValueError for -O mode.
pygtk is not under active development anymore and had its last code changes in 2013. Its codebase has been archived: https://gitlab.gnome.org/Archive/pygtk/commits/master PyGObject at https://gitlab.gnome.org/GNOME/pygobject is its successor. See https://pygobject.readthedocs.io/en/latest/guide/porting.html for porting info. Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect reality. Feel free to open a task in GNOME Gitlab if the issue described in this task still applies to a recent version of PyGObject. Thanks!