Bug 116376

Created attachment 219711 [details] Patch

Created attachment 219882 [details] Patch

Created attachment 219884 [details] Patch

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).

(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!

Created attachment 219927 [details] Patch

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?

Created attachment 219928 [details] Patch for landing

Comment on attachment 219928 [details] Patch for landing Clearing flags on attachment: 219928 Committed r161017: <http://trac.webkit.org/changeset/161017>

All reviewed patches have been landed. Closing bug.