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

Switch the MPS manual to using the Read the Docs theme #166

Merged
merged 13 commits into from
Oct 23, 2023

Conversation

rptb1
Copy link
Member

@rptb1 rptb1 commented Feb 22, 2023

Working on #157 by experimenting with using the Read the Docs theme.

See https://memory-pool-system--166.org.readthedocs.build/en/166/ for the manual built from this pull request by Read the Docs CI.

The search in the Read the Docs theme works when the manual is built locally. It does not depend on the manual being published on a server.

This change could remove the MPS theme and thus reduce maintenance costs.

@rptb1 rptb1 added documentation build Problems with builds and build automation labels Feb 22, 2023
@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

Also consider adopting pip freeze on this branch to avoid the published manual quietly breaking.

Edit: See Ravenbrook Perforce changelist 199182 for an example of fixing a quietly broken manual build using pip freeze.

@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

The current manual has a sidebar like this:
image
sourced from manual/source/_templates . It does not appear in the Read the Docs theme. We must consider whether it is worth retaining and if so, how to do it. (It indicentally contains URLs to www.ravenbrook.com that are going out of date due to Git migration.)

@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

We need to examine the manual for cases where the MPS Sphinx extension and the MPS theme interact. For example, in the Bibiolgraphy, abstracts of papers now render with a strongly-highlighted exclamation-mark box like this
image
as opposed to a more gentle call-out like this in the MPS theme:
image

Of course we should try to use whatever intentional markup Sphinx provides for any purpose.

@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

Of course we should try to use whatever intentional markup Sphinx provides for any purpose.

In the case of abstracts, an admonition is used:

.. admonition:: Abstract

In solving this problem, we should:

  • look at conventions and extensions used in Sphinx for bibliographies
  • ensure the bibliography comes out well in e.g. LaTeX output from Sphinx
  • not break the bibliography at Memory Management Reference or any links to it from the web
  • do something not specific to any particular Sphinx theme

@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

Switching themes may fix #121 but this needs checking.

@rptb1
Copy link
Member Author

rptb1 commented Feb 27, 2023

Switching themes may fix #121 but this needs checking.

Checked. The layout problem does not appear with Sphinx 6.1.3 with the Read the Docs theme, the latest available from PyPI.

@rptb1 rptb1 linked an issue Feb 27, 2023 that may be closed by this pull request
@rptb1
Copy link
Member Author

rptb1 commented Feb 27, 2023

The content navigation bar on the left in the Read the Docs theme is incorrect.

  1. In the Read the Docs theme, the entry "Home" and subsequent entries seem to be renderings of the Memory Management Reference.
  2. The index does not appear.

Here are the entries using the Read the Docs theme:

Guide
Reference
Pool reference
Design
Old design

Bibliography
Memory Management Glossary
Index to source code
Memory Pool System Kit Open Source License
Contact us
Contributing to the MPS
Release notes

Introduction to memory management
Home
Frequently Asked Questions
Copyright
Acknowledgements

Compare to the way the contents appear in the MPS Sphinx theme:

Guide
Reference
Pool reference
Design
Old design

Bibliography
Memory Management Glossary
Index to source code
Memory Pool System Kit Open Source License
Contact us
Contributing to the MPS
Release notes

Index

@rptb1 rptb1 linked an issue Feb 27, 2023 that may be closed by this pull request
@rptb1
Copy link
Member Author

rptb1 commented Feb 27, 2023

The content navigation bar on the left in the Read the Docs theme is incorrect.

This is no doubt because the Read the Docs theme does not support this configuration

html_sidebars = {
'**': ['localtoc.html', 'relations.html', 'links.html', 'contact.html'],
}

See also How the table of contents displays.

@rptb1 rptb1 self-assigned this Feb 27, 2023
@rptb1
Copy link
Member Author

rptb1 commented Feb 27, 2023

The content navigation bar on the left in the Read the Docs theme is incorrect.

It looks like the Read the Docs theme is ignoring the index inserted here, and also the :hidden: instruction.

* :ref:`genindex`
.. toctree::
:hidden:
mmref/index
mmref-index
mmref/faq
mmref-copyright
mmref/credit

@rptb1
Copy link
Member Author

rptb1 commented Feb 28, 2023

It looks like the Read the Docs theme is ignoring the index inserted here

