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.
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.
(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 =)
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.
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.
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.
(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?
(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....
(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 🤷♂️️
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?
(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.
Created attachment 453880 [details] Not so WIP patch
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?
(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 :-)
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.
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.
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.
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.
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.
(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.
Created attachment 456344 [details] Patch for landing
Created attachment 456377 [details] Patch for landing
Created attachment 456398 [details] Patch for landing
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 =)
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].
*** Bug 178901 has been marked as a duplicate of this bug. ***