Bug 139369 – [GTK] gtkdoc does not appear in DevHelp

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Since the switch to cmake, our gtkdoc no longer appears in devhelp.

[Michael Catanzaro]

Created attachment 242755 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 242755 [details]
Patch

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

> Source/WebCore/PlatformGTK.cmake:793
>      "main_sgml_file=webkitdomgtk-docs.sgml\n"

What happens with the -section.txt file and the .types, etc? do they depend on the sgml file name?

> Source/WebKit2/PlatformGTK.cmake:886
>      "main_sgml_file=webkit2gtk-docs.sgml\n"

Ditto.

> Tools/gtk/manifest.txt:105
> -directory $build/Documentation/webkit2gtk/html Documentation/webkit2gtk/html
> -directory $build/Documentation/webkitdomgtk/html Documentation/webkitdomgtk/html
>  directory $build/Documentation Documentation

I don't think this is correct, we don't want to include any file outside html in the tarball.

[Michael Catanzaro]

(In reply to comment #2)
> > Source/WebCore/PlatformGTK.cmake:793
> >      "main_sgml_file=webkitdomgtk-docs.sgml\n"
> 
> What happens with the -section.txt file and the .types, etc? do they depend
> on the sgml file name?

They're still generated with the original filename (webkit2gtk-sections.txt, webkit2gtk.types), which works fine.
 
> > Tools/gtk/manifest.txt:105
> > -directory $build/Documentation/webkit2gtk/html Documentation/webkit2gtk/html
> > -directory $build/Documentation/webkitdomgtk/html Documentation/webkitdomgtk/html
> >  directory $build/Documentation Documentation
> 
> I don't think this is correct, we don't want to include any file outside
> html in the tarball.

Yes, there is tons of stuff outside of html; I'm not sure how I failed to notice that yesterday. I think we can avoid hardcoding the API version there by using a CMake template file for the manifest.

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #2)
> > > Source/WebCore/PlatformGTK.cmake:793
> > >      "main_sgml_file=webkitdomgtk-docs.sgml\n"
> > 
> > What happens with the -section.txt file and the .types, etc? do they depend
> > on the sgml file name?
> 
> They're still generated with the original filename (webkit2gtk-sections.txt,
> webkit2gtk.types), which works fine.
>  
> > > Tools/gtk/manifest.txt:105
> > > -directory $build/Documentation/webkit2gtk/html Documentation/webkit2gtk/html
> > > -directory $build/Documentation/webkitdomgtk/html Documentation/webkitdomgtk/html
> > >  directory $build/Documentation Documentation
> > 
> > I don't think this is correct, we don't want to include any file outside
> > html in the tarball.
> 
> Yes, there is tons of stuff outside of html; I'm not sure how I failed to
> notice that yesterday. I think we can avoid hardcoding the API version there
> by using a CMake template file for the manifest.

I don't think there's any problem leaving the api version

[Michael Catanzaro]

Created attachment 242798 [details]
Patch

[Michael Catanzaro]

Created attachment 244220 [details]
Patch

[Michael Catanzaro]

Created attachment 244928 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 244928 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 244928 [details]
Patch

Rejecting attachment 244928 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 244928, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/6034828709330944

[Carlos Garcia Campos]

Comment on attachment 244928 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +

Ok, the Reviewed by line is missing here, I'll land this manually.

[Carlos Garcia Campos]

Comment on attachment 244928 [details]
Patch

hmm, I've tried it locally and I'm getting a lot of gtk-doc warnings and some things are broken like the index of deprecated symbols in the dom bindings HTML docs.

[Michael Catanzaro]

