[clang][cmake] Include generated rst files in html built by docs-clang-html target

[tstellar]

Summary

This is an attempt to simply the process of building the clang
documentation, which should help avoid some of the recent issues we've
had generating the documentation for the website.

The html documentation for clang is generated by sphinx from the
reStructuredText (rst) files we have in the clang/docs directory.
There are also some rst files that need to be generated by TableGen,
before they can be passed to sphinx. Prior to this patch we were not
generating those rst files as part with the build system and they had to be
generated manually.

This patch enables the automatic generation of these rst files, but
since they are generated at build time the cannot be placed in the
clang/docs directory and must go into the cmake build directory.

Unfortunately sphinx does not currently support multiple source
directories[1], so in order to be able to generate the full
documentation, we need to work around this by copying the
rst files from the clang/docs into the build directory before
generating the html documentation.

[1] https://github.com/sphinx-doc/sphinx/issues/3132

[merge_guards_bot]

Unit tests: pass. 61925 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[aaron.ballman]

Thank you for working on this, I think this is a great improvement. I am not super qualified to review the cmake, but it looks eminently reasonable to me.

[mgorny]

Besides the mentioned misindent, the CMake code looks correct. Could you test whether it works with standalone (out-of-LLVM) builds as well?

clang/docs/CMakeLists.txt

116

Looks like misindent.

[tstellar]

• Fix indentation.
• Remove placeholder AttributeReference.rst. The install target was replacing the generated AttributeReference.rst with this which caused the html page to be empty.
• Tested with stand-alone clang builds: cmake ../clang -G Ninja -DCLANG_INCLUDE_DOCS=ON -DLLVM_BUILD_DOCS=ON -DLLVM_ENABLE_SPHINX=ON -DSPHINX_WARNINGS_AS_ERRORS=OFF

[merge_guards_bot]

Unit tests: pass. 61925 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[delcypher]

llvm/cmake/modules/AddSphinxTarget.cmake

33

@tstellar I'm not 100% sure about this but I think you probably want ${CMAKE_CURRENT_SOURCE_DIR} quoted. If it's not then if it contains spaces I think you might make ARG_SOURCE_DIR a list. Then we you go to print it you'll probably end up with list separators (i.e. ;) in the command you pass the the sphinx binary.

I've not tested this though so I could be wrong.

[tstellar]

• Add quotes around ${CMAKE_CURRENT_SOURCE_DIR}

[merge_guards_bot]

Unit tests: pass. 61925 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[tstellar]

Any other commits before I commit this?

llvm/cmake/modules/AddSphinxTarget.cmake

33

There are quotes used when setting variables above, so I went ahead and changed this to match.

[delcypher]

clang/docs/CMakeLists.txt

93

gen_rst_file is a very ambiguous name. At call sites it might not be obvious what this function does. Something like gen_rst_file_from_td might be more helpful.

94

You might want to add a check that ${CMAKE_CURRENT_SOURCE_DIR}/${source} exists in this function and error if it does not exist.

97

This add_dependencies(...) command is very fragile and is creating an implicit coupling between:

• The implementation of add_sphinx_target().
• The declaration of the clang html sphinx target.
• The implementation of gen_rst_file.

gen_rst_file should probably take an optional argument for the target (in this case docs-clang-html) to add a dependency to.

This does not completely fix the dependency on add_sphinx_target implementation but it does mean that gen_rst_file isn't also dependent.

107–120

Minor nit: You might want to quote uses of ${CMAKE_CURRENT_BINARY_DIR}, ${CMAKE_COMMAND}, and ${CMAKE_CURRENT_SOURCE_DIR}.

[tstellar]

Address most recent review comments.

[tstellar]

Ping.

[delcypher]

LGTM. Thanks for addressing all the issues I raised.

clang/docs/AttributeReference.rst

-..
-  -------------------------------------------------------------------
-  NOTE: This file is automatically generated by running clang-tblgen
-  -gen-attr-docs. Do not edit this file by hand!! The contents for
-  this file are automatically generated by a server-side process.
-  
-  Please do not commit this file. The file exists for local testing
-  purposes only.
-  -------------------------------------------------------------------
-
-===================
-Attributes in Clang
-===================
 

clang/docs/CMakeLists.txt

if (LLVM_ENABLE_DOXYGEN)
 
   if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
     install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/doxygen/html
       DESTINATION docs/html)
   endif()
 endif()
 endif()
 
