GNOME Bugzilla – Bug 598414
Add gobject introspection support
Last modified: 2019-02-22 11:45:49 UTC
gobject introspection don't build from gir-repository. This patch add support gobject introspection build from source.
No patch attached. BTW, the gnome_keyring * functionality will be deprecated in the future. It's replacement is under development. Not sure if this affects your desire to work on it. However I'll continue to accept patches for the libgnome-keyring client library.
Created attachment 145422 [details] [review] support gobject introspection sorry, add patch.
Sorry for the confusion, but in the comment above, what I meant to say was that the gnome_keyring_* functionality has moved to the libgnome-keyring client library. Any patches for the gnome-keyring library should be for that module. Thanks!
Sorry, wrong CC:.
Currently the only way to access gnome keyring functionality from python is the python-gnomekeyring module, which depends on gtk2 and doesn't support python3. Gir support will improve the situation.
Dmitry, What do you recommend as a next step? This issue is currently causing Rhythmbox to crash on startup if the Magnatune plugin is enabled. This related bug report has more details about that: Magnatune plugin crashes Rhythmbox https://bugzilla.gnome.org/show_bug.cgi?id=661957 Mark
I don't know a workaround for such crashes (maybe you can use a standalone process for keyring operations?) Good news is that Stef is preparing a new, better library for keyring operations, and it will support introspection.
FWIW, I don't have anything against anyone working on properly exposing a subset of the old libgnome-keyring API via gobject-introspection. It'd be hard to do the entire thing, due to the strange API, callbacks and cancellation.
For reference, libgnome-keyring is the only library for which we keep gir-repository in openSUSE...
Just to avoid duplicate efforts, I'm going to take a shot at this in the next days as we need it in Ubuntu rather urgently as well.
Thank you, Martin!
Comment on attachment 145422 [details] [review] support gobject introspection Marking the old patch as obsolete, as it was against gnome-keyring, and http://live.gnome.org/GObjectIntrospection/AutotoolsIntegration recommends a different approach now.
Created attachment 205289 [details] [review] [1/7] Fix gnome_keyring_item_info_copy() While I was working on GI support I noticed that gnome_keyring_item_info_copy() is broken, it copies _from_ the newly created copy instead of the original. Straightforward patch attached. OK to push this?
Created attachment 205290 [details] [review] [2/7] Build introspection typelib I got the GI support mostly working now. It is not that easy to get libg-k introspectable, as it uses quite a few varargs functions and also does typedef GArray GnomeKeyringAttributeList; GArray is only useful in C, it's not introspectable at all. I wanted to keep the current library ABI and API, so in order to support attribute lists and the union in GnomeKeyringAttribute I added some wrappers/accessors to the API. The resulting Python code looks a bit ugly, but at least you can do most of the supported functionality through GI now. The last patch in the series adds a "test-gi.py" script which exercises the functionality through pygobject. Once this gets accepted and into a release, I'm happy to add some overrides to pygobject to make using the API less clumsy from Python. This first patch provides the autotools integration, straight from the book (https://live.gnome.org/GObjectIntrospection/AutotoolsIntegration). Note that I called the version "1.0", which is pretty much arbitrary (note that this should not follow the library SONAME, as GI bindings are not that sensitive to mere ABI changes).
Created attachment 205291 [details] [review] [3/7] Add GI annotations This patch adds/fixes the GI annotations, and also fixes some typos in the documentation. This provides the bulk of necessary fixes for making this introspectable. It does not contain any code changes, just changes to the doc comments. The code changes follow in separate patches to ease the review a little.
Created attachment 205292 [details] [review] [4/7] Box structs to make them introspectable This provides boxing for the structs, so that bindings can automatically do the memory management/copying/freeing for them. This bumps the library version info as this adds some new API. It stays completely backwards API/ABI compatible, though.
Created attachment 205293 [details] [review] [5/7] Allow bindings to use GnomeKeyringAttributeList This makes GnomeKeyringAttributeList accessible. Details in patch commit log. This extends the API again but keeps backwards compat. The previous patch already bumps version info, and I suppose they will all go into master together, so no need to bump it again.
Created attachment 205294 [details] [review] [6/7] Add GnomeKeyringAttribute union accessor methods This provides access to the value (string/int union member) of a GnomeKeyringAttribute. I'm not sure whether the inability to access union members is a shortcoming of pygobject or g-i in general; but either way, getting attribute values is rather important, so let's add accessor methods.
Created attachment 205295 [details] [review] [7/7] Add Python test script for GI binding And finally, a test Python script to test the binding. Run this with LD_LIBRARY_PATH=library/.libs/ library/tests/test-gi.py -v This is a Python 3 script, so you'll need pygobject for Python 3. If you prefer a Python 2 version, I can also change it (it's rather easy to do, just changing the print statements).
Created attachment 205339 [details] [review] Fix up documentation after adding introspection
Review of attachment 205289 [details] [review]: Thanks for catching this. You can go ahead and push this patch. Will do further review of the others.
Review of attachment 205291 [details] [review]: BTW, thanks for doing this work! Does it make sense to introspect the gnome_keyring_memory_xxx functions? Trying to use non-pageable memory from languages like javascript, python (or even vala) would be pretty futile no? This is just a partial review. I need to go out for a bit and I'll continue later. ::: library/gnome-keyring.c @@ +575,2 @@ * @destroy_data: A function to free @data when it's no longer needed. * Do the async callbacks work? Don't you need a (closure) annotation? If so, then this comment applies to all the async functions. @@ +899,1 @@ **/ Does cancellation work? If so, then how do we scope the validity of the returned value to prevent a crash if used incorrectly? If cancellation doesn't work, then should we skip return values from async functions and just not support cancellation in the introspected API? @@ +2061,3 @@ + * keyring. + * @ids: (out) (element-type guint): The location to store a %GList of item ids + * (ie: unsigned integers). Does this annotation correctly represent a GList not of guint pointers, but of GUINT_TO_POINTER style guints? @@ +3829,3 @@ * @id: The id of the item * @attributes: The location to return a pointer to the attribute list. * Did you file a bug with the gobject-introspection guys about this crash?
Comment on attachment 205289 [details] [review] [1/7] Fix gnome_keyring_item_info_copy() "Fix gnome_keyring_item_info_copy()" pushed.
(In reply to comment #23) > Does it make sense to introspect the gnome_keyring_memory_xxx functions? Trying > to use non-pageable memory from languages like javascript, python (or even > vala) would be pretty futile no? Right. First they just return a typeless gpointer which confuses at least pygobject quite fast: $ GI_TYPELIB_PATH=library LD_LIBRARY_PATH=library/.libs/ python >>> from gi.repository import GnomeKeyring >>> mem = GnomeKeyring.memory_alloc(1000) >>> GnomeKeyring.memory_is_secure(mem) True >>> mem[0] = 'a' Segmentation fault Second, interpreters tend to copy around stuff quite liberally, so they would presumably land in non-protected memory quite fast. > Do the async callbacks work? Don't you need a (closure) annotation? If so, then > this comment applies to all the async functions. The GI scanner already correctly detects the closures and arguments: <function name="get_info" c:identifier="gnome_keyring_get_info"> [...] <parameter name="callback" transfer-ownership="none" scope="notified" closure="2" destroy="3"> [...] But nevertheless trying to use them crashes at the moment. That's why I disabled the "test_async" check in test-gi.py for the time being. Callbacks to work fine in general in pygobject, I don't yet know off-hand what breaks there. This needs some further debugging. > Does cancellation work? Cannot test until callbacks work. > If so, then how do we scope the validity of the > returned value to prevent a crash if used incorrectly? I'm afraid there is no way to annotate the lifetime of return arguments in GI right now. You will need to be just as careful about them in Python or JS as in C when you use them. > If cancellation doesn't work, then should we skip return values from async > functions and just not support cancellation in the introspected API? Works for me. I'd like to spend some more time to debug why the async methods crash, and if it's not easily fixable, we might just as well (skip) the whole methods for the time being. > @@ +2061,3 @@ > + * keyring. > + * @ids: (out) (element-type guint): The location to store a %GList of item > ids > + * (ie: unsigned integers). > > Does this annotation correctly represent a GList not of guint pointers, but of > GUINT_TO_POINTER style guints? Yes, works fine. I updated test-gi.py to also explicitly test that function, will attach new patch in a sec. > @@ +3829,3 @@ > * @id: The id of the item > * @attributes: The location to return a pointer to the attribute list. > * > > Did you file a bug with the gobject-introspection guys about this crash? Yes, bug 667985. Do you want to (skip) the function for now? Thanks for the followup "documentation fix" patch!
Created attachment 205340 [details] [review] [7/7] Add Python test script for GI binding Updated test-gi.py to also test list_item_ids_sync().
I tried (skip)ing the return value, properly annotating the GnomeKeyringOperationGetKeyringInfoCallback type (add the (closure) annotation to @data), another async function, and other variations, but no success :( OK to skip the async functions for now? I guess it's better to have half the API being introspectable than none at all?
Review of attachment 205292 [details] [review]: Since the boxed types are now part of the API, should we round each one out with #define's like GNOME_KEYRING_TYPE_FOUND, etc..?
Review of attachment 205294 [details] [review]: ::: library/gnome-keyring-utils.c @@ +295,3 @@ +gnome_keyring_attribute_get_string (GnomeKeyringAttribute *attribute) +{ + * use attribute->value.string. Let's use g_return_val_if_fail(), since that's customary for prerequisites. @@ +314,3 @@ +gnome_keyring_attribute_get_uint32 (GnomeKeyringAttribute *attribute) +{ + return attribute->value.string; Ditto here.
(In reply to comment #25) > (In reply to comment #23) > > Does it make sense to introspect the gnome_keyring_memory_xxx functions? Trying > > to use non-pageable memory from languages like javascript, python (or even > > vala) would be pretty futile no? > > Right. First they just return a typeless gpointer which confuses at least > pygobject quite fast: > > $ GI_TYPELIB_PATH=library LD_LIBRARY_PATH=library/.libs/ python > >>> from gi.repository import GnomeKeyring > >>> mem = GnomeKeyring.memory_alloc(1000) > >>> GnomeKeyring.memory_is_secure(mem) > True > >>> mem[0] = 'a' > Segmentation fault > > Second, interpreters tend to copy around stuff quite liberally, so they would > presumably land in non-protected memory quite fast. Okay, let's skip these gnome_keyring_memory_xx() functions. > > If so, then how do we scope the validity of the > > returned value to prevent a crash if used incorrectly? > > I'm afraid there is no way to annotate the lifetime of return arguments in GI > right now. You will need to be just as careful about them in Python or JS as in > C when you use them. Okay, makes sense. > > If cancellation doesn't work, then should we skip return values from async > > functions and just not support cancellation in the introspected API? > > Works for me. I'd like to spend some more time to debug why the async methods > crash, and if it's not easily fixable, we might just as well (skip) the whole > methods for the time being. Alright. > > > @@ +2061,3 @@ > > + * keyring. > > + * @ids: (out) (element-type guint): The location to store a %GList of item > > ids > > + * (ie: unsigned integers). > > > > Does this annotation correctly represent a GList not of guint pointers, but of > > GUINT_TO_POINTER style guints? > > Yes, works fine. I updated test-gi.py to also explicitly test that function, > will attach new patch in a sec. Cool. > > @@ +3829,3 @@ > > * @id: The id of the item > > * @attributes: The location to return a pointer to the attribute list. > > * > > > > Did you file a bug with the gobject-introspection guys about this crash? > > Yes, bug 667985. Do you want to (skip) the function for now? Sounds good. (In reply to comment #27) > I tried (skip)ing the return value, properly annotating the > GnomeKeyringOperationGetKeyringInfoCallback type (add the (closure) annotation > to @data), another async function, and other variations, but no success :( > > OK to skip the async functions for now? I guess it's better to have half the > API being introspectable than none at all? Yes, let's just do the parts that we can do properly for now. Lets leave out the bits that don't work as well. Using introspected APIs is already hard enough due to lack of documentation, so let's make sure that whats exposed at least doesn't crash.
Created attachment 205345 [details] [review] [3/7] Add GI annotations Updated GI annotations patch. This now skips the _memory_* and async stuff, consistently applies the (skip) vs. (skip): syntax, and also folds in most of your "Fix up documentation after adding introspection" patch (the remainder is in patch 4/7), so that changes that belong together are in one patch.
Created attachment 205346 [details] [review] [4/7] Box structs to make them introspectable Changes the g_assert() to g_return_if_fail(), #define GNOME_KEYRING_TYPE_*, and add the new API (including the new _TYPE_ defines) to the documentation sections. Documentation now builds without warnings again.
Created attachment 205348 [details] [review] [5/7] Allow bindings to use GnomeKeyringAttributeList Update patch 5/7: Just unfuzz for the patches that changed earlier.
Created attachment 205349 [details] [review] [6/7] Add GnomeKeyringAttribute union accessor methods Update GnomeKeyringAttribute union accessor methods to use g_return_if_fail instead of g_assert (sorry for the confusion: this belongs here, not to the comment for the updated 4/7 patch)
Created attachment 205350 [details] [review] [7/7] Add Python test script for GI binding Drop disabled async test, as we now disable the API.
I think I now addressed all your comments. Many thanks for the review!
Review of attachment 205349 [details] [review]: Looks good to go in with these changes. ::: library/gnome-keyring-utils.c @@ +284,3 @@ + * + * Return the string value. Return %NULL if @attribute.type is not + * #GNOME_KEYRING_ATTRIBUTE_TYPE_STRING. This method is mostly useful for In general we don't document the precondition failure results. Should be: "It is an error to call this method if @attribute.type is not #GNOME_KEYRING_ATTRIBUTE_TYPE_STRING." @@ +303,3 @@ + * + * Return the uint32 value. Return 0 if @attribute.type is not +const gchar* In general we don't document the precondition failure results. Should be: "It is an error to call this method if @attribute.type is not #GNOME_KEYRING_ATTRIBUTE_TYPE_STRING."
Review of attachment 205346 [details] [review]: Looks good to go in with the following changes. ::: library/gnome-keyring-utils.c @@ +288,3 @@ +gnome_keyring_attribute_free (GnomeKeyringAttribute *attribute) +{ + * Free the memory used by the #GnomeKeyringAttribute @attribute. xxx_free() functions should generally accept %NULL, and simply do nothing. @@ +307,3 @@ +{ + GnomeKeyringAttribute *copy; +} xxx_copy() functions should generally accept %NULL and return %NULL in that case.
I'm having some issues testing this, and I'll give it another shot tomorrow...
Getting this test failure: ====================================================================== FAIL: test_find_items (__main__.KeyringTest) find_items_sync() ---------------------------------------------------------------------- Traceback (most recent call last):
+ Trace 229467
self.assertEqual(type(attr.get_uint32()), type(0))
---------------------------------------------------------------------- Ran 6 tests in 30.948s FAILED (failures=1)
Ah, this looks like an overzealously strict test indeed. I assume you run 32 bit? For me on 64 bit with python 3.2.2 the test works. Would you mind trying if replacing self.assertEqual(type(attr.get_uint32()), type(0)) with self.assertTrue(isinstance(attr.get_uint32()), int) also works for you? That should be less picky about int vs. long.
Created attachment 205503 [details] [review] [7/7] Add Python test script for GI binding New test-gi.py version with slightly relaxed int type test, as per previous comment.
Created attachment 205504 [details] [review] [4/7] Box structs to make them introspectable Fix the _free/_copy methods to deal with NULL, as per comment 39. No other changes.
Created attachment 205505 [details] [review] [6/7] Add GnomeKeyringAttribute union accessor methods Update return value documentation of the _get_uint32/_get_string accessors as per comment 38. No other changes.
Comment on attachment 205290 [details] [review] [2/7] Build introspection typelib Fixed test-gi.py again for int->long, and committed the whole stack after discussing with Stef on IRC. Thanks for the review!