After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 656554 - Marshalling GVariants does not work for closures
Marshalling GVariants does not work for closures
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
Git master
Other Linux
: Normal normal
: ---
Assigned To: Martin Pitt
Python bindings maintainers
Depends on:
Blocks: 656325
 
 
Reported: 2011-08-15 10:19 UTC by Martin Pitt
Modified: 2012-05-14 11:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test case (1.14 KB, application/x-shellscript)
2011-08-15 10:19 UTC, Martin Pitt
  Details
Support marshalling of GVariants for closures (2.97 KB, patch)
2012-05-06 01:26 UTC, Martin Pitt
none Details | Review
Support marshalling of GVariants for closures (3.91 KB, patch)
2012-05-09 21:27 UTC, Martin Pitt
none Details | Review
Support marshalling of GVariants for closures (4.28 KB, patch)
2012-05-09 21:39 UTC, Martin Pitt
none Details | Review

Description Martin Pitt 2011-08-15 10:19:57 UTC
Created attachment 193853 [details]
test case

In bug 656325 we try to make Gio fully introspectable, for providing GDBus support in Python (bug 656330). The initial patch there used callbacks, which were rather easy to fix with GVariant arguments and return values (18a240cc492 and 5189b360c).

However, now that bug 656325 requests using closures, we have the same problem there: calling a closure with a GVariant argument currently just yields a TypeError, from

  http://git.gnome.org/browse/pygobject/tree/gi/_gobject/pygtype.c?id=081dc2eb#n1153

Adding variant support there is surprisingly difficult, as this is using its own marshaller; _pygi_struct_new() is not available there, as _gobject.so does not link against _gi.so.

I also tried to monkey-add an equivalent using the _gobject API:

PyObject*
pyg_variant_new (GVariant* variant)
{
    PyGIStruct *self;
    /*
    PyTypeObject *type = (PyTypeObject *) _pygi_type_get_from_g_type(G_TYPE_VARIANT);
    */
    PyTypeObject *type = (PyTypeObject *) pyg_type_wrapper_new(G_TYPE_VARIANT);
    printf("pyg_variant_new: got type %p\n", type);

    self = (PyGIStruct *) type->tp_alloc(type, 0);
    printf("pyg_variant_new: got self %p\n", self);
    if (self == NULL) {
        return NULL;
    }

    ( (PyGPointer *) self)->gtype = G_TYPE_VARIANT;
    ( (PyGPointer *) self)->pointer = variant;
    self->free_on_dealloc = TRUE;

    return (PyObject *) self;
}

But that just crashed, apparently pyg_type_wrapper_new() doesn't quite work here.

John says that the big and best solution would be to get rid of all the marshalling in _gobject and just use the new one from gi/pygi-marshal-*, but all of this would need to move from _gi.so to gi/_glib/ first, together with quite some refactorization.

I added a test method to g-i's Regress (in master already), and prepared a test case. It shows that closures with simple arguments do work, but not with GVariant arguments/return type.
Comment 1 Martin Pitt 2011-10-27 05:22:04 UTC
I committed the test case with marking the gvariant part as EXFAIL. This ensures that the other part (passing int around) will keep working, and also makes it easier to test patches for this bug, by merely removing the @expectedFailure decorator.
Comment 2 Martin Pitt 2012-05-06 01:26:24 UTC
Created attachment 213532 [details] [review]
Support marshalling of GVariants for closures

On today's plane ride I had a fresh look at this, and figured it out. There it is, review appreciated!
Comment 3 Johan (not receiving bugmail) Dahlin 2012-05-07 15:17:52 UTC
Review of attachment 213532 [details] [review]:

The rest looks good though.

::: gi/_gobject/pygtype.c
@@ +995,3 @@
+	if (obj == Py_None)
+	    g_value_set_variant(value, NULL);
+	else if (g_strcmp0(obj->ob_type->tp_name, "Variant") == 0)

This is quite ugly.
It'd be better to import the PyGVariant_Type from gi.repository.GLib and do a proper isinstance() here.
Comment 4 Martin Pitt 2012-05-07 19:51:47 UTC
(In reply to comment #3)
> +    else if (g_strcmp0(obj->ob_type->tp_name, "Variant") == 0)
> 
> This is quite ugly.

I agree.

> It'd be better to import the PyGVariant_Type from gi.repository.GLib

We can duplicate the logic from _pygi_type_import_by_name() for GLib.Variant certainly, but that means having to import gi.repository.GLib into _gobject just for that. This seems much more expensive, and I'm not sure whether it can have bad side effects? (i. e. _gobject importing GLib and GLib importing _gobject)
Comment 5 Martin Pitt 2012-05-09 21:27:04 UTC
Created attachment 213769 [details] [review]
Support marshalling of GVariants for closures

Here is a cleaner variant which imports GLib.Variant properly instead of just doing string comparison on the PyObject type.
Comment 6 Martin Pitt 2012-05-09 21:39:37 UTC
Created attachment 213770 [details] [review]
Support marshalling of GVariants for closures

This cleans up the error handling. No need to manually set errors, PyImport_ImportModule() and PyObject_GetAttrString() already do that for us. This also fixes pyg_closure_marshal() to only set the generic "can't convert return value to desired type" error when we don't already have one. With that, you will now see

test_variant (test_everything.TestClosures) ... Traceback (most recent call last):
  • File "../gi/module.py", line 323 in __getattr__
    attr = super(DynamicGLibModule, self).__getattr__(name)
  • File "../gi/module.py", line 249 in __getattr__
    return getattr(self._introspection_module, name)
  • File "../gi/module.py", line 107 in __getattr__
    self.__name__, name))
AttributeError: 'gi.repository.GLib' object has no attribute 'Variantt'
ERROR

if the import fails (I temporarily damaged the code to import "Variantt" to test this).
Comment 7 Martin Pitt 2012-05-14 11:32:24 UTC
Argh, I accidentally pushed this already during bugzilla/patch review this morning. I think it's good, but it wasn't officially stamped by another developer.