GNOME Bugzilla – Bug 731406
build setting property docs from gtk-doc docs, not GParamSpec docs
Last modified: 2014-06-24 19:06:45 UTC
Currently every property in NMSetting* is documented twice, once in a gtk-doc comment, and a second time in the GParamSpec definition. This creates extra work, and also, in some places the docs have gotten out of sync. danw/setting-docs fixes this by rewriting generate-setting-specs in python and having it read the gtk-doc docs out of the GIR file. But before doing that, we have to sync up the two versions of the docs, etc. It also has some cleverness so that, eg, if you say "%NM_SETTING_DCB_FLAG_ENABLE" in the gtk-doc, it translates it to "NM_SETTING_DCB_FLAG_ENABLE (0x1)" in nm-settings.5, to try to make up for some of the places where the API docs and settings docs were intentionally different. It is probably not the world's best python code. danw/setting-docs-full has an unsquashed version of the same commits, and so should be easier to review (since it splits the trivial changes out into their own commits). Also, on that branch, the GParamSpec docs are updated at each step as well, and at the point where generate-setting-specs.py is added, it outputs exactly the same thing as the old generate-setting-specs would have at that point (modulo a handful of bugs in the old program, like missing newlines in the output, and the fact that it thinks the DCB flags properties are password flags). So, you can see how the new output differs from the old by looking just at what changes in the paramspec docs on that branch. (In places where the gtk-doc changes but the paramspec doc doesn't, that means that the gtk-doc is being synchronized to the existing paramspec doc text, and vice versa for places where the paramspec doc changes but the gtk-doc doesn't.)
Just for the record, repeating a discussion from IRC... Doing this saves ~50K of space in libnm-util (482128 -> 432976), which is a huge win on memory footprint, both on-disk and runtime. However, nmcli uses the PSpec blurbs for its inline help, so we'll need to somehow convert the CLI over to not doing this. I think it's worthwhile to allow clients to opt-in to the extra memory usage, so that eg nmcli would clearly use it, but other apps that don't care about it wouldn't incur the hit. We should also remove the blurbs/nicks from libnm-glib, and save some space there.
> all: remove remaining GParamSpec name/blurb strings Was there a reason to pass "" for the nick and blurb instead of NULL? glib is NULL-safe for these AFAICT.
Other code might not be NULL-safe. eg, nmcli previously assumed that all NMSetting properties had a non-NULL blurb.
with G_PARAM_STATIC_STRINGS, "" doesn't use any more memory than NULL anyway
The general approach and actual changes look fine to me. Maybe squash everything and then lets take one final look?
squashed and re-pushed. (The first three and last two patches are unchanged. The commits from "libnm-util: build nm-setting-docs.xml from gtk-doc and GParamSpecs" to "cli: generate setting docs from libnm-util/nm-setting-docs.xml" are reorganized versions of what was there before.) Also fixed up "make distcheck". While doing that though, I realized that (a) distcheck now needs to have a hard requirement on --enable-introspection, just like it does on --enable-gtk-doc; and (b) nmcli now only has help strings if you build with introspection... Thoughts?
Note that it's re-pushed as just "setting-docs" and not 'danw/setting-docs"... WRT introspection, I think that's OK. If somebody is generating tarballs they do need to have all that stuff enabled otherwise the tarball isn't reproducible. For nmcli + introspection, I'm cool with how it is now, but our escape-hatch would be to pre-generate the .c files and ship them with the tarball, but overwrite them if re-building locally. Maybe? Better than broken/nothing I guess. FWIW, +1 on 'setting-docs' from me, I re-reviewed all the commits and tried to skim all the property updates but I might have missed something. But if the conversion was automated I'd expect it to be OK...
(In reply to comment #7) > Note that it's re-pushed as just "setting-docs" and not 'danw/setting-docs"... oops, yes > For nmcli + introspection, I'm cool with how it is now, but our escape-hatch > would be to pre-generate the .c files and ship them with the tarball Ah, yes, it already does that actually. So it's only "people building from git without introspection" who will lose. Though I need to fix it to not assume setting-docs.c will be there unconditionally... > FWIW, +1 on 'setting-docs' from me, I re-reviewed all the commits and tried to > skim all the property updates but I might have missed something. But if the > conversion was automated I'd expect it to be OK... The conversion was mostly automated other than the two commits labeled [NON-TRIVIAL] in danw/setting-docs-full.
pushed after testing "make distcheck" and a bunch of other edge cases (distcheck didn't catch the "srcdir != destdir and --disable-introspection" case :)
(For the record) merged as: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=9a06f1a2eb62a27794d7ffba9e0cde3d0f68bd41