This can be solved by extending a Jinja template layout.html, as described here https://stackoverflow.com/a/37843854 .

@rptb1
Copy link
Member Author

rptb1 commented Feb 28, 2023

This branch is in danger of breaking the build of the Memory Management Reference described in #165 (comment) .

@rptb1 rptb1 linked an issue Mar 2, 2023 that may be closed by this pull request
@rptb1
Copy link
Member Author

rptb1 commented Mar 5, 2023

It looks like the Read the Docs theme is ignoring [...] the :hidden: instruction.

Fixed in a708cdb

@rptb1
Copy link
Member Author

rptb1 commented Mar 5, 2023

This can be solved by extending a Jinja template layout.html, as described here https://stackoverflow.com/a/37843854 .

Fixed in 454fc27

@rptb1 rptb1 requested a review from thejayps March 5, 2023 16:06
@thejayps
Copy link
Contributor

executing proc.review.log

Start time 1610

@rptb1 new.M2: What was found in item 7 also applies to everything under master
@UNAA008 new.m In response to m2 by @thejayps , convention is also to have a magnifier for search, we don't have one

@rptb1 raises new.minor.imp Git wont automatically stamp our files with a source control revision ID so we fail to meet rule.genric.ident. There's other sorts of ident (eg product document) that is in a way more important

@rptb1 new.m in "contributing to the mps" the text is stale. We could do better eg reference to the branch.

logging finised at 1649

@rptb1
Copy link
Member Author

rptb1 commented Oct 20, 2023

7. M. The manual published at https://www.ravenbrook.com/project/mps/master/manual/html/ is out of date and this will get worse. Needs to be replaced by a redirect?

Just noting that this is relevant to issue #98 in that the whole tree below https://www.ravenbrook.com/project/mps/ is a manifestation of the Perforce project, and much of it should be redirected. Solving this will also partly resolve #200 .

@rptb1 new.M2: What was found in item 7 also applies to everything under master

Raise: This note effectively add this to #98 .

@rptb1
Copy link
Member Author

rptb1 commented Oct 20, 2023

  1. M. The manual published at https://www.ravenbrook.com/project/mps/master/manual/html/ is out of date and this will get worse. Needs to be replaced by a redirect?

Just noting that this is relevant to issue #98 in that the whole tree below https://www.ravenbrook.com/project/mps/ is a manifestation of the Perforce project, and much of it should be redirected. Solving this will also partly resolve #200 .

Fixed in Perforce changelist 199686 by introducing a redirection in the web server.

@rptb1
Copy link
Member Author

rptb1 commented Oct 20, 2023

c2. The output contains ads "Ads by EthicalAds"

Fix: Subscribed Ravenbrook to "Gold Membership" of Read the Docs. The ad status of projects can be managed at https://readthedocs.org/accounts/gold/projects/ . I have disabled ads on the MPS.

@rptb1
Copy link
Member Author

rptb1 commented Oct 20, 2023

m1. Search doesn't have case sensitive option on phone (eg, search for "Pin" found "MutatorContextCanSte_pIn_struction")

Raise: readthedocs/sphinx_rtd_theme#1534

@rptb1
Copy link
Member Author

rptb1 commented Oct 20, 2023

m2. Search doesn't appear on phone unless you select the "burger" (but maybe this is appropriate and expected by phone users)
@UNAA008 new.m In response to m2 by @thejayps , convention is also to have a magnifier for search, we don't have one

Reject: It is appropriate, I think. You can also rotate the phone :)

@rptb1
Copy link
Member Author

rptb1 commented Oct 20, 2023

m1
requirements.pip header comment names the file as requirements.txt

Fix: Corrected in 5d27364 .

@rptb1
Copy link
Member Author

rptb1 commented Oct 20, 2023

c1. The pull request could do more to explain exactly how it fixes #121

Reply: Discussed during review, but documenting here: We don't know exactly what is wrong with our homegrown theme in #121 and it would be wasteful to debug it, since we've chosen to switch to the Read the Docs theme. In short, #121 is fixed by using a community-maintained up-to-date theme.

@rptb1
Copy link
Member Author

rptb1 commented Oct 20, 2023

Q1
Do we need a different mechanism to implement rule.generic.ident ?

Raise: #98 (comment)

@rptb1
Copy link
Member Author

rptb1 commented Oct 20, 2023

