This is an archive of the discontinued LLVM Phabricator instance.

cmake: Support overriding Sphinx HTML doc install directory
ClosedPublic

Authored by mgorny on Aug 21 2016, 3:06 AM.

Details

Summary

Provide ${PROJECT}_INSTALL_SPHINX_HTML_DIR variables (e.g. LLVM_INSTALL_SPHINX_HTML_DIR) to override Sphinx HTML doc install directory.

Bug: https://llvm.org/bugs/show_bug.cgi?id=23780

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny updated this revision to Diff 68808.Aug 21 2016, 3:06 AM
mgorny retitled this revision from to cmake: Support overriding Sphinx HTML doc install directory.
mgorny updated this object.
mgorny added a reviewer: rnk.
mgorny added a subscriber: llvm-commits.
delcypher edited edge metadata.Sep 21 2016, 2:57 AM

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.

cmake/modules/AddSphinxTarget.cmake
57 ↗(On Diff #68808)

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

?

60 ↗(On Diff #68808)

Why did you change "${SPHINX_BUILD_DIR}" to "${SPHINX_BUILD_DIR}/." ?

delcypher requested changes to this revision.Sep 21 2016, 2:58 AM
delcypher edited edge metadata.
This revision now requires changes to proceed.Sep 21 2016, 2:58 AM

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

cmake/modules/AddSphinxTarget.cmake
57 ↗(On Diff #68808)

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

60 ↗(On Diff #68808)

This is necessary to avoid implicitly creating additional 'html' subdirectory, i.e. otherwise it would be installed as ${...}/html.

mgorny updated this revision to Diff 72032.Sep 21 2016, 6:22 AM
mgorny updated this object.
mgorny edited edge metadata.

Updated the variable name and added to docs.

mgorny marked 2 inline comments as done.Sep 21 2016, 6:23 AM
delcypher requested changes to this revision.Sep 21 2016, 8:30 AM
delcypher edited edge metadata.
delcypher added inline comments.
cmake/modules/AddSphinxTarget.cmake
60 ↗(On Diff #72032)

I see. There should be a comment explaining this then.

docs/CMake.rst
461 ↗(On Diff #72032)

This default is incorrect. The default is actually share/doc/llvm/html. It does not contain the CMAKE_INSTALL_PREFIX.

Your text probably ought to say something like:

The path to install Sphinx-generated HTML documentation to. This path relative to the CMAKE_INSTALL_PREFIX and by default
is `share/doc/llvm/html`.
This revision now requires changes to proceed.Sep 21 2016, 8:30 AM
mgorny added inline comments.Sep 21 2016, 8:40 AM
docs/CMake.rst
461 ↗(On Diff #72032)

Well, this is not 100% correct since it is relative only when it's not absolute ;-). I'll try to find a good way to express it.

mgorny updated this revision to Diff 72060.Sep 21 2016, 8:51 AM
mgorny edited edge metadata.
mgorny marked 5 inline comments as done.

Added the comment and updated the doc. Hope it's clear enough now.

delcypher accepted this revision.Sep 23 2016, 3:33 AM
delcypher edited edge metadata.

Thanks for making the changes. LGTM.

This revision is now accepted and ready to land.Sep 23 2016, 3:33 AM
This revision was automatically updated to reflect the committed changes.

Thanks a lot for the review.