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

Enable docs build with sphinx #1517

Merged
merged 13 commits into from
Aug 26, 2021
Merged

Conversation

sechkova
Copy link
Contributor

@sechkova sechkova commented Aug 9, 2021

Fixes #1482

Description of the changes being introduced by the pull request:

  • Adds .rst source files for building documentation with sphinx. The two mandatory files are conf.py containing the build configuration and the master doc file index.rst.
  • Sphinx uses autodoc to automatically include docstrings. autodoc imports the modules and needs TUF installed in the environment.
  • Adds a tox environment for the docs build
  • Adds a CI build

The following command will generate the documentation from the source files in an html format:
tox -e docs
or it can be build manually in an environment with TUF installed with:
sphinx-build -b html docs/sphinx/source docs/sphinx/build/html

Configuring a build with sphinx is enough for the documentation to be imported built and published on readthedocs.
This would require a readthedocs account for the project which can be further configured from the UI and a .readthedocs.yaml file.
This is a demo project that I have created from my fork and this PR's branch:
https://tuf-sechkova.readthedocs.io/en/latest/

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@jku
Copy link
Member

jku commented Aug 10, 2021

  • Adds a CI build

What's the advantage? Can the build fail somehow?

@sechkova
Copy link
Contributor Author

  • Adds a CI build

What's the advantage? Can the build fail somehow?

It can fail in case of wrong configuration (cannot resolve paths to modules, dependencies etc.)
I was actually expecting the build to fail in case of wrong directives or syntax but it only outputs warnings by default. So we can set a flag (- W) to treat warnings as errors unless this feels too strict? Docs are generated anyway in case of warnings, they need to be checked visually if something wasn't rendered correctly.

@sechkova
Copy link
Contributor Author

sechkova commented Aug 11, 2021

After discussing with @jku, I created this readthedocs project linked to TUF's github page and added us both as maintainers: https://theupdateframework.readthedocs.io/en/latest/
Next steps for publishing the documentation would be:

  • organization-level access needed by readthedocs OAuth App (which I requested through github, so someone already has a mail in their inbox) .
  • configuring the project (.readthedocs.yaml to replace the UI settings). c65b299
  • adding additional maintainers

@sechkova sechkova force-pushed the configure-sphinx branch 2 times, most recently from 3e9738e to c65b299 Compare August 11, 2021 14:46
docs/sphinx/source/index.rst Outdated Show resolved Hide resolved
docs/sphinx/source/index.rst Outdated Show resolved Hide resolved
docs/sphinx/source/index.rst Outdated Show resolved Hide resolved
docs/sphinx/source/tuf.api.rst Outdated Show resolved Hide resolved
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

Reviewing documentation like this is really difficult... I've left some comments now, not sure how useful.

I think the build changes look ok, but I'm not convinced the CI addition is useful: as you said the doc changes almost always need a human reviewer, it's not really possible to "test them in CI" -- I'd love to get docs generated per PR so they could be reviewed but that's probably asking a lot.

@jku
Copy link
Member

jku commented Aug 17, 2021

Spent a moment thinking about the top-level documentation. I think as a short/mid term goal our top level page should have these links:

  • API Reference (this PR mostly)
  • Installation (use current INSTALLATION.rst?)
  • Getting Started (a document that uses example code from docs: tutorial/example for ngclient Updater #1518, similar to current QUICKSTART.md)
  • Contribution Guidelines (use current CONTRIBUTORS.rst?)

@joshuagl
Copy link
Member

  • organization-level access needed by readthedocs OAuth App (which I requested through github, so someone already has a mail in their inbox) .

Granted.

@jku
Copy link
Member

jku commented Aug 18, 2021

this is still missing the __init__() docs (this is some sphinx thing where it thinks classes only have class or init documentation). autoclass_content = 'both' may work

@jku
Copy link
Member

jku commented Aug 20, 2021

I've worked on this branch at https://github.com/jku/tuf/tree/configure-sphinx:

  • some content improvements
  • moved the sphinx root to docs/ (to be able to use the existing docs in the published ones)
  • included "Installation" and "Instructions for Contributors" in the sphinx documentation.
  • mostly removed mentions of "modern implementation" and "legacy implementation": just talk about the API we have

I plan on having a final look through the content and then making a PR (or if teodora is back by then letting her merge my changes to this branch)

@sechkova
Copy link
Contributor Author

Force pushed the additional commits from @jku's branch + the CI step commit is now removed.

Add .rst source files for building documentation with
'sphinx'. The two mandatory files are conf.py containing
the build configuration and the master doc file index.rst.

Sphinx uses 'autodoc' to automatically include docstrings.
'autodoc' imports the modules and needs TUF installed in
the environment.

The following command will generate the documentation from the
source files in an html format:

`sphinx-build -b html docs/sphinx/source docs/sphinx/build/html`

Signed-off-by: Teodora Sechkova <[email protected]>
 - New 'docs' environment in tox enables
   building the sphinx documentation in isolation.
 - New requirements-docs.txt.

Signed-off-by: Teodora Sechkova <[email protected]>
Signed-off-by: Teodora Sechkova <[email protected]>
Fix docstrings which triggered warnings from sphinx-build.

Signed-off-by: Teodora Sechkova <[email protected]>
@sechkova
Copy link
Contributor Author

Rebased to get the pylint fix from develop.

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

LGTM but since half of it is my work maybe someone else should approve as well :)

