[PATCH] D23757: cmake: Support overriding Sphinx HTML doc install directory

[Dan Liew via llvm-commits]

delcypher added a comment.

I agree with what you're trying achieve but I have a few nits. Once we've fixed those you also need to document the option in `doc/CMake.rst` (under LLVM-specific variables). You probably only need to document for LLVM (i.e. `LLVM_INSTALL_SPHINX_HTML_DIR` or whatever we decide to name it) and not the other sub-projects.

If you can get all that done and @rnk has no objections we can probably merge.


================
Comment at: cmake/modules/AddSphinxTarget.cmake:57
@@ +56,3 @@
+        string(TOUPPER "${project}" project_upper)
+        set(${project_upper}_INSTALL_HTML "share/doc/${project}/html"
+            CACHE STRING "HTML documentation install directory for ${project}")
----------------
This is not a good name for the variable because we also generate HTML doxygen documentation and thus this variable name is ambiguous (see https://github.com/llvm-mirror/llvm/blob/2c37c7740e3ea7a68447b4890e062abca9b69c0f/docs/CMakeLists.txt#L97 ). How about

`${project_upper}_INSTALL_SPHINX_HTML_DIR`

?

================
Comment at: cmake/modules/AddSphinxTarget.cmake:60
@@ +59,3 @@
+
+        install(DIRECTORY "${SPHINX_BUILD_DIR}/."
+                DESTINATION "${${project_upper}_INSTALL_HTML}")
----------------
Why did you change `"${SPHINX_BUILD_DIR}"` to `"${SPHINX_BUILD_DIR}/."` ?


https://reviews.llvm.org/D23757