GNOME Bugzilla – Bug 759714
Fix annotations of EDataServer and Camel
Last modified: 2016-01-18 10:40:09 UTC
Here is a subset of patches that fix a lot of missing or wrong annotations. For an easier review, a branch is set here: https://github.com/tintou/evolution-data-server/commits/master
Created attachment 317714 [details] [review] Fixed EDataServer - Vala
Created attachment 317715 [details] [review] Fixed EDataServer - Annotations
Created attachment 317716 [details] [review] Fixed Camel - Vala
Created attachment 317717 [details] [review] Fixed Camel - Annotations
Review of attachment 317717 [details] [review]: Thanks for a bug report and patches. I'll commit it as a one large commit, but before that, I've questions: ::: camel/camel-filter-driver.h @@ +51,2 @@ struct _CamelSession; +typedef struct _CamelSession CamelSession; This is usually not the way this is done in C for "external" types (forward declarations). You do not use typedef for them. Is this change really necessary? ::: camel/camel-folder.c @@ +2090,3 @@ * camel_folder_free_summary: * @folder: a #CamelFolder + * @summary: (element-type CamelMessageInfo): the summary array to free I do not agree with the rename of the 'array' parameter of camel_folder_free_summary() to 'summary'. It's confusing with respect of the CamelFolder's summary, which is an object. Thus better to keep it an 'array'. ::: camel/camel-provider.c @@ +331,3 @@ * in the list are owned by Camel and should not be modified or freed. * + * Returns: (element-type CamelProviderModule) (transfer container): a #GList of #CamelProvider structs This change is not correct, the returned type of the element is not CamelProviderModule, but CamelProvider, as the comment says. ::: camel/camel.c @@ +348,2 @@ /** + * camel_binding_bind_property_with_closures: (rename-to camel_binding_bind_property_full) _with_closures() is not the same as _full(), why is there the rename-to then? Similar with the counterparts in e-data-server-util.c
Review of attachment 317715 [details] [review]: ::: libedataserver/e-data-server-util.c @@ +1423,2 @@ /** + * e_binding_bind_property_with_closures: (rename-to e_binding_bind_property_full) as mentioned earlier, the rename-to doesn't look right ::: libedataserver/e-data-server-util.h @@ +38,3 @@ struct _ESourceRegistry; +typedef struct _ESourceRegistry ESourceRegistry; +typedef struct _ESource ESource; as mentioned in the Camel patch, this doesn't look right. You do not typedef forward declarations in C.
The patches apply cleanly on the git master of the evolution-data-server, at commit ac3cbb61ff, but I get a build break: >Camel-1.2.gir:2263.7-2263.24: error: unknown child element `record' in `record' > <record name="sign" c:type="sign"> > ^^^^^^^^^^^^^^^^^^ >Camel-1.2.gir:2274.7-2274.36: error: unknown child element `record' in `record' > <record name="encrypt" c:type="encrypt"> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >EDataServer-1.2.metadata:1.17-3.13: warning: argument never used >Camel-1.2.gir:20163.7-20163.27: warning: Virtual method `Camel.Object.state_read' conflicts with method of the same name > <virtual-method name="state_read"> > ^^^^^^^^^^^^^^^^^^^^^ >Camel-1.2.gir:20176.7-20176.29: warning: Virtual method `Camel.Object.state_write' conflicts with method of the same name > <virtual-method name="state_write"> > ^^^^^^^^^^^^^^^^^^^^^^^ >Camel-1.2.gir:20984.7-20984.84: warning: Signal `Camel.Operation.pop_message' conflicts with method of the same name > <function name="pop_message" c:identifier="camel_operation_pop_message"> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >Camel-1.2.gir:21003.7-21003.72: warning: Signal `Camel.Operation.progress' conflicts with method of the same name > <function name="progress" c:identifier="camel_operation_progress"> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >make[2]: *** [libcamel-1.2.vapi] Error 1 >make[2]: *** Waiting for unfinished jobs.... >make[1]: *** [all-recursive] Error 1 >make: *** [all] Error 2
Thanks for the review (the patch are quite long…) Here is a new version of the patch reverting the unnecessary changes with the typedefs, fixes the vapi generation error and warnings. I kept the rename-to because I wanted to be consistent with the GLib function which these functions are based on (here is the justification for the rename: https://git.gnome.org/browse/glib/tree/gobject/gbinding.c#n1122 mostly done because it's a bindings-friendly version of the full one)
Created attachment 318913 [details] [review] Fixed Introspection
(In reply to Corentin Noël from comment #8) > I kept the rename-to because I wanted to be consistent with the GLib > function which these functions are based on (here is the justification for > the rename: https://git.gnome.org/browse/glib/tree/gobject/gbinding.c#n1122 > mostly done because it's a bindings-friendly version of the full one) Thanks for the update. The rename-to looks wrong to me, because the g_object_bind_property_with_closures() does some preprocessing on the transform_to and transform_from closures, it's not a plain replacement for g_object_bind_property_full(), but I'm not going to argue about that. Otherwise the patch doesn't cause a build break here, thus I'm committing it to the sources. There's one new warning during the compilation: > VAPIG ../camel/Camel-1.2.gir > > ** (vapigen:32717): CRITICAL **: vala_symbol_get_name: assertion 'self != NULL' failed I didn't notice it before at least. I'd appreciate if you could look on it, as a follow up work of this. Created commit 6fc1581 in eds master (3.19.4+)
This patch breaks the build on Continuous. I've tagged e-d-s: https://git.gnome.org/browse/gnome-continuous/commit/?id=d72d64d820a8a6f7c9cf6487142aa23ae6c2ff38
(In reply to Emmanuele Bassi (:ebassi) from comment #11) > This patch breaks the build on Continuous. Do you have a link for the exact error, please? I do not use that ancient tools, thus I do not understand why it would break for you and not for me (it did earlier for me).
The build breaks because of the new vapi generation rule: Soup-2.4.gir:1314.7-1314.21: warning: Virtual method `Soup.AuthDomain.accepts' conflicts with method of the same name <virtual-method name="accepts"> ^^^^^^^^^^^^^^^ Soup-2.4.gir:1330.7-1330.25: warning: Virtual method `Soup.AuthDomain.challenge' conflicts with method of the same name <virtual-method name="challenge"> ^^^^^^^^^^^^^^^^^^^ EDataServer-1.2.metadata:1.17-3.13: warning: argument never used EDataServer-1.2.metadata:26.26-27.8: error: The symbol `Posix' could not be found EDataServer-1.2.metadata:28.15-29.25: error: The symbol `Posix' could not be found EDataServer-1.2.metadata:27.13-28.10: error: The symbol `Posix' could not be found EDataServer-1.2.metadata:29.35-30.16: error: The symbol `Posix' could not be found EDataServer-1.2.metadata:30.26-31.0: error: The symbol `Posix' could not be found EDataServer-1.2.metadata:20.24-21.24: error: The symbol `Posix' could not be found EDataServer-1.2.metadata:21.33-22.27: error: The symbol `Posix' could not be found EDataServer-1.2.metadata:22.36-23.18: error: The symbol `Posix' could not be found EDataServer-1.2.metadata:23.27-24.15: error: The symbol `Posix' could not be found EDataServer-1.2.metadata:24.24-25.13: error: The symbol `Posix' could not be found EDataServer-1.2.metadata:25.18-26.21: error: The symbol `Posix' could not be found Generation failed: 11 error(s), 3 warning(s) Makefile:771: recipe for target 'libedataserver-1.2.vapi' failed
Created attachment 319053 [details] [review] Fix Continuous
Here is a patch that should fix continuous and fixes some other missing parameters
I also have reported the vapigen critical error (Bug #760624), a patch is waiting for review there.
(In reply to Corentin Noël from comment #15) > Here is a patch that should fix continuous and fixes some other missing > parameters Hmm, it's a fix for vala, not for the evolution-data-server. It was enough to have this part of yours bug #760624.
Comment on attachment 319053 [details] [review] Fix Continuous I'm marking the patch as obsolete, because it doesn't belong to the evolution-data-server.
Created attachment 319111 [details] [review] Fixed Vapigen error with missing libraries
Sorry, I attached the wrong patch… Here is the one that applies to E-D-S
Created attachment 319113 [details] [review] Fixed Vapigen error with missing libraries Here is the right one…
I see, that explains it. Created commit 09ac7d6 in eds master (3.19.4+)
Still fails in Continuous after untagging. I was able to reproduce it by using builddir != srcdir.
Simple way to reproduce what Continuous does: $ cd evolution-data-server $ NOCONFIGURE=1 ./autogen.sh $ mkdir _build && cd _build $ ../configure --enable-vala-bindings $ make You'll get the failure of comment 13.
Thanks for giving a way to reproduce the break, here is a patch.
Created attachment 319131 [details] [review] Propagate changes to other vapigen calls
Review of attachment 319131 [details] [review]: Yep, can confirm that this fixes the issue. You don't even need to add libsoup: just adding --pkg posix makes the build succeed.
Thanks for the patch, the testing and the steps for a reproducer (I always forget that you compile out of the source). I committed it before today's release. Created commit 96b98610eb7 in eds master (3.19.4+)