Bug 226662 – [GTK] Replace gtk-doc with gi-docgen

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

If possible, we should try to eliminate our dependency on gtk-doc and replace it with gi-docgen: https://gnome.pages.gitlab.gnome.org/gi-docgen/. This would help us avoid a lot of problems.

I have not yet investigated whether this is actually possible, but I'm pretty sure it is.

[Carlos Garcia Campos]

Yes, I think Adrian has some wip locally. We should probably stop including the generated doc in the tarballs too. WE still need a way to validate the docs while building, even if the html is not generated like we do now.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #1)
> Yes, I think Adrian has some wip locally. We should probably stop including
> the generated doc in the tarballs too. WE still need a way to validate the
> docs while building, even if the html is not generated like we do now.

Yes, I have a slightly outdated branch around — let me try and rebase it.
We have been indeed waiting on having some validation support in gi-docgen
and now that https://gitlab.gnome.org/GNOME/gi-docgen/-/issues/51 has been
implemented I believe we should be good to go =)

[Adrian Perez]

Created attachment 431318 [details]
Very WIP Patch

This is some bare minimum to get the JSC docs built with the WPE port.
Configure as usual, passing -DENABLE_DOCUMENTATION=ON to CMake. The
old flag was ENABLE_GTKDOC but this is not using gtk-doc anymore, so
renaming the flag seemed fitting. Then run "ninja JavaScriptCore-doc",
and the build will end up failing with a linker failure, something like
this:

  /usr/bin/ld: /home/aperez/WebKit/tmp-introspect_3e5lxrx/JavaScriptCore-1.0.o:(.data+0x28):
    undefined reference to `jsc_weak_value_get_type'

I have a vague memory of seeing this particular linker issue before,
it may have something to do with the patches to use -fvisibility=hidden.

At any rate, this will need factoring out the CMake bits into functions
so then we can do something like:

  WEBKIT_ADD_GI_DOCGEN(JavaScriptCore)

…in order to avoid repeating the same CMake snippets for each target.

[Adrian Perez]

Another option is taking care of the GTK port first, for which we
know that currently both introspection and documentation builds fine,
then follow-up with the needed fixes for the WPE port.

[Michael Catanzaro]

Comment on attachment 431318 [details]
Very WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431318&action=review

> Source/JavaScriptCore/PlatformWPE.cmake:72
> +            --warn-all

I recently changed GTK to use --warn-error as well, in bug #222985, more recently than you copied this.

[Michael Catanzaro]

(In reply to Adrian Perez from comment #3)
> Created attachment 431318 [details]
> Very WIP Patch
> 
> This is some bare minimum to get the JSC docs built with the WPE port.
> Configure as usual, passing -DENABLE_DOCUMENTATION=ON to CMake.

Can we make it on by default?

[Michael Catanzaro]

(In reply to Adrian Perez from comment #3)
> I have a vague memory of seeing this particular linker issue before,
> it may have something to do with the patches to use -fvisibility=hidden.

Weird. Its declaration is:

JSC_API GType
jsc_weak_value_get_type  (void);

where JSC_API expands to __attribute__((visibility("default"))). So it *should* be exported....

[Adrian Perez]

(In reply to Michael Catanzaro from comment #7)
> (In reply to Adrian Perez from comment #3)
> > I have a vague memory of seeing this particular linker issue before,
> > it may have something to do with the patches to use -fvisibility=hidden.
> 
> Weird. Its declaration is:
> 
> JSC_API GType
> jsc_weak_value_get_type  (void);
> 
> where JSC_API expands to __attribute__((visibility("default"))). So it
> *should* be exported....

Yet “nm -DC <builddir>/lib/libWPEWebKit-1.0.so.3.16.0|grep jsc_weak_” shows
nothing, so definitely there is something fishy here. As said above, I am
going to tackle using gi-docgen for the GTK port first and get back to the
missing symbols issue later 🤷‍♂️️

[Michael Catanzaro]

I think this sort of blocks GTK4 port, because all the API changes/removal bugs depend on being able to conditionally build the documentation for different API versions (WPE, GTK 3, GTK 4), and we don't want to have to figure that out twice for gtk-doc vs. gi-docgen. Adrian, do you agree?

[Adrian Perez]

(In reply to Michael Catanzaro from comment #9)
> I think this sort of blocks GTK4 port, because all the API changes/removal
> bugs depend on being able to conditionally build the documentation for
> different API versions (WPE, GTK 3, GTK 4), and we don't want to have to
> figure that out twice for gtk-doc vs. gi-docgen. Adrian, do you agree?

Yes, this makes sense. Also in the GTK chat ebassi mentioned that gi-docgen
can pick documentation just fine from files which do not have any code inside
them (just documentation comments), which means for symbols/signals/etc. which
have the same name in GTK3 and the GTK4 API, but parameters or description
should be different, we can have e.g. AdditionalDocumentationGTK{3,4}.h for
those cases, which do not need being built. Another nicety of gi-docgen is
that some documentation infos are extracted from GObject Introspection data
so what gets used by the documentation is exactly what gets built, which
should ease things a bit, too.

[Adrian Perez]

Created attachment 453880 [details]
Not so WIP patch

[Carlos Garcia Campos]

Comment on attachment 453880 [details]
Not so WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453880&action=review

It seems we are missing the installation targets and also the option to check api is documented during build.

> Source/cmake/FindGIDocgen.cmake:1
> +find_package(GObjectIntrospection REQUIRED)

This file is missing header comment with license and docs.

> Source/cmake/FindGIDocgen.cmake:9
> +find_program(GIDocgen_EXE
> +    NAMES gi-docgen gi-docgen.py
> +    HINTS "${CMAKE_SOURCE_DIR}/gi-docgen/bin"
> +          "${CMAKE_SOURCE_DIR}/gi-docgen"
> +    DOC "Path to the gi-docgen program"
> +    REQUIRED
> +)

Is gi-docgen really available in distros already? Since it can be installed with pip, I wonder if we could autoinstall it somehow from webkitpy, or import in in ThirdParty.

> Source/cmake/FindGIDocgen.cmake:75
> +function(GI_DOCGEN_ADD target)

Does this kind of public macros belong to FindFoo modules?

> Source/cmake/FindGIDocgen.cmake:118
> +            ${INTROSPECTION_SCANNER}

Do we need to generate another gir here?

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #12)
> Comment on attachment 453880 [details]
> Not so WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453880&action=review
> 
> It seems we are missing the installation targets and also the option to
> check api is documented during build.

Yes, I am working through all the missing things, removing more parts
of the existing gtk-doc infrastructure, and making sure that at least we
will have feature parity. At this very moment, “git show --stat|tail -1”
outputs:

  22 files changed, 302 insertions(+), 8432 deletions(-)

While some more CMake lines will be added for the missing features, at
this point I am already quite sure the net result of moving over to
gi-docgen will be thousands of line removed :)

As a bonus, due to introspection being needed for documentation, we will
have initial support for GI in the WPE port.

> > Source/cmake/FindGIDocgen.cmake:1
> > +find_package(GObjectIntrospection REQUIRED)
> 
> This file is missing header comment with license and docs.
> 
> > Source/cmake/FindGIDocgen.cmake:9
> > +find_program(GIDocgen_EXE
> > +    NAMES gi-docgen gi-docgen.py
> > +    HINTS "${CMAKE_SOURCE_DIR}/gi-docgen/bin"
> > +          "${CMAKE_SOURCE_DIR}/gi-docgen"
> > +    DOC "Path to the gi-docgen program"
> > +    REQUIRED
> > +)
> 
> Is gi-docgen really available in distros already? Since it can be installed
> with pip, I wonder if we could autoinstall it somehow from webkitpy, or
> import in in ThirdParty.

Some distributions are shipping packages already (Arch, Fedora), and for the
ones which do not yet have them, I wrote FindGIDocgen.cmake in a way that
the following works (from the top level source directory):

  % virtualenv gi-docgen
  % gi-docgen/bin/pip install 'gi-docgen==2022.1'

or:

  % git clone --branch=2022.1 https://gitlab.gnome.org/GNOME/gi-docgen.git

or:

  % mkdir gi-docgen
  % curl -sL \
    https://gitlab.gnome.org/GNOME/gi-docgen/-/archive/2022.1/gi-docgen-2022.1.tar.bz2 \
    | tar -xjz - --strip-components=1 -C gi-docgen

…or any other similar method. Which means distros can list the tarball as
part of the sources needed to build WebKit and arrange to place unpack it
under “gi-docgen/”, in case they do not have packages.

Therefore, I would rather not bundle gi-docgen in Source/ThirdParty/.

> > Source/cmake/FindGIDocgen.cmake:75
> > +function(GI_DOCGEN_ADD target)
> 
> Does this kind of public macros belong to FindFoo modules?

Sure, for example when using “find_package(PkgConfig)” that reads the
“FindPkgConfig.cmake” file, which both searches for “pkg-config” and
defines “pkg_check_modules()” and the rest of related functions.

> > Source/cmake/FindGIDocgen.cmake:118
> > +            ${INTROSPECTION_SCANNER}
> 
> Do we need to generate another gir here?

No, I have things to clean up still, as said :-)

[Adrian Perez]

Some notes to self:

 - Split the .gir creation and the HTML generation out from
   GI_DOCGEN_ADD(), instead have two functions, for example
   GOBJECT_INTROSPECTION_ADD(target …) and GI_DOCGEN_ADD(target …)
   and make the second pick properties set on “target” by the
   first one.

 - GOBJECT_INTROSPECTION_ADD() should be a no-op when
   ENABLE_INTROSPECTION is disabled.

 - GI_DOCGEN_ADD() should be a no-op when ENABLE_INTROSPECTION
   is disabled, and only run “gi-docgen check” when option
   ENABLE_DOCUMENTATION is disabled.

 - We want ENABLE_INTROSPECTION=ON + ENABLE_DOCUMENTATION=OFF
   for developer builds.

[Adrian Perez]

More notes to self:

 - GOBJECT_INTROSPECTION_ADD() should also add targets for generating
   the .typelib files, and setup installation of .gir and .typelib files.

 - GI_DOCGEN_ADD() should setup installation of the documentation, too.

[Adrian Perez]

Created attachment 454294 [details]
The “getting there” patch



- Building the GTK port correctly generates GI+docs.
- Untested first attempt at generating GI+docs for WPE (only the JSC part for now).
- Missing check mode when doing developer build with documentation disabled.
- Missing install targets.

In order to test before tackling bug #237681 one can do a build without using the
Flatpak SDK, or entering the SDK shell and installing it with  a virtualenv under
the gi-docgen/ subdirectory as mentioned in an earlier comment.

[Adrian Perez]

Created attachment 455242 [details]
The “only missing ChangeLogs and doc-comments in CMake modules” patch


- Both GTK and WPE ports correctly generate .gir and .typelib files
  using rules added by FindGI.cmake, and documentation using rules
  added by FindGIDocgen.cmake \o/

- Using the documentation check targets (i.e. “doc-check-all”) passes
  checks for JSC, there are a few warnings for WebKit, and a bunch of
  warnings for the WebExtension documentation.

- I did most of the grunt work for the first two to pass checks (add
  brief descriptions where missing, fix typos in symbol names, stop
  using “SECTION:Foo”, etc.) but considering that the style encouraged
  by gi-docgen is different from that of gtk-doc it would be a good
  thing to comb over the documentation in follow-up patches to do
  editorial improvements.

- While not warnings/errors, it would be still good to add some missing
  annotations, like the ones that associate properties with their setter
  and getter functions.

- Also, linking between symbols, code examples, and a few other things
  ares done a bit differently with gi-docgen, which embraces more usage
  of Markdown.

- With gi-docgen we can add Markdown documents to be integrated into
  the “Additional Documentation” area of the resulting output. For
  example in GTK4: https://docs.gtk.org/gtk4/#extra - it would be good
  to add an introduction/overview and some tutorial(s), and a document
  with the list of environment variables that affect operation. We can
  have a different set of files for each port.

All the improvements suggested above could be done in follow-up patches,
I think.

[Michael Catanzaro]

Comment on attachment 455242 [details]
The “only missing ChangeLogs and doc-comments in CMake modules” patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455242&action=review

The patch is too big to practically review everything closely, but in general things look fine. I see plenty of nice mistakes fixed throughout, though.

BTW the existing gtk-doc for webkit2gtk-5.0 is not parallel-installable with the docs for webkit2gtk-4.0. I hope your patch incidentally fixes this problem.

> Source/WebKit/PlatformWPE.cmake:486
> +# XXX: Using ${JavaScriptCore_INSTALLED_HEADERS} here expands to nothing.

What's up with this?

> Source/WebKit/UIProcess/API/gtk/WebKitDefines.h:53
> +/**
> + * WEBKIT_DEPRECATED_FOR: (skip)
> + * @f: replacement symbol name
> + *
> + * Marks a symbol as deprecated, indicating a replacement.
> + */

I'm not super fond that we have to document this just to be able to (skip) it, but if that's the best way to handle this, then OK.

> Source/cmake/FindGIDocgen.cmake:18
> +# Allow dropping a copy of the tool source under gi-docgen/, as it can run
> +# uninstalled, and also search gi-docgen/bin/ to allow placing a virtualenv
> +# in that location.
> +find_program(GIDocgen_EXE
> +    NAMES gi-docgen gi-docgen.py
> +    HINTS "${CMAKE_SOURCE_DIR}/gi-docgen/bin"
> +          "${CMAKE_SOURCE_DIR}/gi-docgen"
> +    DOC "Path to the gi-docgen program"
> +    REQUIRED
> +)

So if I understand correctly: gi-docgen can be a system dependency, OR it could be bundled in the source directory, but if so it has to be copied there manually prior to the build: it's not going to be provided by WebKit itself?

> Source/cmake/OptionsGTK.cmake:83
> +WEBKIT_OPTION_DEPEND(ENABLE_DOCUMENTATION ENABLE_INTROSPECTION)
>  WEBKIT_OPTION_DEPEND(ENABLE_3D_TRANSFORMS USE_OPENGL_OR_ES)
>  WEBKIT_OPTION_DEPEND(ENABLE_ASYNC_SCROLLING USE_OPENGL_OR_ES)

This should be alphabetized (under ENABLE_ASYNC_SCROLLING). Currently USE_ANGLE_WEBGL is the only other option out of place here.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #18)
> Comment on attachment 455242 [details]
> The “only missing ChangeLogs and doc-comments in CMake modules” patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455242&action=review
> 
> The patch is too big to practically review everything closely, but in
> general things look fine. I see plenty of nice mistakes fixed throughout,
> though.

In reality the most interesting part of the patch is in “Source/cmake/”,
then the “.toml.in” templates for the configuration are mildly interesting,
and the rest are mechanical changes — mostly fixups, as you have noticed. 
 
> BTW the existing gtk-doc for webkit2gtk-5.0 is not parallel-installable with
> the docs for webkit2gtk-4.0. I hope your patch incidentally fixes this
> problem.

Like in https://people.igalia.com/aperez/Documentation/ ?

> > Source/WebKit/PlatformWPE.cmake:486
> > +# XXX: Using ${JavaScriptCore_INSTALLED_HEADERS} here expands to nothing.
> 
> What's up with this?

No idea, usually it looks like I know how to deal with CMake, and sometimes
I trip on things which are completely unexpected to me. I suppose it may have
to do something with the variable being scoped to the Source/JavaScriptCore/
subdirectory — but apparently I have to re-read the manual and learn how the
scoping actually works in CMake 🤷️

> > Source/WebKit/UIProcess/API/gtk/WebKitDefines.h:53
> > +/**
> > + * WEBKIT_DEPRECATED_FOR: (skip)
> > + * @f: replacement symbol name
> > + *
> > + * Marks a symbol as deprecated, indicating a replacement.
> > + */
> 
> I'm not super fond that we have to document this just to be able to (skip)
> it, but if that's the best way to handle this, then OK.

I asked Emmanuelle Bassi about this, and there is no better way at the moment;
but there may be something in the future.

Guarding with __GI_DOCGEN__ does not work here because the macro is actually
used in headers that gi-docgen has to preprocess and go through (in the case
of JavaScriptCore a similar macro is not used and it can be guarded instead).

> > Source/cmake/FindGIDocgen.cmake:18
> > +# Allow dropping a copy of the tool source under gi-docgen/, as it can run
> > +# uninstalled, and also search gi-docgen/bin/ to allow placing a virtualenv
> > +# in that location.
> > +find_program(GIDocgen_EXE
> > +    NAMES gi-docgen gi-docgen.py
> > +    HINTS "${CMAKE_SOURCE_DIR}/gi-docgen/bin"
> > +          "${CMAKE_SOURCE_DIR}/gi-docgen"
> > +    DOC "Path to the gi-docgen program"
> > +    REQUIRED
> > +)
> 
> So if I understand correctly: gi-docgen can be a system dependency, OR it
> could be bundled in the source directory, but if so it has to be copied
> there manually prior to the build: it's not going to be provided by WebKit
> itself?

Correct. There is a guarantee that gi-docgen can work uninstalled, so a tarball
for it can be one of the sources for e.g. the WebKitGTK package in distro, and
placing its contents under the “gi-docgen/” subdir inside the WebKit source tree
is enough. The “HINTS” in the find_program() call above also make it possible to
use a virtualenv if needed. I wrote about this in this earlier comment:

  https://bugs.webkit.org/show_bug.cgi?id=226662#c13

I intend to write down this in the documentation for the FindGIDocgen.cmake
module :) 

> > Source/cmake/OptionsGTK.cmake:83
> > +WEBKIT_OPTION_DEPEND(ENABLE_DOCUMENTATION ENABLE_INTROSPECTION)
> >  WEBKIT_OPTION_DEPEND(ENABLE_3D_TRANSFORMS USE_OPENGL_OR_ES)
> >  WEBKIT_OPTION_DEPEND(ENABLE_ASYNC_SCROLLING USE_OPENGL_OR_ES)
> 
> This should be alphabetized (under ENABLE_ASYNC_SCROLLING). Currently
> USE_ANGLE_WEBGL is the only other option out of place here.

Sure, I can reorganize these.

[Adrian Perez]

Created attachment 456344 [details]
Patch for landing

[Adrian Perez]

Created attachment 456377 [details]
Patch for landing

[Adrian Perez]

Created attachment 456398 [details]
Patch for landing

[Adrian Perez]

Comment on attachment 456398 [details]
Patch for landing

The wk-api failures happen without this patch anyway, so let's cq+
so we can push ahead with the documentation improvements =)

[EWS]

Committed r292263 (249161@main): <https://commits.webkit.org/249161@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456398 [details].

[Adrian Perez]

*** Bug 178901 has been marked as a duplicate of this bug. ***

[Adrian Perez]

*** Bug 179916 has been marked as a duplicate of this bug. ***