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

Doxygen improvements #2476

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

lmoureaux
Copy link
Contributor

@lmoureaux lmoureaux commented Dec 29, 2024

The aim of this PR is to add the Doxygen output to our ReadTheDocs website.

Once merged one can write:

:freeciv21:`function_name`

and get a link to that function in the latest documentation. Unfortunately it's not possible to do it for PR builds, the link prefix needs to be fixed (you can replace it manually though).

Example from this PR: https://longturn--2476.org.readthedocs.build/en/2476/Doxygen/fcintl_8h.html

Note: Currently Doxygen is not working (fixed)

We need a special version of doxylink from my fork. I opened a PR but
it's relatively large and upstream doesn't seem very active, so we can
use the fork for now.
This includes a link in the sidebar and most code references in the
Coding section.
@lmoureaux lmoureaux marked this pull request as ready for review December 30, 2024 00:05
@lmoureaux lmoureaux requested a review from jwrober December 30, 2024 00:05
Copy link
Collaborator

@jwrober jwrober left a comment

Choose a reason for hiding this comment

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

We should also update the docs style guide to describe when to use the new freeciv21 interpreted text role. I'm sure there is more. I want to test locally. Is there a needed change to requirements.txt? Another package to install for doc authors?

@@ -17,4 +17,5 @@ architected.
logging.rst
network-protocol.rst
scorelog.rst
Doxygen Documentation <https://longturn.readthedocs.io/en/latest/Doxygen>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This won't work for local builds. Can we use a :doc: link instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not, Sphinx doesn't know about the Doxygen pages.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about a relative URI vs absolute?

Copy link
Contributor Author

@lmoureaux lmoureaux Dec 30, 2024

Choose a reason for hiding this comment

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

Sphinx tries to find a rst file and complains that it's not there. The only workaround I found is to create a dummy rst, then replace the generated HTML.

Copy link
Collaborator

Choose a reason for hiding this comment

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

is the complaint a warning or fatal? If a warning, I'm personally fine. We have some warnings already due to how the man pages are done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It cannot resolve the reference and doesn't produce a link at all...

Copy link
Collaborator

Choose a reason for hiding this comment

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

bummer, ok

@@ -1,3 +1,6 @@
sphinx<7.2 # See #2023
sphinx_rtd_theme
sphinx_last_updated_by_git

# Optional: link Doxygen docs from sphinx
git+https://github.com/lmoureaux/doxylink
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we should rely on something external to the Longturn set of repos

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a PR upstream, but it will take some time for them to merge and release. It's not a particularly small one...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok. How about an update to the comment to the same so we know we need to change to upstream at some point in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can also fork it within the org.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meanwhile my PR upstream has been reviewed.

This is needed for doxygen.
@lmoureaux
Copy link
Contributor Author

lmoureaux commented Dec 30, 2024

To produce the Doxygen docs locally, run:

cmake --build build --target docs

from the root of the source tree. The doc index gets written to doxygen/html/index.html.
This also builds the Sphinx docs, but cross-referencing won't work.

To build the cross-linked Sphinx docs, you need a virtualenv:

python -m virtualenv venv
source venv/bin/activate
pip install -r docs/requirements.txt

Then build outside of CMake:

sphinx-build docs build/docs/

I'm not sure where and how to explain this in the docs. The setup is definitely tricky.

@lmoureaux
Copy link
Contributor Author

I could change CMake to create the virtualenv and run Sphinx inside it, but I don't like when build systems try to pull packages from random internet locations.

# command does it. The output is a dummy one so the command always runs.
# doxygen has some level of caching so it doesn't take that long, and
# collecting a list of all source files would be cumbersome.
add_custom_command(OUTPUT dummy-doxygen
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not call this "dummy", I would simply call this what it is. You are building doxy. I'd prefer it be in /build tho, vs the root so we don't need another .gitignore.

Copy link
Contributor Author

@lmoureaux lmoureaux Dec 30, 2024

Choose a reason for hiding this comment

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

I would not call this "dummy", I would simply call this what it is. You are building doxy.

The output is really a dummy one. CMake expects a file there, but I give something that never gets produced to force running doxygen every time. This is what the comment above tries to explain.

I'd prefer it be in /build tho, vs the root so we don't need another .gitignore.

The output folder is set in the Doxyfile and cannot be overridden in the Doxygen command line. I cannot easily change it because I need to run doxygen in the RTD builder, and installing all the dependencies there would be overkill. Maybe I can come up with some CMake magic to configure the Doxyfile on RTD...

@jwrober
Copy link
Collaborator

jwrober commented Jan 4, 2025

Testing locally, having challenges. As we move through this, we definitely need to document it all in docs/Getting/compile.rst once we get everything figured out.

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

Successfully merging this pull request may close these issues.

2 participants