Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build doxygen and sphinx (read-the-docs style) docs using cmake -D{project}_BUILD_DOCUMENTATION #14

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

tanaya-mankad
Copy link
Contributor

Description

Add support files for documentation scaffolding, and a CMake workflow that both checks for doxygen comments and converts them into web docs. (Any client of this workflow will still have to customize the ReStructuredText files.)

Author Progress Checklist:

  • Iterate with reviewers until all changes are approved
    • Make changes in response to reviewer comments
    • Re-request review in GitHub

Reviewer Checklist:

  • Read the pull request description
  • Perform a code review on GitHub
  • Confirm all CI checks pass and there are no build warnings
  • Pull, build, and run automated tests locally
  • Perform manual tests of the fix or feature locally
  • Add any review comments, if applicable
  • Submit review in GitHub as either
    • Request changes, or
    • Approve
  • Iterate with author until all changes are approved
  • If you are the last reviewer to approve, merge the pull request and delete the branch

@tanaya-mankad tanaya-mankad added the documentation Improvements or additions to documentation label Mar 18, 2024
@tanaya-mankad tanaya-mankad requested a review from nealkruis March 18, 2024 19:23
@tanaya-mankad tanaya-mankad self-assigned this Mar 18, 2024
Copy link
Member

@nealkruis nealkruis left a comment

Choose a reason for hiding this comment

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

  1. We need readme description to document our approach (and justification?) and instructions for installing dependencies.
  2. The CI should build and deploy the docs (maybe to GH pages for now?)

Copy link
Member

Choose a reason for hiding this comment

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

Is this file ever used? It doesn't look like it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it isn't. I'll remove it if you're content with the "hard-coded" sphinx exe name.
Originally, I would have liked to install the sphinx dependency with poetry during the configuration phase, then call FindSphinx.cmake to grab the executable's name, then use that in the target command. Alas, such a workflow doesn't run.

MAIN_DEPENDENCY ${DOXYFILE_OUT} ${DOXYFILE_IN}
COMMENT "Generating docs")

add_custom_target(Doxygen ALL DEPENDS ${DOXYGEN_INDEX_FILE})
Copy link
Member

Choose a reason for hiding this comment

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

This target name needs to be project-specific (otherwise it will conflict with submodules that also create a doxygen target).

# Note that "poetry run" during CMake's build phase does not activate an environment installed
# during CMake's configuration phase!

add_custom_target(Sphinx ALL
Copy link
Member

Choose a reason for hiding this comment

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

Needs project-specific target name (see previous comment).

pyproject.toml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

If we are adding a python dependency (which I think we want/need to), we should consider how much to utilize it? Should we constrain it just to cases where "BUILD_DOCUMENTATION" is set? or do we want to leverage it for other things too?

  • doit for task automation (could potentially replace some of our CMake scripts)
  • regression testing framework?
  • others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I put these use cases under Issues so we don't lose them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants