GNOME Bugzilla – Bug 678846
Vala bindings
Last modified: 2012-06-28 11:39:12 UTC
Created attachment 217270 [details] [review] Add Vala bindings. Patch attached. Note the last line of the metadata file is commented out. If you uncomment this it will move the Secret.password_* functions into a Secret.Password namespace. I'm not sure whether or not you want to do that (some people love namespaces, some hate them).
Review of attachment 217270 [details] [review]: I'm wondering if there's a way to split this into two vapi's for now, one for the stable api and another for the unstable? Can two vapi's provide the same namespace? ::: library/Makefile.am @@ +129,3 @@ +VAPIGEN_VAPIS = libsecret-@SECRET_MAJOR@.vapi + +libsecret_@SECRET_MAJOR@_vapi_DEPS = gio-2.0 Should this be glib as well? ::: library/Secret-0.metadata @@ +1,1 @@ +Service Could we add a comment to the top of this file to describe what its for? Obviously it's for vala bindings, but want that to be clear to others right away. @@ +10,3 @@ +password_removev finish_name="secret_password_remove_finish" +password_storev finish_name="secret_password_store_finish" +// password_* parent="Secret.Password" name="password_(.+)" I agree with keeping everything in one namespace.
(In reply to comment #1) > Review of attachment 217270 [details] [review]: > > I'm wondering if there's a way to split this into two vapi's for now, one for > the stable api and another for the unstable? Can two vapi's provide the same > namespace? Yes, but vapis are supposed to correspond 1:1 with pkg-config files. We've been planning on adding a [Version] attribute to Vala so that you can specify when symbols appear, stabilize, and are deprecated (currently we only have a [Deprecated] attribute for the latter), so I think the right solution would be to go ahead and implement that and use it here. > ::: library/Makefile.am > @@ +129,3 @@ > +VAPIGEN_VAPIS = libsecret-@SECRET_MAJOR@.vapi > + > +libsecret_@SECRET_MAJOR@_vapi_DEPS = gio-2.0 > > Should this be glib as well? glib-2.0 and gobject-2.0 are included implicitly. > ::: library/Secret-0.metadata > @@ +1,1 @@ > +Service > > Could we add a comment to the top of this file to describe what its for? > Obviously it's for vala bindings, but want that to be clear to others right > away. Sure, that seems reasonable. > @@ +10,3 @@ > +password_removev finish_name="secret_password_remove_finish" > +password_storev finish_name="secret_password_store_finish" > +// password_* parent="Secret.Password" name="password_(.+)" > > I agree with keeping everything in one namespace. Ok. I think the API feels a bit off (Secret.password_store instead of Secret.store_password), but it's really not a big deal. Assuming you're ok with the [Version] thing I'll post an updated patch after that is done.
(In reply to comment #2) > (In reply to comment #1) > > Review of attachment 217270 [details] [review] [details]: > > > > I'm wondering if there's a way to split this into two vapi's for now, one for > > the stable api and another for the unstable? Can two vapi's provide the same > > namespace? > > Yes, but vapis are supposed to correspond 1:1 with pkg-config files. We've > been planning on adding a [Version] attribute to Vala so that you can specify > when symbols appear, stabilize, and are deprecated (currently we only have a > [Deprecated] attribute for the latter), so I think the right solution would be > to go ahead and implement that and use it here. Ok. > > @@ +10,3 @@ > > +password_removev finish_name="secret_password_remove_finish" > > +password_storev finish_name="secret_password_store_finish" > > +// password_* parent="Secret.Password" name="password_(.+)" > > > > I agree with keeping everything in one namespace. > > Ok. I think the API feels a bit off (Secret.password_store instead of > Secret.store_password), but it's really not a big deal. Yeah, I know what you mean. If vala (and other bindings) had solid documentation then I wouldn't be against renaming all the methods. However as it stands only the C API has solid documentation, and thus developers in other languages often have to guess what methods they should be using. Renaming would add salt to the wound :S > Assuming you're ok with the [Version] thing I'll post an updated patch after > that is done. Roger.
(In reply to comment #3) > Yeah, I know what you mean. If vala (and other bindings) had solid > documentation then I wouldn't be against renaming all the methods. However as > it stands only the C API has solid documentation, and thus developers in other > languages often have to guess what methods they should be using. Renaming would > add salt to the wound :S Other languages' documentation arent't great (though they are making progress), but Vala's are actually quite good, especially for bindings generated from GIRs. For example, here is Clutter's: http://valadoc.org/#!wiki=clutter-1.0/index Not sure if that justifies an changing the API, but if you want I can throw together a patch.
(In reply to comment #4) > Other languages' documentation arent't great (though they are making progress), > but Vala's are actually quite good, especially for bindings generated from > GIRs. For example, here is Clutter's: > http://valadoc.org/#!wiki=clutter-1.0/index That looks really nice. > Not sure if that justifies an changing the API, but if you want I can throw > together a patch. Yeah, torn on this one. When it comes down to it seeing "Secret.password_store() " really isn't the end of the world. It's still readable and makes sense. The other bindings (python/js) look like that too. To round out the patch, could you add the following: * A vala example section to the gtk-doc documentation, similar to how we have javascript, python, and C examples. You would just use the same layout and examples that we have for those languages, translated to vala. Would go in: docs/reference/libsecret/libsecret-examples.sgml * The basic javascript/python tests translated to vala. To do this you may have to make a local (non-installed) vapi for MockService. See: library/tests/Makefile.am library/tests/test-*-password.py
Created attachment 217355 [details] [review] Add Vala bindings Updated version. The only thing missing is the [Version] stuff.
Review of attachment 217355 [details] [review]: Thanks for adding the test and examples. Could you also add tests for looking up a password and removing a password? Similar to how javascript and python have multiple tests. I guess we could merge without the [Version] stuff. When I build this (using vala from git master) I get: test-store-vala.vala:54.3-54.46: warning: unhandled error `GLib.Error' MockService.start ("mock-service-normal.py"); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Compilation succeeded - 1 warning(s) CC test-store-vala.o test-store-vala.c: In function ‘test_store_async_ex_finish’: test-store-vala.c:221:24: error: variable ‘_data_’ set but not used [-Werror=unused-but-set-variable] test-store-vala.c: In function ‘_vala_main’: test-store-vala.c:442:2: error: passing argument 3 of ‘g_test_add_data_func’ from incompatible pointer type [-Werror] In file included from /usr/include/glib-2.0/glib.h:84:0, from test-store-vala.c:5: /usr/include/glib-2.0/glib/gtestutils.h:108:9: note: expected ‘GTestDataFunc’ but argument is of type ‘void (*)(void *)’ test-store-vala.c:443:2: error: passing argument 3 of ‘g_test_add_data_func’ from incompatible pointer type [-Werror] In file included from /usr/include/glib-2.0/glib.h:84:0, from test-store-vala.c:5: /usr/include/glib-2.0/glib/gtestutils.h:108:9: note: expected ‘GTestDataFunc’ but argument is of type ‘void (*)(void *)’ We should be able to build without warnings, no? You can build libsecret with --enable-strict to check for this stuff. You'll also want to make sure 'make distcheck' passes if you haven't already. ::: .gitignore @@ -14,3 @@ .deps .cproject .libs May want to add *.stamp-t as well, as it seems vala likes to use those files. ::: docs/reference/libsecret/libsecret-examples.sgml @@ +659,3 @@ + attributes["number"] = "8"; + attributes["string"] = "eight"; + to use the schema.</para> Out of interest. Is there no language construct in Vala that'll build a usable hash table for us? I designed the API so that languages like python and javascript could have really simple syntax storing and retrieving secrets. ::: library/tests/Makefile.am @@ +32,3 @@ +VALAFLAGS = \ + --target-glib 2.32 \ Should we define a variable in configure.ac that defines which minimum glib to use? We could use that variable in the PKG_CHECK_MODULES check in configure.ac as well as here. That way we won't forget to update this version number.
Created attachment 217474 [details] [review] Add Vala bindings. (In reply to comment #7) > Thanks for adding the test and examples. Could you also add tests for looking > up a password and removing a password? Similar to how javascript and python > have multiple tests. I added a few tests, the list is now: /vala/lookup/sync /vala/lookup/async /vala/lookup/no-name /vala/store/sync /vala/store/async /vala/remove/sync /vala/remove/async > We should be able to build without warnings, no? You can build libsecret with > --enable-strict to check for this stuff. You'll also want to make sure 'make > distcheck' passes if you haven't already. No. C warnings are pretty much a fact of life with Vala and that isn't going to change any time soon (if ever), especially if GCC keeps adding warnings about stuff which don't even come close to violating standards. I've added -w to the CFLAGS for the test written in Vala (and tested to make sure it works with --enable-strict). This patch also passes make distcheck. > ::: .gitignore > @@ -14,3 @@ > .deps > .cproject > .libs > > May want to add *.stamp-t as well, as it seems vala likes to use those files. Automake's built-in Vala support was causing problems with distcheck so I decided to ditch it, which means this is no longer an issue (it's automake that likes them, not valac). > ::: docs/reference/libsecret/libsecret-examples.sgml > @@ +659,3 @@ > + attributes["number"] = "8"; > + attributes["string"] = "eight"; > + to use the schema.</para> > > Out of interest. Is there no language construct in Vala that'll build a usable > hash table for us? I designed the API so that languages like python and > javascript could have really simple syntax storing and retrieving secrets. No, but Vala can use variadic functions. I added some examples in "Vala example: Store a password". > ::: library/tests/Makefile.am > @@ +32,3 @@ > > +VALAFLAGS = \ > + --target-glib 2.32 \ > > Should we define a variable in configure.ac that defines which minimum glib to > use? We could use that variable in the PKG_CHECK_MODULES check in configure.ac > as well as here. That way we won't forget to update this version number. No. That flag was unnecessary (I just wrote it out of habit) and I've removed it... AFAIK there is nothing in the test case which would require targeting anything more recent than valac does by default (IIRC that's currently 2.16).
Review of attachment 217474 [details] [review]: Looks really good. However I get this during distcheck, perhaps because the libsecret vapi isn't installed on my machine? /usr/bin/g-ir-compiler --includedir=../../../library/tests --includedir=. --includedir=. MockService-0.gir -o MockService-0.typelib /usr/bin/vapigen-0.18 --library mock-service-0 --metadatadir ../../../library/tests --vapidir ../../../library --pkg gio-2.0 --pkg libsecret-0 MockService-0.gir error: Package `libsecret-0' not found in specified Vala API directories or GObject-Introspection GIR directories Generation failed: 1 error(s), 0 warning(s) ::: docs/reference/libsecret/libsecret-examples.sgml @@ +632,3 @@ + attributes["even"] = Secret.SchemaAttributeType.BOOLEAN; + + are defined in a schema. The schema is usually defined once globally. Hmmm, I wonder if I should change this around to have a varargs function to create a schema, for vala's sake? The libsecret API isn't yet stable so now would be a good time to make changes. @@ +735,3 @@ + <informalexample><programlisting language="vala"><![CDATA[ + string password = Secret.password_lookup_sync (example_schema, attributes, null, + attributes["string"] = "eight"; Out of interest, does Vala automatically add the NULL varargs terminator here? @@ +777,3 @@ + attributes["even"] = "true"; + + <informalexample><programlisting language="vala"><![CDATA[ Let's do the varargs one here too.
Created attachment 217480 [details] [review] Add Vala bindings. (In reply to comment #9) > Review of attachment 217474 [details] [review]: > > Looks really good. > > However I get this during distcheck, perhaps because the libsecret vapi isn't > installed on my machine? Yeah, I think having the header installed was masking a srcdir != builddir issue. Actually, I'm surprised it got that far... I had to make a small change in order to get VPATH builds working (I'll create a separate attachment for it). > ::: docs/reference/libsecret/libsecret-examples.sgml > @@ +632,3 @@ > + attributes["even"] = Secret.SchemaAttributeType.BOOLEAN; > + > + are defined in a schema. The schema iIs usually defined once > globally. > > Hmmm, I wonder if I should change this around to have a varargs function to > create a schema, for vala's sake? The libsecret API isn't yet stable so now > would be a good time to make changes. That could be nice, and it might fit better with the rest of the API, but I guess secret_schema_new isn't used from C very much? I actually see a few places where similar changes might make sense in order to have a consistent API. Just grep in the vapi for HashTable and you'll see them. > @@ +735,3 @@ > + <informalexample><programlisting language="vala"><![CDATA[ > + string password = Secret.password_lookup_sync (example_schema, > attributes, null, > + attributes["string"] = "eight"; > > Out of interest, does Vala automatically add the NULL varargs terminator here? Yes. We can also have custom sentinels (e.g., G_TYPE_INVALID, -1, etc), or no sentinel at all. > @@ +777,3 @@ > + attributes["even"] = "true"; > + > + <informalexample><programlisting language="vala"><![CDATA[ > > Let's do the varargs one here too. Done.
Created attachment 217482 [details] [review] Fix VPATH build. To reproduce, just mkdir foo; cd foo; ../autogen.sh ...; make
(In reply to comment #10) > Created an attachment (id=217480) [details] [review] > Add Vala bindings. > > (In reply to comment #9) > > Review of attachment 217474 [details] [review] [details]: > > > > Looks really good. > > > > However I get this during distcheck, perhaps because the libsecret vapi isn't > > installed on my machine? > > Yeah, I think having the header installed was masking a srcdir != builddir > issue. Actually, I'm surprised it got that far... I had to make a small change > in order to get VPATH builds working (I'll create a separate attachment for > it). > > > ::: docs/reference/libsecret/libsecret-examples.sgml > > @@ +632,3 @@ > > + attributes["even"] = Secret.SchemaAttributeType.BOOLEAN; > > + > > + are defined in a schema. The schema iIs usually defined once > > globally. > > > > Hmmm, I wonder if I should change this around to have a varargs function to > > create a schema, for vala's sake? The libsecret API isn't yet stable so now > > would be a good time to make changes. > > That could be nice, and it might fit better with the rest of the API, but I > guess secret_schema_new isn't used from C very much? No it's meant for the bindings that can't just declare a SecretSchema struct. Actually maybe Vala can just declare a secret schema struct. Anyway, for consistency I made the change of having a varargs version of that function. But now I can't get the tests to work, I get "does not have a default constructor" for "new Secret.Schema" invocations and the vapi says has_construct_function = false. Could you fix this up to make it work against master? > I actually see a few places where similar changes might make sense in order to > have a consistent API. Just grep in the vapi for HashTable and you'll see > them. Hmmm, I think you mean the various functions on Secret.Service? I was actually thinking of dropping the varargs functions there (and perhaps having a secret_attributes_build() function which produces a hashtable from varargs). The 'password' API is meant to be the one that's easy and quick to use, and the varargs do that for C. The other APIs don't need to be quick and easy, and that's why I'm considering removing the varargs. > > Out of interest, does Vala automatically add the NULL varargs terminator here? > > Yes. We can also have custom sentinels (e.g., G_TYPE_INVALID, -1, etc), or no > sentinel at all. Cool.
Created attachment 217490 [details] [review] Add Vala bindings. > > > Hmmm, I wonder if I should change this around to have a varargs function to > > > create a schema, for vala's sake? The libsecret API isn't yet stable so now > > > would be a good time to make changes. > > > > That could be nice, and it might fit better with the rest of the API, but I > > guess secret_schema_new isn't used from C very much? > > No it's meant for the bindings that can't just declare a SecretSchema struct. > Actually maybe Vala can just declare a secret schema struct. It could if we treated it as a struct, but it's currently a compact class since it is registered as a boxed type. It's possible to override this in metadata, but compact classes tend to be a bit easier to use from Vala in general, so I'd prefer to leave it as-is. > Anyway, for consistency I made the change of having a varargs version of that > function. > > But now I can't get the tests to work, I get "does not have a default > constructor" for "new Secret.Schema" invocations and the vapi says > has_construct_function = false. Could you fix this up to make it work against > master? Sure, updated version is attached. All that was necessary was adding "Schema.new skip=false" in metadata, and I've updated the tests and the documentation. > > I actually see a few places where similar changes might make sense in order to > > have a consistent API. Just grep in the vapi for HashTable and you'll see > > them. > > Hmmm, I think you mean the various functions on Secret.Service? I was actually > thinking of dropping the varargs functions there (and perhaps having a > secret_attributes_build() function which produces a hashtable from varargs). Yes, but there are also some in Secret.Item. I was thinking about the following function families: Item.create Item.set_attributes Service.create_collection_path Service.create_item_path Service.search Service.search_for_paths > The 'password' API is meant to be the one that's easy and quick to use, and the > varargs do that for C. The other APIs don't need to be quick and easy, and > that's why I'm considering removing the varargs. That seems reasonable. I just don't like the inconsistency... with the current API, you can't really be sure whether something is going to be variadic or take a hash table. IMHO it would be good to either get rid of them entirely or add them everywhere (I would prefer the latter, but only slightly).