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

Fix ReadTheDocs build #321

Merged
merged 5 commits into from Jan 25, 2021
Merged

Fix ReadTheDocs build #321

merged 5 commits into from Jan 25, 2021

Conversation

twm
Copy link
Contributor

@twm twm commented Jan 24, 2021

  • Use Python 3.7 instead of 2.7 to build the documentation
  • Move the Sphinx dependency to the docs extra

I've also enabled PR builds in the ReadTheDocs configuration, so the build status should be reported here.

Closes #296.

@twm twm added the bug label Jan 24, 2021
@twm twm marked this pull request as ready for review January 24, 2021 23:12
@twm twm requested a review from a team January 24, 2021 23:12
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

I love it :) Thanks.

Only minor comments.

Feel free to address them as you wish and merge without another round of review.

version: 2

sphinx:
fail_on_warning: false
Copy link
Member

Choose a reason for hiding this comment

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

This is ok. If you set it to True, RTD will report a failure if a warning is present, but will not generate the HTML.

I prefer to have the HTML on RTD even on warnings... so that you can check the result.
And have a separate GiHub action that will fail on errors.

I see there is an existing 'docs' jobs it's green even with one warning

 /home/runner/work/treq/treq/docs/api.rst:80: WARNING: more than one target found for cross-reference 'request': treq.request, treq.client.HTTPClient.request, treq.testing.treq.testing.StubTreq.request, treq.testing.RequestTraversalAgent.request

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 agree, if only I could figure out how to fix that warning.


formats:
- pdf
- epub
Copy link
Member

Choose a reason for hiding this comment

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

Is epub used?

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 was enabled in the RTG GUI before, so I left it enabled.


pip install treq[dev]
tox -e py38-twisted_latest
Copy link
Member

Choose a reason for hiding this comment

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

just a minor comment.... feel free to ignore as I might be wrong :)

It would be nice to name the tox test envs like py38-test-twisted_latest``. In this way, you can call tox -e test-twisted_latest` and it will run with your default python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, you can do tox -e twisted_latest to run with your default Python as-is. The default commands are to test.


Build docs::

tox -e docs

To do it all::

tox -p
Copy link
Member

Choose a reason for hiding this comment

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

On my tox version I need to explicitly specify all or a number of parallel builds

$ tox --version
3.13.2
Suggested change
tox -p
tox -p all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You should update your Tox. :) I don't need to pass anything on 3.20.1; it defaults to the number of CPU on my machine. all is semantically different: it runs all the jobs in parallel, regardless of available resources.

Copy link
Member

Choose a reason for hiding this comment

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

@@ -53,6 +51,7 @@ commands =
check-manifest

[testenv:docs]
extras = docs
changedir = docs
commands =
sphinx-build -b html . html
Copy link
Member

Choose a reason for hiding this comment

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

My suggestion is fail on warnings. In this way the docs should be "clean".

Suggested change
sphinx-build -b html . html
sphinx-build -W --keep-going -b html . html

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 100% agree, but I've never been able to figure out how to clear the remaining warning. It has been a while since I looked at it, but IIRC it is implying a cross-reference from an ivar (which makes no sense, but I've seen similar issues on other projects).

CONTRIBUTING.rst Show resolved Hide resolved
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
@twm
Copy link
Contributor Author

twm commented Jan 25, 2021

Thank you for the review @adiroiban! I filed #322 to follow up on the Sphinx warning. I'll go ahead and merge this once it greens up since doing so will fix the failing RTD check on another PR.

@twm twm merged commit 321fb6e into twisted:master Jan 25, 2021
@twm twm deleted the rtd-conf branch January 25, 2021 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable readthedocs PR builds and move readthedocs config into .readthedocs.yml
3 participants