+function (gen_rst_file_from_td output_file td_option source docs_target)
+  if (NOT EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/${source}")
+    message(FATAL_ERROR "Cannot find source file: ${source} in ${CMAKE_CURRENT_SOURCE_DIR}")
+  endif()
+  get_filename_component(TABLEGEN_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/${source}" DIRECTORY)
+  list(APPEND LLVM_TABLEGEN_FLAGS "-I${TABLEGEN_INCLUDE_DIR}")
+  clang_tablegen(${output_file} ${td_option} SOURCE ${source} TARGET "gen-${output_file}")
+  add_dependencies(${docs_target} "gen-${output_file}")
+endfunction()
+
 if (LLVM_ENABLE_SPHINX)
   include(AddSphinxTarget)
   if (SPHINX_FOUND)
     if (${SPHINX_OUTPUT_HTML})
-      add_sphinx_target(html clang)
+      add_sphinx_target(html clang SOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}")
+
+      # Copy rst files to build directory before generating the html
+      # documentation.  Some of the rst files are generated, so they
+      # only exist in the build directory.  Sphinx needs all files in
+      # the same directory in order to genrate the html, so we need to
+      # copy all the non-gnerated rst files from the source to the build
+      # directory before we run sphinx.
+      add_custom_target(copy-clang-rst-docs
+        COMMAND "${CMAKE_COMMAND}" -E copy_directory
+        "${CMAKE_CURRENT_SOURCE_DIR}"
+        "${CMAKE_CURRENT_BINARY_DIR}")
+      add_dependencies(docs-clang-html copy-clang-rst-docs)
+
       add_custom_command(TARGET docs-clang-html POST_BUILD
-        COMMAND ${CMAKE_COMMAND} -E copy
+        COMMAND "${CMAKE_COMMAND}" -E copy
         "${CMAKE_CURRENT_SOURCE_DIR}/LibASTMatchersReference.html"
         "${CMAKE_CURRENT_BINARY_DIR}/html/LibASTMatchersReference.html")
+
+      # Generated files
+      gen_rst_file_from_td(AttributeReference.rst -gen-attr-docs ../include/clang/Basic/Attr.td docs-clang-html)
+      gen_rst_file_from_td(DiagnosticsReference.rst -gen-diag-docs ../include/clang/Basic/Diagnostic.td docs-clang-html)
+      gen_rst_file_from_td(ClangCommandLineReference.rst -gen-opt-docs ../include/clang/Driver/ClangOptionDocs.td docs-clang-html)
     endif()
     if (${SPHINX_OUTPUT_MAN})
       add_sphinx_target(man clang)
     endif()
   endif()
 endif()

llvm/cmake/modules/AddSphinxTarget.cmake

 
 # Handy function for creating the different Sphinx targets.
 #
 # ``builder`` should be one of the supported builders used by
 # the sphinx-build command.
 #
 # ``project`` should be the project name
 function (add_sphinx_target builder project)
+  cmake_parse_arguments(ARG "" "SOURCE_DIR" "" ${ARGN})
   set(SPHINX_BUILD_DIR "${CMAKE_CURRENT_BINARY_DIR}/${builder}")
   set(SPHINX_DOC_TREE_DIR "${CMAKE_CURRENT_BINARY_DIR}/_doctrees-${project}-${builder}")
   set(SPHINX_TARGET_NAME docs-${project}-${builder})
 
   if (SPHINX_WARNINGS_AS_ERRORS)
     set(SPHINX_WARNINGS_AS_ERRORS_FLAG "-W")
   else()
     set(SPHINX_WARNINGS_AS_ERRORS_FLAG "")
   endif()
 
+  if (NOT ARG_SOURCE_DIR)
+    set(ARG_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}")
+  endif()
+
   add_custom_target(${SPHINX_TARGET_NAME}
                     COMMAND ${SPHINX_EXECUTABLE}
                             -b ${builder}
                             -d "${SPHINX_DOC_TREE_DIR}"
                             -q # Quiet: no output other than errors and warnings.
                             ${SPHINX_WARNINGS_AS_ERRORS_FLAG} # Treat warnings as errors if requested
-                            "${CMAKE_CURRENT_SOURCE_DIR}" # Source
+                            "${ARG_SOURCE_DIR}" # Source
                             "${SPHINX_BUILD_DIR}" # Output
                     COMMENT
                     "Generating ${builder} Sphinx documentation for ${project} into \"${SPHINX_BUILD_DIR}\"")
 
   # When "clean" target is run, remove the Sphinx build directory
   set_property(DIRECTORY APPEND PROPERTY
                ADDITIONAL_MAKE_CLEAN_FILES
                "${SPHINX_BUILD_DIR}")