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

[Michał Górny via llvm-commits]

mgorny added a comment.

Thanks for the review. I've replied to your comments, and will update the patch in a few hours.


================
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}")
----------------
delcypher wrote:
> 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`
> 
> ?
You're right, I'll use that. I'm planning to also add variables for the other kinds of documentation in a separate patch (since they end up in /usr/docs...).

================
Comment at: cmake/modules/AddSphinxTarget.cmake:60
@@ +59,3 @@
+
+        install(DIRECTORY "${SPHINX_BUILD_DIR}/."
+                DESTINATION "${${project_upper}_INSTALL_HTML}")
----------------
delcypher wrote:
> Why did you change `"${SPHINX_BUILD_DIR}"` to `"${SPHINX_BUILD_DIR}/."` ?
This is necessary to avoid implicitly creating additional 'html' subdirectory, i.e. otherwise it would be installed as ${...}/html.


https://reviews.llvm.org/D23757