GNOME Bugzilla – Bug 740814
"make check" should ensure that every symbol is documented
Last modified: 2018-05-24 17:17:12 UTC
It is really common to forget to add new symbols in the doc, I think "make check" should fail if we don't have 100% doc coverage. telepathy-glib does that.
Created attachment 291671 [details] [review] Doc: Verify at "make check" that all symbols are documented
The above patch does it for glib. The same work should be done for gobject and gio.
Created attachment 291673 [details] [review] Doc: Verify at "make check" that all symbols are documented
Review of attachment 291673 [details] [review]: hi. Thanks for undertaking this work but there are a few problems here. The first is that the patches to fix the docs ought to be split out. The second is that this feature should not be implemented inside of GLib but rather inside of gtk-doc as something that be switched on by modules that are interested in using it. The third problem is that I'm not even sure that we can do that. The list of symbols that end up being found by gtk-doc tends to change with the options and platform that we build GLib with. I fear that we may end up in a situation where we permanently break 'make check' on every platform other than Linux...
(In reply to comment #4) > Review of attachment 291673 [details] [review]: > The first is that the patches to fix the docs ought to be split out. Right, I can do that. > The second is that this feature should not be implemented inside of GLib but > rather inside of gtk-doc as something that be switched on by modules that are > interested in using it. Right, can be moved there but I never looked at gtkdoc code. The advantage of doing it like this is that it's a trivial copy/paste from tp-glib. Can be moved to gtkdoc later if someone cares, or do you think that's merge blocker? > The third problem is that I'm not even sure that we can do that. The list of > symbols that end up being found by gtk-doc tends to change with the options and > platform that we build GLib with. I fear that we may end up in a situation > where we permanently break 'make check' on every platform other than Linux... We could enable that check only for linux builds if it turns out it breaks everything else (it probably does tbh). Also in tp-glib that check is enabled only when building dev version (version x.y.z.1), not release tarballs (version x.y.z). I don't know if glib has a way in its build system to detect that? The point is that a few symbols are missing in the doc, I realized that when looking for g_output_stream_write_all_async() and friends. That mistake will repeat forever unless we make the build fail if doc is missing.
btw, I already made libgobject pass the test, and I'm working on libgio atm. I hope to finish it this weekend.
(In reply to comment #5) > The point is that a few symbols are missing in the doc, I realized that when > looking for g_output_stream_write_all_async() and friends. That mistake will > repeat forever unless we make the build fail if doc is missing. I completely agree that this is worth fixing. I think your proposal to enable it only on Linux builds is a good pragmatic solution.
Reported bug #740875 with patch against gtk-doc.
My opinion: this approach does not help really help. Unless you force patch committers to run make, all this does is make it more likely that I release a tarball that is generated with make dist instead of distcheck.
Created attachment 291775 [details] [review] Doc: glib: Fix all undocumented/unused/undeclared symbols
Created attachment 291776 [details] [review] Doc: gobject: Fix all undocumented/unused/undeclared symbols
Created attachment 291777 [details] [review] Doc: gio: Fix all undocumented/unused/undeclared symbols
Created attachment 291778 [details] [review] Doc: enable gtk-doc checks on linux This fail "make check" if any symbol is missing from the doc. This will ensure we stay at 100% coverage.
(In reply to comment #9) > My opinion: this approach does not help really help. Unless you force patch > committers to run make, all this does is make it more likely that I release a > tarball that is generated with make dist instead of distcheck. Forcing committers to run make check looks like a sane idea.
One thing I noticed is that doc for GTypeValueTable is totally broken since the change to use markdown. The problem is you cannot have empty line on item doc otherwise gtkdoc think that starts the enum description. But you cannot use markdown list without an empty line to end it.
oh, also notice that I moved G_PARAM_READWRITE out of the enum and made it a macro, because gtkdoc didn't like having such value in the enum and that symbol never appeared in the doc. I believe this is not API/ABI break, right?
(In reply to comment #16) > oh, also notice that I moved G_PARAM_READWRITE out of the enum and made it a > macro, because gtkdoc didn't like having such value in the enum and that symbol > never appeared in the doc. then gtk-doc needs to be fixed, because I moved the symbol in the enumeration for a reason — see bug #726037. I'm pretty sure gtk-doc survived when I built the documentation after fixing that bug, but you may have an older version.
Please double check, but I think you broke the doc and didn't notice precisely because we don't fail "make check" when symbols are not documented. At least here G_PARAM_READWRITE is undocumented with gtkdoc master. Note that I could also say fix introspection scanner about bug #726037. A workaround would be to give the numerical value and tell in a comment that it's READ|WRITE, like I had to do in GTokenType.
Hm, or maybe jhbuild is playing tricks to me and I'm not using gtkdoc master? I'm not 100% sure :/
Created attachment 292324 [details] [review] Doc: glib: Fix all undocumented/unused/undeclared symbols
Created attachment 292325 [details] [review] Doc: gobject: Fix all undocumented/unused/undeclared symbols
Created attachment 292326 [details] [review] Doc: gio: Fix all undocumented/unused/undeclared symbols
Created attachment 292327 [details] [review] Doc: enable gtk-doc checks on linux This fail "make check" if any symbol is missing from the doc. This will ensure we stay at 100% coverage.
@Emmanuele: you were right, reverted my change for G_PARAM_READWRITE, the problem was more trivial: you forgot to remove it from the sections.txt :-) Rebased my patches on top of master, where GNetworkConnectivity and friends got added and doc was missing (proofing my point...)
Review of attachment 292324 [details] [review]: Looks like a lot of good improvements, but please see a few outstanding issues below. ::: glib/ghash.c @@ +1309,3 @@ * Checks if @key is in @hash_table. * + * Returns: %TRUE of @key is in @hash_table, %FALSE otherwise. s/of/if/ Also: please an extra line between Returns: and Since: ::: glib/gkeyfile.c @@ +397,3 @@ + * + * A key under #G_KEY_FILE_DESKTOP_GROUP, whose value is a boolean set to true + * if the application is dbus activatable. we typically call it like "D-Bus" in our docs. @@ +406,3 @@ + * + * A key under #G_KEY_FILE_DESKTOP_GROUP, whose value is a string list + * giving the available application #GAction. just say "application actions" ::: glib/gscanner.h @@ +80,3 @@ G_TOKEN_RIGHT_PAREN = ')', + G_TOKEN_LEFT_CURLY = 0x7b, /* gtk-doc doesn't like '{' */ + G_TOKEN_RIGHT_CURLY = 0x7d, /* gtk-doc doesn't like '}' */ No. gtk-doc needs to be fixed. ::: glib/gstdio.c @@ +849,1 @@ * Since: 2.36 Extra empty line please. ::: glib/valgrind.h @@ +73,3 @@ #define __VALGRIND_H +#ifndef __GTK_DOC_IGNORE__ I'd prefer if we could avoid adding things like this to this (imported) file. Can we just put it on IGNORE_HFILES instead?
Review of attachment 292325 [details] [review]: ::: docs/reference/gobject/gobject-sections.txt @@ -885,3 @@ -<SUBSECTION> -g_cclosure_marshal_VOID__VOID -g_cclosure_marshal_VOID__BOOLEAN Why do you feel that it's appropriate to delete these? Is it just to avoid writing docstrings for each one? Admittedly that sounds painful... I often do a quick check in devhelp to see which of these are available... ::: gobject/gtype.h @@ +415,3 @@ + gdouble v_double; + gpointer v_pointer; +}; Why are you moving around code in a docs patch?
(In reply to comment #25) > ::: glib/gscanner.h > @@ +80,3 @@ > G_TOKEN_RIGHT_PAREN = ')', > + G_TOKEN_LEFT_CURLY = 0x7b, /* gtk-doc doesn't like '{' */ > + G_TOKEN_RIGHT_CURLY = 0x7d, /* gtk-doc doesn't like '}' */ > > No. gtk-doc needs to be fixed. https://bugzilla.gnome.org/show_bug.cgi?id=741305
(In reply to comment #26) > Review of attachment 292325 [details] [review]: > > ::: docs/reference/gobject/gobject-sections.txt > @@ -885,3 @@ > -<SUBSECTION> > -g_cclosure_marshal_VOID__VOID > -g_cclosure_marshal_VOID__BOOLEAN > > Why do you feel that it's appropriate to delete these? Is it just to avoid > writing docstrings for each one? Admittedly that sounds painful... > > I often do a quick check in devhelp to see which of these are available... Was too lazy to write the doc for all of them, since they should be replaced with generic marshaller anyway. But I wrote the doc now. > ::: gobject/gtype.h > @@ +415,3 @@ > + gdouble v_double; > + gpointer v_pointer; > +}; > > Why are you moving around code in a docs patch? For some reason gtk-doc does not find it in gvaluecollector.h, if you check current doc you'll see that all members are missing. I think that's because it should be in same module than the typedef.
Created attachment 292450 [details] [review] fixup! Doc: glib: Fix all undocumented/unused/undeclared symbols
Created attachment 292451 [details] [review] fixup! Doc: gobject: Fix all undocumented/unused/undeclared symbols
(In reply to comment #28) > Was too lazy to write the doc for all of them, since they should be replaced > with generic marshaller anyway. But I wrote the doc now. > If that was the case, they would be deprecated in favor of the generic marshaller. But sadly, the ffi-based marshaller is slower, which is why these are still around and undeprecated.
Attachment 292324 [details] pushed as 1a2a689 - Doc: glib: Fix all undocumented/unused/undeclared symbols
Created attachment 292620 [details] [review] Doc: enable gtkdoc-check on linux This fail "make check" if any symbol is missing from the doc. This will ensure we stay at 100% coverage.
Review of attachment 292327 [details] [review]: No need, gtk-doc already had his way of checking the doc.
Created attachment 292624 [details] [review] Doc: gobject: Fix all undocumented/unused/undeclared symbols
Created attachment 292765 [details] [review] Doc: gio: Fix all undocumented/unused/undeclared symbols
Created attachment 292766 [details] [review] configure.ac: Add ENABLE_GTK_DOC_CHECK conditional
Created attachment 292767 [details] [review] Doc: gobject: enable gtkdoc-check
Created attachment 292768 [details] [review] Doc: glib: enable gtkdoc-check
Created attachment 292769 [details] [review] Doc: gio: enable gtkdoc-check
Not re-attaching patches again, did a few small changes, branch is there: http://cgit.collabora.com/git/user/xclaesse/glib.git/log/?h=doc
Okay -- I've applied a lot of these patches with some modifications. I didn't apply any of the ones for enabling the checks by default. Despite what I said earlier, I've slowly become vaguely unimpressed by the Linux-only approach. I think I'd really prefer to just have file containing a list of symbols that we know to be problematic. In terms of the "big patches" all that's left is the biggest of all (the GIO one) plus one small part of the GObject patch that I didn't apply: what's the deal with the moving of the declaration of GTypeCValue? That seems suspicious and is not explained at all.
And now most of the GIO changes are pushed as well. I didn't include a couple of changes. See below. Adding @g_iface everywhere seems like we're working around a bug of some kind. We should probably fix that. Likewise the shuffling of < private > and < public > in various vtables. Why did you ignore gnetworking.h?
(In reply to comment #42) > Okay -- I've applied a lot of these patches with some modifications. Thanks :D > I didn't apply any of the ones for enabling the checks by default. Despite > what I said earlier, I've slowly become vaguely unimpressed by the Linux-only > approach. I think I'd really prefer to just have file containing a list of > symbols that we know to be problematic. I would be happy to enable on other platforms as well, it's just that can't test if it pass. But people doing windows builds should work on making it pass there as well so we can enable the check for them. IMO the linux-only approach is temporary, and I personally don't care fixing doc for other platforms than linux. I was tempted by a blacklist file as well, but in the end it turns out that there are really few problematic symbols, and those case should be fixed in gtk-doc itself anyway. I prefer working on having no problematic symbols than working on a blacklist system. > In terms of the "big patches" all that's left is the biggest of all (the GIO > one) plus one small part of the GObject patch that I didn't apply: what's the > deal with the moving of the declaration of GTypeCValue? That seems suspicious > and is not explained at all. For some obscure reason gtk-doc is unhappy to have that typedef away from its doc block. I agree that case should be investigated in gtkdoc itself.
(In reply to comment #43) > And now most of the GIO changes are pushed as well. \o/ > Adding @g_iface everywhere seems like we're working around a bug of some kind. > We should probably fix that. > > Likewise the shuffling of < private > and < public > in various vtables. Would be cool to special case them in gtk-doc indeed. > Why did you ignore gnetworking.h? Because it #define symbols that are probably considered private (no idea what all that does tbh) so that was the easy way out. On 2nd though I could add them in a <SUBSECTION Private> in sections.txt.
(In reply to comment #45) > > Adding @g_iface everywhere seems like we're working around a bug of some kind. > > We should probably fix that. > > > > Likewise the shuffling of < private > and < public > in various vtables. > > Would be cool to special case them in gtk-doc indeed. Adding this to gtk-doc fix it: # Remove g_iface, parent_instance and parent_class if they are first member $declaration =~ s/(\{)\s*(\w)+\s+(g_iface|parent_instance|parent_class)\s*;/$1/g; But then we have to remove all the @parent_instance args from our doc. Does that sounds good to you?
reported bug #744061
Adopting the wip/$username/$branch convention from now on, I've pushed my work in git.gnome.org instead of collabora server. - wip/xclaesse/doc-part1: Rebased on top of what you already merged in master. It requires gtk-doc's wip/xclaesse/fixes-for-glib. The only pending issue is GTypeCValue. - wip/xclaesse/doc-part2: Enable gtkdoc-check on linux.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/963.