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

Move documentation build scripts from mpi_cmake_modules to separate package #1

Merged
merged 16 commits into from
Sep 30, 2022

Conversation

luator
Copy link
Member

@luator luator commented Sep 27, 2022

Description

This is an attempt to move the Python code for building package documentation out of the mpi_cmake_modules package into its own package.

Apart from minor adjustments the following two changes have been made compared to the version from mpi_cmake_modules:

  • Make the --project-version argument optional. If not set, the tool tries to auto-detect the version by looking at files like package.xml. Currently supported are package.xml (ament packages) and CMakeLists.txt for pure CMake packages. Support for other types of packages can easily be added.
  • Add a CSS fix to work around an incompatibility issue between the RTD theme and autodoc.

How I Tested

By using it to build documentation for some of my packages.

TODOs

  • Rename the package to breathing-cat

Copied with minor adjustments from mpi_cmake_modules.
Make `--project-version` optional and, if not set, try to auto-detect
the version of the package.
Currently it tries to parse the version from a package.xml or (if that
fails) from a CMakeLists.txt.  More functions can be added easily to
support more package types.
This will execute the same as `python3 -m mpiis_doc_build` but looks a
bit nicer.
The RTD theme has an incompatibility issue with autodoc as both use a
"property" CSS class for different purposes.  Add a custom CSS file with
a workaround to fix this issue.

See readthedocs/sphinx_rtd_theme#1247 for more
information.
@luator luator self-assigned this Sep 27, 2022
@MaximilienNaveau MaximilienNaveau added documentation Improvements or additions to documentation enhancement New feature or request labels Sep 27, 2022
import datetime
import os
import sys
from m2r import MdInclude

Choose a reason for hiding this comment

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

we need another dependency this one is deprecated as far I as I remember

Copy link
Member Author

Choose a reason for hiding this comment

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

m2r is for allowing a mix of Markdown and rst, right? I anyway found this possible mixing of two styles in one file a bit confusing, so I'm happy to drop it.

I think the officially supported way of using Markdown with Sphinx is using the MyST parser, so I would suggest switching to that.

Pro:

  • It is the official suggestion in the Sphinx documentation.
  • Clearer separation between Markdown and rst (you can use both md and rst files but within one file you have to be consistent).
  • MyST has some special syntax (non-standard markdown) to support rst roles/directives, so you still have all the options to use references, add autodoc entries, etc. when using Markdown.

Con:

  • It would be a breaking change as a mix-up of both markdown and rst within one file would not work anymore. However, my opinion is that this should not be done anyway, so I would be okay with this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created an issue for this: #2

import os
import sys
from m2r import MdInclude
from recommonmark.transform import AutoStructify

Choose a reason for hiding this comment

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

I do not remember why we needed this haha

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to look it up and noticed this in the documentation:

Warning: recommonmark is now deprecated. We recommend using MyST for a docutils bridge going forward. See this issue for background and discussion.

So I think this should be dropped together with m2r in favour of MyST. It is too much for this PR, though, I'll open an issue for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created an issue for this: #2

import datetime
import os
import sys
from m2r import MdInclude

Choose a reason for hiding this comment

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

I think this is deprecated.

breathing_cat/resources/sphinx/conf.py.in Show resolved Hide resolved
breathing_cat/resources/sphinx/conf.py.in Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
readme = "README.md"
license = {file = "LICENSE"}

dependencies = [

Choose a reason for hiding this comment

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

doxygen?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doxygen is not a Python package (i.e. cannot be installed by pip), so I think it shouldn't be listed here.
Unfortunately, I don't see a better way than asking the user to install it manually.

Choose a reason for hiding this comment

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

There must be a way to check for system dependencies, we have a packaging wizard in LAAS I will ask

Copy link
Member Author

Choose a reason for hiding this comment

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

It currently will show an error at runtime if doxygen is not found, including an apt command to install it. I think this should be enough.

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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants