GNOME Bugzilla – Bug 690455
Dynamic enum and flag gtype registrations are not namespaced
Last modified: 2013-01-01 20:47:35 UTC
Trying to use the Gst.SeekType enum in master results in the following: In [1]: from gi.repository import Gst; Gst.SeekType.SET gi/module.py:153: Warning: cannot register existing type `SeekType' wrapper = enum_register_new_gtype_and_add(info) ** (ipython:22658): CRITICAL **: pyg_enum_add: assertion `typename != NULL' failed --------------------------------------------------------------------------- SystemError Traceback (most recent call last) <ipython-input-1-ff02bdbcec1d> in <module>() ----> 1 from gi.repository import Gst; Gst.SeekType.SET /home/thiblahute/devel/pitivi/1.0-uninstalled/pygobject/gi/module.pyc in __getattr__(self, name) 314 return registry[key] 315 --> 316 return getattr(self._introspection_module, name) 317 318 def __dir__(self): /home/thiblahute/devel/pitivi/1.0-uninstalled/pygobject/gi/module.pyc in __getattr__(self, name) 151 else: 152 assert g_type == TYPE_NONE --> 153 wrapper = enum_register_new_gtype_and_add(info) 154 155 wrapper.__info__ = info SystemError: error return without exception set I git bisected the issue and it comes from: commit 3fcd987272a779e5ee9173a2c3a043b4b7475842 Author: Simon Feltman <sfeltman@src.gnome.org> Date: Tue Oct 23 13:56:32 2012 -0700 Add support for overriding GObject.Object Shift pygi module mechanics so the introspection generated 'Object' class becomes derived from the static GObject class. Add initial GObject.Object override which sets all static methods back essentially leapfrogging the introspection methods. This sets the stage for having the ability to remove static methods piecemeal in favor of introspection/python in future commits. https://bugzilla.gnome.org/show_bug.cgi?id=672727
I was able to reproduce this with an initial test in ipython. However, after trying vanilla python both interactively, in a console, and in a script and it now longer shows up (even in ipython) which is odd. $ python3 -c 'from gi.repository import Gst; print(Gst.SeekType.SET)' <enum GST_SEEK_TYPE_SET of type SeekType> However, I do believe the commit mentioned could be a problem (or there is some underlying memory corruption somewhere else). There also seems to be multiple things broken here. For instance, the CRITICAL being printed should really be an exception raised from pyg_enum_add (along with the code following it using a g_warning and NULL return). Otherwise we get these "SystemError: error return without exception set" exceptions which are not very helpful. I will review the changes in that commit and continue trying to reproduce again (and perhaps reboot my machine).
I was able to better reproduce the problem as follows: $ python3 -c 'from gi.repository import GObject, Gst; print(Gst.SeekType.SET)' Note GObject also being imported along with Gst. If I only import Gst I don't see the error. I also think I am observing the error only occuring when a typelib is imported which includes python overrides. So for instance: $ python3 -c 'from gi.repository import Atk, Gst; print(Gst.SeekType.SET)' Does not cause the error whereas using GObject, Gtk, or GLib in place of Atk (or any typelib without overrides), the error will occur. This starts to give some clue as to the problem. Does your dev environment or pitivi also include Gst overrides?
I did my own bisect using the following as the test: $ python3 -c 'from gi.repository import GLib, Gst; print(Gst.SeekType.SET)' This led me to the commit: https://bugzilla.gnome.org/show_bug.cgi?id=686795 http://git.gnome.org/browse/pygobject/commit/?id=15e717ce2c2a26c02c913f79bc7cf6876d943e92 The reason this commit causes a problem is that GLib.SeekType is now being pulled in and registered. The issue ends up being is simpler than I thought in that Enum and Flag types created and registered through pygi don't use a named spaced name. So GLib.SeekType and Gst.SeekType are both fighting for "SeekType".
Created attachment 232433 [details] [review] Change dynamic enum and flag gtype creation to use namespaced naming Use the combination of g_base_info_get_namespace and g_base_info_get_name as the name for registering enum and flag types with glib through g_enum_register_static and g_flags_register_static. This avoids conflicts with types like GLib.SeekType and Gst.SeekType. Add better exceptions for invalid registration problems.
Review of attachment 232433 [details] [review]: The patch makes a lot of sense to me. A couple of nitpicks below ::: gi/gimodule.c @@ +64,3 @@ int i; + const gchar *type_name, *namespace; + gchar *full_name = NULL; nitpicking: why full name gets initialized and the other two don't when they are all assigned at the same spot? @@ -112,2 +114,2 @@ type_name = g_base_info_get_name ((GIBaseInfo *) info); - g_type = g_enum_register_static (type_name, g_enum_values); + full_name = g_strconcat(namespace, type_name, NULL); nitpicking: most of the codebase use a space before (
Thanks Simon! Another nitpick, do gtype names need to look like 'GIMarshallingTestsNoTypeFlags', i. e. do they need to be a valid identifier? Or could the namespace and name be separated with a dot, i. e. 'GIMarshallingTests.NoTypeFlags'? Also, I concur with Paolo's comments.
(In reply to comment #5) > Review of attachment 232433 [details] [review]: > > The patch makes a lot of sense to me. A couple of nitpicks below > > ::: gi/gimodule.c > @@ +64,3 @@ > int i; > + const gchar *type_name, *namespace; > + gchar *full_name = NULL; > > nitpicking: why full name gets initialized and the other two don't when they > are all assigned at the same spot? What is a good practice for this case? Should everything just be nulled out? Personally I'd like if the declarations were all just moved to where they are used. (In reply to comment #6) > Thanks Simon! > > Another nitpick, do gtype names need to look like > 'GIMarshallingTestsNoTypeFlags', i. e. do they need to be a valid identifier? > Or could the namespace and name be separated with a dot, i. e. > 'GIMarshallingTests.NoTypeFlags'? I initially tried using a dot separator but glib complained about it.
> > nitpicking: why full name gets initialized and the other two don't when they > > are all assigned at the same spot? > > What is a good practice for this case? Should everything just be nulled out? > Personally I'd like if the declarations were all just moved to where they are > used. > I think the accepted good practice is: - move declaration to the start of the inner code block where the var is used - do not initialize to NULL/FALSE if the var is assigned inconditionally below - declare one variable per line - declare variables more or less in the same order as they are used
Created attachment 232437 [details] [review] Change dynamic enum and flag gtype creation to use namespaced naming Updated patch that fixes style issues. Additional updates: * Added full memory cleanup in error condition. * The zeroing out of the last item in g_enum_values and g_flags_values was also removed. It seemed unnecessary as the memory is created with g_new0 which already does that.
I don't understand this conditional free: + if (enum_value->value_name != enum_value->value_nick) + g_free ((gchar *) enum_value->value_name); Otherwise it looks good to me. Thanks!
Comment on attachment 232437 [details] [review] Change dynamic enum and flag gtype creation to use namespaced naming Please commit with an additional comment about the conditional free. Thanks!
The following fix has been pushed: 6c02ab0 Change dynamic enum and flag gtype creation to use namespaced naming
Created attachment 232464 [details] [review] Change dynamic enum and flag gtype creation to use namespaced naming Use the combination of g_base_info_get_namespace and g_base_info_get_name as the name for registering enum and flag types with glib through g_enum_register_static and g_flags_register_static. This avoids conflicts with types like GLib.SeekType and Gst.SeekType. Add better exceptions and memory cleanup for invalid registration problems.