-
Notifications
You must be signed in to change notification settings - Fork 2
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
Create .readthedocs.yaml #94
Conversation
Problem with doxygen generated html resolved. Had to use the post_build command found in the readthedoc.yaml as stated in read the docs docs Here is the link |
Fantastic! |
In this branch should I remove the github based hosting docs or keep that for now too? Long story short after this is merged we will have two different places for this our docs which seems unnecessary. |
I would remove github-hosted docs in a separate PR. I think a little bit of redundancy is good at this time. By the way, should we merge #92 first and then this PR? @cameronrutherford, please chime in, as well. |
CC @kswirydo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I added some minor suggestion.
with open('./doxygen/Doxyfile.in', 'a') as f: | ||
f.write("\nOUTPUT_DIRECTORY=../_readthedocs/html/doxygen") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be super helpful to document where this directory is locally and where it is copied to later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This directory does not exist locally and will only exist on the read the docs server where the builds happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding a comment like this would be helpful. It beats me why it is ../_readthedocs
. It is ../
relative to where? In any case, we can create a separate issue to document these pathing issues, no need to hold this PR.
html_static_path = [os.path.join(conf_directory, 'sphinx/_build/_static')] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, it would be helpful to understand why this specific path is chosen. Thinking of me trying to figure out why docs are not showing up at the remote location a few months from now :).
OUTPUT_DIRECTORY = sphinx/_build/doxygen | ||
# OUTPUT_DIRECTORY = @PROJECT_BINARY_DIR@/html/doxygen | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we using Doxygen default output dir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is replaced by the command in conf.py but I will document this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. There are only some minor issues, mainly documenting scripts, that need to be handled.
I would wait for @cameronrutherford's feedback before merging this, but as far as I am concerned this is good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few necessary changes, but this is awesome!
I still think this is the bigger selling point over quarto (having versioning), but I would pay big money to figure out how to mix the two... I think it's possible
.readthedocs.yaml
Outdated
apt_packages: | ||
- ghostscript | ||
- graphviz | ||
- texlive-full |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if you need texlive if we aren't rendering into a PDF.
I don't know what ghostscript is either...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ghostscript is a tool that processes postscript files and is often used with tex engines. I think it may be needed if we want to produce docs for printing. If it is not causing any unnecessary overhead, I would leave it as is for now.
post_build: | ||
- cd docs && doxygen ./doxygen/Doxyfile.in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just what we were looking for :)
User guides and source code documentation are always linked on this site. | ||
`ReSolve Github Project <https://github.com/ORNL/ReSolve>`_. | ||
`Source documentation <doxygen/html/index.html>`_ | ||
`Source documentation <html/index.html>`_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this page is a super quick redirect, maybe we could just make the sidebar entry link straight to Doxygen docs? IMO we could also change name to just Doxygen Docs
then as well. (maybe Source Documentation is cleaner language)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we leave links to both, general ReSolve documentation (top level docs) and the direct link to source documentation (Doxygen generated). The former is for users who just want to interface to ReSolve and the latter is for contributors who need to know about ReSolve's internals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cameronrutherford looks like sphinx toctree can only include rst based documents in the toctree and there isn't very obvious support for including html files in the toctree which is what the doxygen page are.
docs/sphinx/notice.rst
Outdated
Notice | ||
******* | ||
|
||
.. mdinclude:: ../../NOTICE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://resolve.readthedocs.io/en/latest/sphinx/notice.html - you can't simply use an extension without installing it ;)
See #92 and look a little closer into what python deps I install
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So why don't we simply write these files (license and notice) in rst format and include them without the conversion tool?
Post ReSolve documentation at readthedocs.
Include Doxygen generated source code documentation, as well, and provide a link to it on readthedocs..