Bug 116376 - [GTK] [CMake] Add support for generating gtkdoc
Summary: [GTK] [CMake] Add support for generating gtkdoc
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gustavo Noronha (kov)
URL:
Keywords:
Depends on:
Blocks: 115966
  Show dependency treegraph
 
Reported: 2013-05-17 19:31 PDT by Martin Robinson
Modified: 2013-12-23 13:57 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.54 KB, patch)
2013-12-19 18:25 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Patch (9.22 KB, patch)
2013-12-22 12:39 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Patch (9.56 KB, patch)
2013-12-22 12:55 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Patch (9.10 KB, patch)
2013-12-23 13:04 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Patch for landing (8.91 KB, patch)
2013-12-23 13:22 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2013-05-17 19:31:52 PDT
We need the CMake build to generate gtkdoc just like the autotools one.
Comment 1 Gustavo Noronha (kov) 2013-12-19 18:25:26 PST
Created attachment 219711 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2013-12-22 12:39:32 PST
Created attachment 219882 [details]
Patch
Comment 3 Gustavo Noronha (kov) 2013-12-22 12:55:59 PST
Created attachment 219884 [details]
Patch
Comment 4 Martin Robinson 2013-12-22 15:07:07 PST
Comment on attachment 219884 [details]
Patch

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

Nice!

