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

Updates documentation versioning #4613

Merged
merged 20 commits into from
Apr 24, 2024
Merged

Updates documentation versioning #4613

merged 20 commits into from
Apr 24, 2024

Conversation

jasonb5
Copy link
Collaborator

@jasonb5 jasonb5 commented Apr 11, 2024

  • Adds new side bar toggle for versions
  • Removes fork of sphinx_rtd_theme and dropdown version selector

Test suite: n/a
Test baseline: n/a
Test namelist changes: n/a
Test status: n/a

Fixes n/a
User interface changes?: n/a
Update gh-pages html (Y/N)?: N

Copy link
Contributor

You can preview documentation at https://esmci.github.io/cime/branch/fix_docs_version/html/index.html

@jasonb5
Copy link
Collaborator Author

jasonb5 commented Apr 12, 2024

TODO:

  • Update wiki with new build/publish instructions

Copy link
Contributor

@jedwards4b jedwards4b left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

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

Thanks for your work on the documentation @jasonb5 . I appreciate the goal of removing the dependence on the sphinx_rtd_theme fork, and at first I liked the simplicity of this new approach. But as I note below, I wonder if this means dropping one of the major goals of the original versioned documentation....

</dl>
{% endif %}
<hr/>
{% trans %}Free document hosting provided by <a href="http://www.readthedocs.org">Read the Docs</a>.{% endtrans %}
Copy link
Member

Choose a reason for hiding this comment

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

I'm seeing this ReadTheDocs advertisement appear at the bottom. Is that intentional? It's not a big deal, but if it isn't intentional I'm wondering if we should remove that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea I'll remove this, came with the boilerplate code and we're not even hosting them on readthedocs.

Comment on lines 216 to 222
versions = (
("master", "master"),
("CESM2.2", "cesm2.2"),
("CESM2.1 (cime5.6)", "maint-5.6"),
("ufs_release_v1.1", "ufs_release_v1.1"),
("ufs_release_v1.0", "ufs_release_v1.0"),
)
Copy link
Member

Choose a reason for hiding this comment

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

If I understand this right, I am concerned about this piece. A major goal of the original versioned documentation was to avoid needing to update every branch and rebuild all versions of the documentation whenever we add a new version. The current approach does that by building the version list dynamically with a bit of JavaScript... if I remember correctly that's done with https://github.com/ESMCI/cime/blob/master/doc/source/_static/pop_ver.js, using the versions listed in https://github.com/ESMCI/cime/blob/gh-pages/versions/versions.json. Note that the versions.json just exists in a single place, on the gh-pages branch, so adding a new version just requires updating that file on the gh-pages branch: no changes are needed to the doc source, and no rebuild is needed.

Am I understanding right that this new approach will require keeping this versions list up to date on all branches from which documentation is built, and then rebuilding the documentation from all branches whenever a new version is added? If so, I want to make sure that this is something that people are comfortable with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@billsacks Yes all versions built after this change won't have new versions listed but will have all previous versions. The master documentation will have links to all versions and all versions will have a link to master. This would break consistency if a user were to go master -> cesm2.2 -> cesm2.3 where cesm2.3 was built after cesm2.2.

If the above use case is a hard requirement I can look into supporting it.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know about a hard requirement, but I think this is worth at least having a discussion about (unless there already has been some discussion that I missed). If this can't be supported, I guess I'd like to hear a little more about what's motivating this change: were there problems supporting the sphinx_rtd_theme? I'm partly interested because we use this same approach throughout CESM, so I'm curious what problems might apply elsewhere as well and if we can come up with a consistent solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can look into supporting this being dynamic as before. As for the motivation, using the newest sphinx conflicted with the fork of sphinx_rtd_theme when building the docs, switching to the latest resolved the issue. Quick search showed a method to add a version selector using the latest version of the theme. Rather than continuing to maintain a fork of the theme, I thought it best to utilize the boiler code and remove the dependency on the fork.

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, I was afraid that might happen. A bit of background, if I remember correctly: the fork I set up was based on a branch that someone created that (I believe) was never actually merged into the main sphinx_rtd_theme. I hoped we could keep it limping along, and we've needed to make a few changes to keep it working. But I agree that this is a maintenance issue.

In the near term, I'm fine with the change you have here, and I don't mind if you go ahead and merge it if you'd like. But before we go too far along with this, I wonder if we should revisit where documentation is hosted. Should we just move to hosting on readthedocs, for example, which I think might resolve these issues?

If you can find a way to support dynamic versioning (e.g., via the existing JavaScript or something else) with the new method without investing too much time, that could be ideal... but that could also be a rabbit hole, so I leave it up to you whether to pursue that.

Thanks again for your work on this, and I'm happy to talk more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found a quick way to utilize the existing versions.json, we're back to that being the only file that needs editing.

@jedwards4b
Copy link
Contributor

@billsacks are you planning to re-review or can this be merged?

Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

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

Thanks for all your work on this @jasonb5 ! I'm really happy that you found a way to use versions.json dynamically! This solution could be something we want to apply to other CESM documentation repos.

@jedwards4b jedwards4b merged commit a3abfd9 into master Apr 24, 2024
2 checks passed
@jasonb5 jasonb5 deleted the fix_docs_version branch April 24, 2024 18:04
samsrabin added a commit to samsrabin/CTSM that referenced this pull request Oct 3, 2024
Based on ESMCI/cime#4613. Builds OK, but I'm not sure if the new versions setup works.
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.

5 participants