Let's try to get the readthedocs PR build working before merging (so it would be easier to review the work), but in case that doesn't happen building locally is easy:
tox -e docs
docs/build/html now contains the html docs

EDIT: readthedocs build now works: see list of checks below

Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

This is a really great start, nice work @sechkova and @jku.

I left several comments in-line. I don't mind if we address these here or in a future PR, but please update the comments here to indicate which.

.readthedocs.yaml Outdated Show resolved Hide resolved
::
Assuming you trust `the maintainer's PGP key
<https://github.com/theupdateframework/tuf/blob/develop/docs/MAINTAINERS.txt>`_,
the detached ASC signature can be downloaded and verified. For example::

$ gpg --verify securesystemslib-0.10.8.tar.gz.asc
gpg: assuming signed data in 'securesystemslib-0.10.8.tar.gz'
Copy link
Member

Choose a reason for hiding this comment

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

While we are editing this document, should we change this example to be a tuf release and an active maintainer?

Copy link
Member

Choose a reason for hiding this comment

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

this one is from last release

$ gpg --verify tuf-0.17.0.tar.gz.asc 
gpg: assuming signed data in 'tuf-0.17.0.tar.gz'
gpg: Signature made Thu 25 Feb 2021 13:42:50 EET
gpg:                using RSA key 08F3409FCF71D87E30FBD3C21671F65CB74832A4
gpg: Good signature from "Joshua Lock (GPG on YubiKey) <[email protected]>" [unknown]

it does unfortunately continue with

gpg: WARNING: This key is not certified with a trusted signature!
gpg:          There is no indication that the signature belongs to the owner.
Primary key fingerprint: 08F3 409F CF71 D87E 30FB  D3C2 1671 F65C B748 32A4

even though I fully trust your key. I hate gpg

Copy link
Member

Choose a reason for hiding this comment

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

😩

This was probably scope creep for this PR anyway.

docs/api/api-reference.rst Outdated Show resolved Hide resolved
docs/api/api-reference.rst Outdated Show resolved Hide resolved
docs/api/tuf.api.rst Outdated Show resolved Hide resolved
docs/api/tuf.ngclient.fetcher.rst Outdated Show resolved Hide resolved
docs/api/tuf.ngclient.rst Outdated Show resolved Hide resolved

* :doc:`tuf.ngclient.updater` implements the client update workflow
* :doc:`tuf.ngclient.config` provides optional configuration for the updater
* :doc:`tuf.ngclient.fetcher` can be used for optional low-level network I/O control
Copy link
Member

Choose a reason for hiding this comment

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

should we mention the default fetcher implementation and be more explicit about when users might want to implement this interface?

Copy link
Contributor Author

@sechkova sechkova Aug 25, 2021

Choose a reason for hiding this comment

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

I will add this to another issue for further improving the documentation -> #1543

sechkova and others added 3 commits August 25, 2021 17:05
Add a configuration file for Read The Docs.

Signed-off-by: Teodora Sechkova <[email protected]>
* Improve content
* Make ngclient Updater __init__() visible in docs
* Remove "legacy implementation" (except for the note on API stability)

Signed-off-by: Jussi Kukkonen <[email protected]>
* Remove link to outdated roadmap
* Link to maintainers file in the same way as two lines earlier
* Fix formatting issues with code blocks

These fixes allow the installation rst to be used from sphinx sources
and from docs root.

Signed-off-by: Jussi Kukkonen <[email protected]>
Jussi Kukkonen and others added 5 commits August 25, 2021 17:05
This allows using existing documentation in the published documentation
without
* moving the existing docs (which would break external links)
* tricks like symlinks that create issues with relative links

Put the api reference files into a subdirectory to avoid polluting the
main docs/ directory.

Include "Installation" and "Instructions for Contributors" in the
published documentation.

Signed-off-by: Jussi Kukkonen <[email protected]>
Write a bit more about the two modules, hide the actual TOC to not
repeat (and not have sphinx complain about missing items in TOC)

Signed-off-by: Jussi Kukkonen <[email protected]>
Signed-off-by: Jussi Kukkonen <[email protected]>
Remove/add new lines at the end of file.

Signed-off-by: Teodora Sechkova <[email protected]>
@sechkova
Copy link
Contributor Author

Closing to test RTD webhooks.

@sechkova sechkova closed this Aug 25, 2021
@sechkova sechkova reopened this Aug 25, 2021
@sechkova sechkova closed this Aug 25, 2021
@sechkova sechkova reopened this Aug 25, 2021
@jku jku closed this Aug 25, 2021
@jku jku reopened this Aug 25, 2021
Improve class DelegatedRole docstring in order to be rendered
correctly in the documentation built with sphinx and autodoc.

Signed-off-by: Teodora Sechkova <[email protected]>
@sechkova sechkova mentioned this pull request Aug 26, 2021
4 tasks
@jku
Copy link
Member

jku commented Aug 26, 2021

Let's see what happens 🤞

@jku jku merged commit 5f0445d into theupdateframework:develop Aug 26, 2021
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.

Docs: Enable continuous documentation build
3 participants