> Tools/gtk/CMakeLists.txt:1
> +set(GTK_DOCS_DEPENDENCIES

Perhaps it makes sense to put this into Source/PlatformGTK.cmake ? I think that would be a better match for the CMake build structure and would also fit well next to the parts that will trigger the GObject API test builds. I sort of regret putting the makefile into the tools directory now, as it depends  on the sources.

> Tools/gtk/CMakeLists.txt:11
> +if (ENABLE_WEBKIT)
> +    list(APPEND GTK_DOCS_DEPENDENCIES
> +        WebKit
> +        "${CMAKE_SOURCE_DIR}/Source/WebKit/gtk/docs/webkitgtk-docs.sgml"
> +        "${CMAKE_SOURCE_DIR}/Source/WebKit/gtk/docs/webkitgtk-sections.txt"
> +    )
> +endif ()

This is probably okay for a first cut, but eventually we want to have the actual API source files listed here as well -- perhaps just an overly greedy glob of the directory. The reasoning is that we want the build to error out when someone forgets to update the sections file. Without the dependencies set up properly, a developer might miss keeping the docs up-to-date and worse the build bot will also not complain. Unless of course, I'm misunderstanding this!

> Tools/gtk/generate-gtkdoc:69
> +    # CMake uses lib instead of .libs.
> +    library_path = common.build_path('.libs')
> +    if not os.path.exists(library_path):
> +        library_path = common.build_path('lib')
> +
>      return {
>          'decorator': 'WEBKIT_API|WEBKIT_DEPRECATED|WEBKIT_DEPRECATED_FOR\(.+\)',
>          'deprecation_guard': 'WEBKIT_DISABLE_DEPRECATED',
> -        'library_path' : common.build_path('.libs'),
> +        'library_path' : library_path,

Maybe we could abstract this away into common to be something like common.library_build_path()?

> Tools/gtk/generate-gtkdoc:157
> +    # The CMake build puts the webkitversion.h file in DerivedSources/webkit, the
> +    # autotools one uses Source/WebKit/gtk/webkit.
> +    built_webkit_path = common.build_path('Source', 'WebKit', 'gtk', 'webkit')
> +    if os.path.exists(built_webkit_path):
> +        source_dirs.append(built_webkit_path)
> +    else:
> +        source_dirs.append(common.build_path('DerivedSources', 'webkit'))
> +

Looks like this could be abstracted away into a helper?

Imagine:

[src_path('webkit'), path_to_webkitversionh()],

> Tools/gtk/generate-gtkdoc:256
> +webkitdom_docs_path = common.build_path('DerivedSources', 'webkitdom', 'docs')
> +if os.path.exists(webkitdom_docs_path):
> +    generator = gtkdoc.PkgConfigGTKDoc(pkg_config_path, get_webkitdom_options())
> +    if '--rebase' not in sys.argv:
> +        print("\nGenerating WebKitDOM documentation...")
> +        saw_webkitdom_warnings = generate_doc(generator)
> +    else:
> +        print("\nRebasing WebKitDOM documentation...")
> +        try:
> +            generator.rebase_installed_docs()
> +        except Exception:
> +            print("Rebase did not happen, likely no documentation is present.")

Maybe instead of checking for the path, do this conditionally based on whether or not it's a cmake build? That way we'll get an error if something goes wrong.

> Tools/gtk/generate-gtkdoc:289
> +# For CMake we are still generating warnings because we lack DOM bindings docs,
> +# so do not cause the build to fail for now.
> +if not os.path.exists(os.path.join(os.getcwd(), 'CMakeCache.txt')):
> +    sys.exit(saw_webkit1_warnings or saw_webkit2_warnings)

It seems that we could make a helper like common.is_cmake_build_path(dir).
Comment 5 Gustavo Noronha (kov) 2013-12-23 05:57:25 PST
(In reply to comment #4)
> > Tools/gtk/CMakeLists.txt:11
> > +if (ENABLE_WEBKIT)
> > +    list(APPEND GTK_DOCS_DEPENDENCIES
> > +        WebKit
> > +        "${CMAKE_SOURCE_DIR}/Source/WebKit/gtk/docs/webkitgtk-docs.sgml"
> > +        "${CMAKE_SOURCE_DIR}/Source/WebKit/gtk/docs/webkitgtk-sections.txt"
> > +    )
> > +endif ()
> 
> This is probably okay for a first cut, but eventually we want to have the actual API source files listed here as well -- perhaps just an overly greedy glob of the directory. The reasoning is that we want the build to error out when someone forgets to update the sections file. Without the dependencies set up properly, a developer might miss keeping the docs up-to-date and worse the build bot will also not complain. Unless of course, I'm misunderstanding this!
> 

Yeah, I intended to do this in this patch, but then I ended up spending too much energy on fixing the other bits and the patch started looking quite complex already so I decided to take the easy way on this front (I planned to comment that when I submitted the patch but got distracted by bees).

> > Tools/gtk/generate-gtkdoc:69
> > +    # CMake uses lib instead of .libs.
> > +    library_path = common.build_path('.libs')
> > +    if not os.path.exists(library_path):
> > +        library_path = common.build_path('lib')
> > +
> >      return {
> >          'decorator': 'WEBKIT_API|WEBKIT_DEPRECATED|WEBKIT_DEPRECATED_FOR\(.+\)',
> >          'deprecation_guard': 'WEBKIT_DISABLE_DEPRECATED',
> > -        'library_path' : common.build_path('.libs'),
> > +        'library_path' : library_path,
> 
> Maybe we could abstract this away into common to be something like common.library_build_path()?

Sounds good!

> > Tools/gtk/generate-gtkdoc:157
> > +    # The CMake build puts the webkitversion.h file in DerivedSources/webkit, the
> > +    # autotools one uses Source/WebKit/gtk/webkit.
> > +    built_webkit_path = common.build_path('Source', 'WebKit', 'gtk', 'webkit')
> > +    if os.path.exists(built_webkit_path):
> > +        source_dirs.append(built_webkit_path)
> > +    else:
> > +        source_dirs.append(common.build_path('DerivedSources', 'webkit'))
> > +
> 
> Looks like this could be abstracted away into a helper?
> 
> Imagine:
> 
> [src_path('webkit'), path_to_webkitversionh()],

Yep!

> > Tools/gtk/generate-gtkdoc:256
> > +webkitdom_docs_path = common.build_path('DerivedSources', 'webkitdom', 'docs')
> > +if os.path.exists(webkitdom_docs_path):
> > +    generator = gtkdoc.PkgConfigGTKDoc(pkg_config_path, get_webkitdom_options())
> > +    if '--rebase' not in sys.argv:
> > +        print("\nGenerating WebKitDOM documentation...")
> > +        saw_webkitdom_warnings = generate_doc(generator)
> > +    else:
> > +        print("\nRebasing WebKitDOM documentation...")
> > +        try:
> > +            generator.rebase_installed_docs()
> > +        except Exception:
> > +            print("Rebase did not happen, likely no documentation is present.")
> 
> Maybe instead of checking for the path, do this conditionally based on whether or not it's a cmake build? That way we'll get an error if something goes wrong.

Yes, maybe a helper function that caches whether it's a cmake build and we can use that knowledge on those other helpers above.

> > Tools/gtk/generate-gtkdoc:289
> > +# For CMake we are still generating warnings because we lack DOM bindings docs,
> > +# so do not cause the build to fail for now.
> > +if not os.path.exists(os.path.join(os.getcwd(), 'CMakeCache.txt')):
> > +    sys.exit(saw_webkit1_warnings or saw_webkit2_warnings)
> 
> It seems that we could make a helper like common.is_cmake_build_path(dir).

This =) thanks for the review!
Comment 6 Gustavo Noronha (kov) 2013-12-23 13:04:09 PST
Created attachment 219927 [details]
Patch
Comment 7 Martin Robinson 2013-12-23 13:08:15 PST
Comment on attachment 219927 [details]
Patch

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

Thanks! A couple things to consider for landing...

> Source/PlatformGTK.cmake:1
> +set(GTK_DOCS_DEPENDENCIES

I have been using camel case for most CMake variables, so perhaps this could be DocumentationDependencies instead of GTK_DOCS_DEPENDENCIES?

> Tools/gtk/common.py:33
> +    if type(is_cmake) is not bool:

is_cmake == None ?

> Tools/gtk/generate-gtkdoc:140
> +    source_dirs = [src_path('webkit')]
> +

Looks like this is unused now?
Comment 8 Gustavo Noronha (kov) 2013-12-23 13:22:53 PST
Created attachment 219928 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2013-12-23 13:57:46 PST
Comment on attachment 219928 [details]
Patch for landing

Clearing flags on attachment: 219928

Committed r161017: <http://trac.webkit.org/changeset/161017>
Comment 10 WebKit Commit Bot 2013-12-23 13:57:49 PST
All reviewed patches have been landed.  Closing bug.