@rptb1 new.m in "contributing to the mps" the text is stale. We could do better eg reference to the branch.

Raise: This is already pending as part of #123 .

@rptb1
Copy link
Member Author

rptb1 commented Oct 20, 2023

Executing proc.review.edit.

  1. Start time 12:15.
  2. Written responses submitted to Switch the MPS manual to using the Read the Docs theme #166 .
  3. End time 14:46.

Copy link
Contributor

@thejayps thejayps left a comment

Choose a reason for hiding this comment

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

"Approving" after checking edits as requested by @rptb1

ready for exit

@thejayps
Copy link
Contributor

performing proc.review.exit

Start time 1830

Transgressions:

T1 exit.universal.edit: "m3. "Abstract" in bibliography appears with a "!" like a warning" - there is no action recorded

T2 exit.universal.imp: "new.minor.imp Git wont automatically stamp our files with a source control revision ID so we fail to meet rule.genric.ident. There's other sorts of ident (eg product document) that is in a way more important" - there is no action recorded

T3: Fixed: exit.universal.record estimation of remaining defects is missing. Estimating 1M remains, as 2M were found

(not a transgression) exit.universal.rates - rates were in danger of being too fast as the estimation of LOC was based only on the diffs. However, relevant checkers took more time and then reported having enough time to do the checking properly

Exit paused at 1847 while transgressions T1-2 discussed

@rptb1
Copy link
Member Author

rptb1 commented Oct 20, 2023

T2 exit.universal.imp: "new.minor.imp Git wont automatically stamp our files with a source control revision ID so we fail to meet rule.genric.ident. There's other sorts of ident (eg product document) that is in a way more important" - there is no action recorded

Included in #166 (comment) but I failed to link it.

T1 exit.universal.edit: "m3. "Abstract" in bibliography appears with a "!" like a warning" - there is no action recorded

Genuinely overlooked that one. I'll see if there's some better markup.

@rptb1
Copy link
Member Author

rptb1 commented Oct 21, 2023

T1 exit.universal.edit: "m3. "Abstract" in bibliography appears with a "!" like a warning" - there is no action recorded

Genuinely overlooked that one. I'll see if there's some better markup.

Reject: Cosmetic and too expensive to fix. I've investigated how this comes about and overriding it would be tricky, fragile, and costly. The markup is semantically probably correct (a custom admonition). We may come up with something later in passing.

The offending bit of source code is here https://github.com/readthedocs/sphinx_rtd_theme/blob/8ce23cec96f628ac0d29737bf1cf8cc2e750f068/src/sass/_theme_rst.sass#L137

@rptb1
Copy link
Member Author

rptb1 commented Oct 21, 2023

4. m. https://memory-pool-system--166.org.readthedocs.build/en/166/glossary/index.html It's hard to visually separate the terms in the navigation of the glossary.

I'm concerned that I missed this, and @thejayps didn't find it at exit. It suggests that proc.review.log isn't producing a findable log.

@rptb1
Copy link
Member Author

rptb1 commented Oct 21, 2023

4. m. https://memory-pool-system--166.org.readthedocs.build/en/166/glossary/index.html It's hard to visually separate the terms in the navigation of the glossary.

Fixed: Introduced punctuation and spacing in ebeee66

@rptb1
Copy link
Member Author

rptb1 commented Oct 21, 2023

I believe that concludes proc.review.exit.fix so merge is pending a pass from @thejayps .

@thejayps
Copy link
Contributor

exit.pass

@rptb1 rptb1 merged commit e2f9497 into master Oct 23, 2023
15 checks passed
@rptb1
Copy link
Member Author

rptb1 commented Oct 23, 2023

Executing proc.merge.pull-request.

  1. Start time 10:07.
  2. Checklist: The test cases for the issues are not automated: Manual is hard to use on a phone #185 is subjective, The manual lacks a search #157 is UI (though we could conceivably automate), The manual can't be used when built with Sphinx >= 5 #121 is a build failure.
  3. Skipping steps to do with Git Fusion.
  4. End time 10:14.

@rptb1
Copy link
Member Author

rptb1 commented Oct 23, 2023

3. Skipping steps to do with Git Fusion.

We should get on with #228 to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Problems with builds and build automation documentation essential Will cause failure to meet customer requirements. Assign resources.
Projects
None yet
3 participants