(In reply to comment #11)
> Comment on attachment 244928 [details]
> Patch
> 
> hmm, I've tried it locally and I'm getting a lot of gtk-doc warnings 

If you still get the warnings after deleting the Documentation and CMakeFiles directories in your build directory, please post them and I'll take a look.

> and
> some things are broken like the index of deprecated symbols in the dom
> bindings HTML docs.

This is broken for me too, so r- indeed; I will investigate.

[Michael Catanzaro]

(In reply to comment #12)
> If you still get the warnings after deleting the Documentation and
> CMakeFiles directories in your build directory, please post them and I'll
> take a look.

Nevermind, I see them. I think all the broken link warnings were there before, but the missing field description warnings are new.

[Carlos Garcia Campos]

I saw a lot of warnings with the patch applied and I don't see them now that I removed it.

[Michael Catanzaro]

(In reply to comment #14)
> I saw a lot of warnings with the patch applied and I don't see them now that
> I removed it.

Most of my broken link warnings were caused because I've been building our dependencies with --disable-gtkdoc, since GNOME jhbuild adds that flag by default. This obscured many errors that the patch did introduce. Sorry about that.

[Michael Catanzaro]

Created attachment 245011 [details]
Patch

A different approach, avoids the issues with the previous patch. The huge diffs are just renamed files.

[Carlos Garcia Campos]

Comment on attachment 245011 [details]
Patch

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

Could you explain what was wrong and why we need to rename all the files?

> Source/WebCore/ChangeLog:9
> +        * bindings/gobject/webkitdom-4.0.symbols: Renamed from Source/WebCore/bindings/gobject/webkitdom.symbols.

I don't think we need to rename this file. This will break the script to detect DOM bindings API breaks, and the DOM bindings generator itself.

> Source/WebKit2/ChangeLog:9
> +        * UIProcess/API/gtk/docs/webkit2gtk-4.0-docs.sgml: Renamed from Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-docs.sgml.

Why do we need to rename the docs sgml file? doesn't the gtk-doc main-sgml-file command line option work? is it a bug in gtk-doc?

> Source/WebKit2/ChangeLog:11
> +        * UIProcess/API/gtk/docs/webkit2gtk-4.0-sections.txt: Renamed from Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt.
> +        * UIProcess/API/gtk/docs/webkit2gtk-4.0.types: Renamed from Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk.types.

I'm fine with renaming these if gtk-doc assumes <module-name>-sections.txt and <module-name>-.types

> Tools/gtk/generate-gtkdoc:64
> -    # This technically is not needed for the GObject DOM bindings documentation itself,
> -    # but adding it doesn't hurt and allows us to avoid a check here.
> -    paths.append(common.build_path('Documentation', 'webkitdomgtk', 'html'))
> +
> +    if webkitdom_basename is not None:
> +        paths.append(common.build_path('Documentation', webkitdom_basename, 'html'))

I think the comment is still valid, it's harmless to add this for webkitdom, but we avoid not only a check but having to pass the webkitdom_basename all around. Here we just need to append the binary-version to webkitdomgtk

[Michael Catanzaro]

(In reply to comment #17)
> Could you explain what was wrong and why we need to rename all the files?

No I can't, not without digging much deeper into gtk-doc than I care to go. gtkdoc expects the filenames to match the output. My first patch attempted to use alternate filenames anyway, which mostly worked, but I failed to determine what was causing the broken links.

> I don't think we need to rename this file. This will break the script to
> detect DOM bindings API breaks, and the DOM bindings generator itself.

Indeed, drat.

> Why do we need to rename the docs sgml file? doesn't the gtk-doc
> main-sgml-file command line option work?

Well that's what my previous patch used, and it mostly worked, but it caused those broken links that I couldn't explain. I originally attempted --main-sgml-file as a workaround so that I wouldn't have to rename anything, but I suppose renaming is just a workaround for the problems with --main-sgml-file from another perspective.

> is it a bug in gtk-doc?

Maybe, but probably an oversight in my original patch.

> I'm fine with renaming these if gtk-doc assumes <module-name>-sections.txt
> and <module-name>-.types

It assumes they match the main sgml file.

> I think the comment is still valid, it's harmless to add this for webkitdom,
> but we avoid not only a check but having to pass the webkitdom_basename all
> around. Here we just need to append the binary-version to webkitdomgtk

I only did that to avoid hardcoding the API version, which was slightly awkward here but is achieved everywhere else in our gtkdoc generation scripts. Whether it's added unconditionally or after the check makes no difference.

[Carlos Garcia Campos]

(In reply to comment #18)
> (In reply to comment #17)
> > Could you explain what was wrong and why we need to rename all the files?
> 
> No I can't, not without digging much deeper into gtk-doc than I care to go.
> gtkdoc expects the filenames to match the output. My first patch attempted
> to use alternate filenames anyway, which mostly worked, but I failed to
> determine what was causing the broken links.
> 
> > I don't think we need to rename this file. This will break the script to
> > detect DOM bindings API breaks, and the DOM bindings generator itself.
> 
> Indeed, drat.
> 
> > Why do we need to rename the docs sgml file? doesn't the gtk-doc
> > main-sgml-file command line option work?
> 
> Well that's what my previous patch used, and it mostly worked, but it caused
> those broken links that I couldn't explain. I originally attempted
> --main-sgml-file as a workaround so that I wouldn't have to rename anything,
> but I suppose renaming is just a workaround for the problems with
> --main-sgml-file from another perspective.

Not exactly, your previous patch used main-sgml-file but left -sections.txt and .types with the original names. 

> > is it a bug in gtk-doc?
> 
> Maybe, but probably an oversight in my original patch.
> 
> > I'm fine with renaming these if gtk-doc assumes <module-name>-sections.txt
> > and <module-name>-.types
> 
> It assumes they match the main sgml file.

Are you sure? Have you tried renaming only -sectiions.txt and .types files and leaving the sgml file using --main-sgml-file?

> > I think the comment is still valid, it's harmless to add this for webkitdom,
> > but we avoid not only a check but having to pass the webkitdom_basename all
> > around. Here we just need to append the binary-version to webkitdomgtk
> 
> I only did that to avoid hardcoding the API version, which was slightly
> awkward here but is achieved everywhere else in our gtkdoc generation
> scripts. Whether it's added unconditionally or after the check makes no
> difference.

If we are going to add the check, I prefer to pass the module name always, and check if it starts with webkitdom instead of passing webkitdom_basename or None

[Michael Catanzaro]

(In reply to comment #19)
> Are you sure? Have you tried renaming only -sectiions.txt and .types files
> and leaving the sgml file using --main-sgml-file?

OK, that does work (exactly the same as when none of the files are renamed: i.e. the broken links are still a problem). Apparently the names of these files don't matter at all. That is more than a bit surprising....

> If we are going to add the check, I prefer to pass the module name always,
> and check if it starts with webkitdom instead of passing webkitdom_basename
> or None

But the webkit2gtk docs need to link to the webkitdom docs; webkitdom doesn't need to add itself to its own search path.

[Carlos Garcia Campos]

(In reply to comment #20)
> (In reply to comment #19)
> > Are you sure? Have you tried renaming only -sectiions.txt and .types files
> > and leaving the sgml file using --main-sgml-file?
> 
> OK, that does work (exactly the same as when none of the files are renamed:
> i.e. the broken links are still a problem). Apparently the names of these
> files don't matter at all. That is more than a bit surprising....

Pretty weird, because it works for webkit2gtk but not for webkitdom.

> > If we are going to add the check, I prefer to pass the module name always,
> > and check if it starts with webkitdom instead of passing webkitdom_basename
> > or None
> 
> But the webkit2gtk docs need to link to the webkitdom docs; webkitdom
> doesn't need to add itself to its own search path.

Oh, you are right. Then let's make it generic and call it extra_module instead of webkitdom_basename

[Michael Catanzaro]

(In reply to comment #21)
> Pretty weird, because it works for webkit2gtk but not for webkitdom.

OK, this was a pain. I finally discovered something by comparing the generated files with attachment #244928 [details] applied. We have Documentation/webkitdomgtk-4.0/webkitdomgtk-4.0-sections.txt with this weird pseudoclass:

<SECTION>
<FILE>WebKitDOMDeprecated</FILE>
webkit_dom_html_element_get_inner_html
webkit_dom_html_element_set_inner_html
webkit_dom_html_element_get_outer_html
webkit_dom_html_element_set_outer_html
</SECTION>

We also have DerivedSources/webkitdom/docs/webkitdomgtk-sections.txt, which has those methods under WebKitDOMHTMLElement where they belong. So why is this file being generated twice? Well the answer explains why "the names of these files don't matter at all" (they do).

Here is the problem: -sections.txt DOES need to match the module name (the name we use for the output, not the main sgml file!); I think .types and .symbols do as well. But it is generated by gtkdoc-scan if it's not already present [1]. That's what's happening with my first patch; when we try to use our original name webkit2gtk-sections.txt when generating output that file is ignored, gtkdoc-scan creates webkit2gtk-4.0-sections.txt itself (in the build directory where I didn't notice) and uses its new copy, ignoring ours. For webkit2gtk that works mostly all right (the only problem I noticed is that the links to webkitdom were broken, but that was because I hadn't discovered and updated that portion of the generate-gtkdoc script for that patch), but for webkitdom the difference is significant.

So the patch that renames everything is the correct approach, and I just need to update the scripts I missed (DOM bindings generator and DOM API checker). Renaming the main sgml files is optional, but it would be weird not to do so as we have to rename the other ones anyway.

[1] https://developer.gnome.org/gtk-doc-manual/stable/howdoesgtkdocwork.html.en

[Michael Catanzaro]

(In reply to comment #22)
> So the patch that renames everything is the correct approach, and I just
> need to update the scripts I missed (DOM bindings generator and DOM API
> checker).

It's hard to avoid hardcoding the API version in the DOM bindings generator. And I'm afraid of breaking the DOM API checker on the build slaves if I pass it as an argument to DOM API checker. So I think it's best to hardcode it in generate-gtkdoc as well; trying to avoid that everywhere is much more effort than it's worth.

[Michael Catanzaro]

Created attachment 245146 [details]
Patch

[Carlos Garcia Campos]

Created attachment 245225 [details]
Patch

This works for me. I think some of the problems you had before were partially caused by previous generated doc files in the builddir. This is mostly a mix of previous patches and a slightly different approach by me in generate-gtkdoc

[Michael Catanzaro]

(In reply to comment #25) 
> This works for me. I think some of the problems you had before were
> partially caused by previous generated doc files in the builddir.

Maybe, but I was really careful about deleting those. Anyway, I confirm this patch works with none of the problems I previously noticed.

[WebKit Commit Bot]

Comment on attachment 245225 [details]
Patch

Clearing flags on attachment: 245225

Committed r179111: <http://trac.webkit.org/changeset/179111>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.