Skip to content

Allow Staging Sphinx Documentation#764

Merged
chapman39 merged 8 commits into
developfrom
feature/chapman39/specify-sphinx-source-tree
Jun 3, 2026
Merged

Allow Staging Sphinx Documentation#764
chapman39 merged 8 commits into
developfrom
feature/chapman39/specify-sphinx-source-tree

Conversation

@chapman39
Copy link
Copy Markdown
Contributor

  • adds new arguments to blt_add_sphinx_target to set sphinx source and conf.py dirs, as well as an explicit DEPENDS argument
  • adds documentation for the motivation and how to use this new feature
  • tested minimal, advanced examples, also one using conf.py.in file

Copy link
Copy Markdown
Member

@kennyweiss kennyweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chapman39

The PR description says

tested minimal, advanced examples, also one using conf.py.in file

but I didn't see a test.

Also, please see my comment about staging

Comment thread cmake/SetupDocs.cmake Outdated
Comment thread docs/api/documentation.rst
Comment thread cmake/SetupDocs.cmake Outdated
Comment thread cmake/SetupDocs.cmake Outdated
Comment thread cmake/SetupDocs.cmake Outdated
@chapman39
Copy link
Copy Markdown
Contributor Author

thank you for the feedback @kennyweiss .

about testing, i meant i tested the cmake macro locally for the 3 use-cases mentioned. if we want documentation testing in blt let me know. we might need to modify the host configs to include SPHINX_DIR, or something. or i can create an issue on it and we can come back to it.

@kennyweiss
Copy link
Copy Markdown
Member

I think it's in good shape after your latest changes (i.e. highlighting in the code/user docs and RELEASE_NOTES that TARGET is now required)

@chapman39 chapman39 merged commit 8999632 into develop Jun 3, 2026
17 checks passed
@chapman39 chapman39 deleted the feature/chapman39/specify-sphinx-source-tree branch June 3, 2026 18:06
Comment thread cmake/SetupDocs.cmake
set(_doxygen_options)
set(_doxygen_single_value_args TARGET)
set(_doxygen_multi_value_args)
cmake_parse_arguments(DOXYGEN
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with the rest of BLT, this should have been:

    cmake_parse_arguments(arg

Like:

cmake_parse_arguments(arg
"${options}" "${singleValuedArgs}" "${multiValuedArgs}" ${ARGN} )
# Sanity checks
if(NOT DEFINED arg_TARGET)
message(FATAL_ERROR "TARGET is a required parameter for blt_set_target_folder macro")
endif()

This makes it so all the arguments stand out, for example DOXYGEN_TARGET would have been arg_TARGET.

This isnt user facing so is easy to fix in a follow up PR. Would you mind doing one?

Comment thread cmake/SetupDocs.cmake
"blt_add_doxygen_target received unexpected arguments: ${DOXYGEN_UNPARSED_ARGUMENTS}")
endif()

if(NOT DOXYGEN_TARGET)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specific check should probably go first as well.

.. code-block:: cmake

set(PROJECT_A_STAGE_DIR "${CMAKE_CURRENT_BINARY_DIR}/_stage")
set(PROJECT_A_SOURCE_DIR "${PROJECT_A_STAGE_DIR}/src/docs")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should set PROJECT_B_SOURCE_DIR here as well and just call it SPHINX_STAGE_DIR not PROJECT_A_STAGE_DIR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmake documentation Issues related to documentation feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants