WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
116376
[GTK] [CMake] Add support for generating gtkdoc
https://bugs.webkit.org/show_bug.cgi?id=116376
Summary
[GTK] [CMake] Add support for generating gtkdoc
Martin Robinson
Reported
2013-05-17 19:31:52 PDT
We need the CMake build to generate gtkdoc just like the autotools one.
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Gustavo Noronha (kov)
Comment 1
2013-12-19 18:25:26 PST
Created
attachment 219711
[details]
Patch
Gustavo Noronha (kov)
Comment 2
2013-12-22 12:39:32 PST
Created
attachment 219882
[details]
Patch
Gustavo Noronha (kov)
Comment 3
2013-12-22 12:55:59 PST
Created
attachment 219884
[details]
Patch
Martin Robinson
Comment 4
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).
Gustavo Noronha (kov)
Comment 5
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!
Gustavo Noronha (kov)
Comment 6
2013-12-23 13:04:09 PST
Created
attachment 219927
[details]
Patch
Martin Robinson
Comment 7
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?
Gustavo Noronha (kov)
Comment 8
2013-12-23 13:22:53 PST
Created
attachment 219928
[details]
Patch for landing
WebKit Commit Bot
Comment 9
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
>
WebKit Commit Bot
Comment 10
2013-12-23 13:57:49 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug