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

#10066 Update pydoctor version and remove Twisted pydoctor custom code that is now available as standard features. #11578

Merged
merged 24 commits into from
Jul 29, 2022

Conversation

adiroiban
Copy link
Member

Scope and purpose

Fixes #10066

If the epydoc API markup is invalid, the CI is happy and green.

We need to update the CI to fail on pydoctor build errors.
This should help make sure the API doc is valid and sane :)

As part of this PR I have updated the pydoctor version.

Also, instead of using the custom python script to call pydoctor, this calls pydoctor directly from tox.
I hope that in this way it should be easier to understand what is going on there, with fewer redirection layers.

Contributor Checklist:

  • I ran tox -e lint to format my patch to meet the Twisted Coding Standard
  • I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
  • The title of the PR starts with the associated issue number (without the # character).
  • I have updated the automated tests and checked that all checks for the PR are green.
  • The merge commit will use the below format
    The first line is automatically generated by GitHub based on PR ID and branch name.
    The other lines generated by GitHub should be replaced.
Merge pull request #123 from twisted/4356-branch-name-with-issue-id

Author: adiroiban
Reviewer: 
Fixes #10066

Update pydoctor version.
Fail CI for a PR on pydoctor build errors.

@adiroiban adiroiban changed the title 10066 Update pydoctor and fail CI on pydoctor errors 10066 Update pydoctor and fail CI on pydoctor errors. Jul 12, 2022
tox.ini Outdated

extras = dev_release
commands = {toxinidir}/bin/admin/build-apidocs {toxinidir}/src/ apidocs
commands =
pydoctor --quiet \
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing --template-dir option.

setup.cfg Outdated Show resolved Hide resolved
@adiroiban
Copy link
Member Author

Many thanks for the review.

I will try to reduce the things changed in this PR.

For now, I just want to update the pydoctor version and see that CI fails on pydoctor errors.

In a separate ticket we can look at fixing the pydoctor errors and also cleaning the things that are no longer needed.

I will see how hard is to fix the current pydoctor errors.
I would like to fix them in this PR, so that we can have a green CI run.

The extra cleanup can be done in a separate PR.

@tristanlatr
Copy link
Contributor

The build is currently failing with error: could not import module twisted.python._pydoctor.

I'de suggest to simply drop the custom twisted.python._pydoctor.TwistedSystem because it's not compatible with pydoctor 22.7 anyway. Then, in the later PR, adjust it and we can re-enable it (it will only contains the inventory tweaks at this point).

@adiroiban
Copy link
Member Author

thanks @tristanlatr

I have updated twisted/twisted-contributors team. Not sure why you weren't there.

If you have time. Can you make the required changes to get pydoctor working ?

Thanks

@tristanlatr
Copy link
Contributor

Alright, but there is one thing that bother me. It’s like the _pydoctor module is not included in the default dist build. So when we’ll need to re enable the custom system again, this module needs to be installed. You know what I mean ?

@adiroiban
Copy link
Member Author

It’s like the _pydoctor module is not included in the default dist build.

I don't know why it is not included.
But I think that is a bug.

In the code, I see that the intention is to have it included.

twisted/MANIFEST.in

Lines 40 to 44 in fc20692

# Include our docs templates
recursive-include src/twisted/python/_pydoctortemplates *.html
# Include all modules, even on a Python we're not installing for
recursive-include src/twisted *.py

python setup.py bdist_wheel

adding 'twisted/python/_pydoctor.py'

and it was installed in my environment


@tristanlatr
Copy link
Contributor

Ok, maybe the twisted package is not correctly installed in the tox environment then.

@tristanlatr
Copy link
Contributor

tristanlatr commented Jul 28, 2022

The pydoctor build is fixed, I believe. Though, I haven't read the logs in the CI, but tox -e apidocs runs on my local device.

This leaves the warnings to fix:

/Users/tristan/Documents/twisted/src/twisted/test/test_adbapi.py:362: Assignment to constant "DB_DIR" inside an instance is ignored, this value will not be part of the docs.
/Users/tristan/Documents/twisted/src/twisted/trial/test/test_assertions.py:1610: Invalid call to twisted.python.deprecate.deprecated(), first argument should be a call to incremental.Version()
/Users/tristan/Documents/twisted/src/twisted/trial/test/test_assertions.py:1618: Invalid call to twisted.python.deprecate.deprecated(), first argument should be a call to incremental.Version()
/Users/tristan/Documents/twisted/src/twisted/trial/_dist/functional.py:34: bad docstring: Improper paragraph indentation.
/Users/tristan/Documents/twisted/src/twisted/web/iweb.py:674: Cannot find link target for "__init__"
/Users/tristan/Documents/twisted/src/twisted/web/iweb.py:686: Cannot find link target for "__init__"
/Users/tristan/Documents/twisted/src/twisted/conch/checkers.py:117: Cannot find link target for "twisted.conch.checkers.pwd.struct_passwd", resolved from "pwd.struct_passwd"
/Users/tristan/Documents/twisted/src/twisted/conch/checkers.py:136: Cannot find link target for "spwd.struct_spwd" (you can link to external docs with --intersphinx)
/Users/tristan/Documents/twisted/src/twisted/conch/checkers.py:41: Cannot find link target for "twisted.conch.checkers.pwd.struct_passwd", resolved from "pwd.struct_passwd"
/Users/tristan/Documents/twisted/src/twisted/conch/checkers.py:86: Cannot find link target for "twisted.conch.checkers.pwd.struct_passwd", resolved from "pwd.struct_passwd"
/Users/tristan/Documents/twisted/src/twisted/conch/checkers.py:86: Cannot find link target for "spwd.struct_spwd" (you can link to external docs with --intersphinx)
/Users/tristan/Documents/twisted/src/twisted/internet/tcp.py:918: Cannot find link target for "errno.WSAEMFILE", resolved from "EMFILE" (you can link to external docs with --intersphinx)
/Users/tristan/Documents/twisted/src/twisted/logger/_logger.py:179: Cannot find link target for "Failure"
/Users/tristan/Documents/twisted/src/twisted/spread/jelly.py:211: Cannot find link target for "twisted.spread.flavors.setUnjellyableForClass", resolved from "setUnjellyableForClass"
/Users/tristan/Documents/twisted/src/twisted/web/_flatten.py:454: Cannot find link target for "Deferred"
/Users/tristan/Documents/twisted/src/twisted/web/_flatten.py:462: Cannot find link target for "Deferred"
/Users/tristan/Documents/twisted/src/twisted/web/_flatten.py:477: Cannot find link target for "Deferred"
/Users/tristan/Documents/twisted/src/twisted/web/_template_util.py:1069: Cannot find link target for "IRequest"
/Users/tristan/Documents/twisted/src/twisted/web/_template_util.py:1070: Cannot find link target for "IRenderable"
these 2 objects' docstrings contain syntax errors:
    twisted.python._pydoctor
    twisted.trial._dist.functional.takeWhile

Let's note that pydoctor has this known bug: twisted/pydoctor#295 which might explain some of the errors.

Also: twisted.python.deprecate warnings are still triggered for twisted/trial/test/test_assertions.py:1610 even if the module is hidden because the warning comes from the AST parsing step, which is done for all source, no matter is there's hidden, unlike the docstring parsing.

@adiroiban
Copy link
Member Author

Ok, One test failing

Traceback (most recent call last):
  File "/home/runner/work/twisted/twisted/.tox/alldeps-withcov-posix/lib/python3.8/site-packages/twisted/trial/runner.py", line 688, in loadByName
    return self.suiteFactory([self.findByName(name, recurse=recurse)])
  File "/home/runner/work/twisted/twisted/.tox/alldeps-withcov-posix/lib/python3.8/site-packages/twisted/trial/runner.py", line 451, in findByName
    obj = reflect.namedModule(searchName)
  File "/home/runner/work/twisted/twisted/.tox/alldeps-withcov-posix/lib/python3.8/site-packages/twisted/python/reflect.py", line 156, in namedModule
    topLevel = __import__(name)
  File "/home/runner/work/twisted/twisted/.tox/alldeps-withcov-posix/lib/python3.8/site-packages/twisted/python/test/test_pydoctor.py", line 18, in <module>
    from twisted.python._pydoctor import TwistedSphinxInventory, TwistedSystem
  File "/home/runner/work/twisted/twisted/.tox/alldeps-withcov-posix/lib/python3.8/site-packages/twisted/python/_pydoctor.py", line 17, in <module>
    from pydoctor import astbuilder, model, zopeinterface  # type: ignore[import]
builtins.ImportError: cannot import name 'zopeinterface' from 'pydoctor' (/home/runner/work/twisted/twisted/.tox/alldeps-withcov-posix/lib/python3.8/site-packages/pydoctor/__init__.py)

twisted.python.test.test_pydoctor

@tristanlatr
Copy link
Contributor

Yes, this is normal, pydoctor 22.7 introduced a breaking change in this regards, but we never had a stable API, that was clear from the beginning.

I'll just cleanup _pydoctor.py is that OK with you.

@adiroiban
Copy link
Member Author

I'll just cleanup _pydoctor.py is that OK with you.

Yes. Thanks.

Feel free to push anything.

I can then see how to break this PR to get these changes reviewed and merged.

tristanlatr and others added 2 commits July 28, 2022 14:06
Newer versions of pydoctor includes support for twisted.python.deprecate and allows to tweak the privacy of some objects.
@tristanlatr
Copy link
Contributor

tristanlatr commented Jul 28, 2022

The tests are now all successful. The only thing that bothers me is that we’re not actually using the custom system, we just have tests for it.

And more, I don’t think it’s required anymore, meaning zope has fixed it’s wrong inventory links: because I can’t see any warnings that refers to the zope api. So I believe we can trash it all together.

@adiroiban
Copy link
Member Author

The only thing that bothers me is that we’re not actually using the custom system, we just have tests for it.

We can remove the custom system code if not used.

less is more :)

@tristanlatr
Copy link
Contributor

Ok, I believe it's pretty clean now.

@tristanlatr
Copy link
Contributor

tristanlatr commented Jul 28, 2022

I've enabled the readthedocs theme, I believe that was the plan :) But tell me if I should use the classic theme

@adiroiban adiroiban changed the title 10066 Update pydoctor and fail CI on pydoctor errors. #10066 Update pydoctor and fail CI on pydoctor errors. Jul 29, 2022
@adiroiban
Copy link
Member Author

These changes are awesome.

I love the config file.


The theme is also ok.

Just some feedback for the theme.

There are now 2 main links:

  • Twisted main site
  • API docs index

image

Maybe instead of Twisted main site the link should be to the narrative docs base - like https://twisted--11578.org.readthedocs.build/en/11578/index.html this should match RTD links.


Now, in order to have this merged ASAP, as you mentioned on the issue, I think that is best to reduce the scope of this PR.

Instead of pydoctor upgrade + ci fail, we should use this PR only to upgrade the pydoctor version.

I/we can look to fix the pydoctor errors in a separate PR.

Copy link
Member

@glyph glyph left a comment

Choose a reason for hiding this comment

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

LGTM. Love to see all this unnecessary support code getting zapped.

Comment on lines -228 to -241
"--project-name=Twisted",
"--project-url=https://twistedmatrix.com/",
"--system-class=twisted.python._pydoctor.TwistedSystem",
"--docformat=epytext",
"--intersphinx=https://docs.python.org/3/objects.inv",
"--intersphinx=https://cryptography.io/en/latest/objects.inv",
"--intersphinx=https://pyopenssl.readthedocs.io/en/stable/objects.inv",
"--intersphinx=https://hyperlink.readthedocs.io/en/stable/objects.inv",
"--intersphinx=https://twisted.org/constantly/docs/objects.inv",
"--intersphinx=https://twisted.org/incremental/docs/objects.inv",
"--intersphinx=https://python-hyper.org/projects/hyper-h2/en/stable/objects.inv",
"--intersphinx=https://priority.readthedocs.io/en/stable/objects.inv",
"--intersphinx=https://zopeinterface.readthedocs.io/en/latest/objects.inv",
"--intersphinx=https://automat.readthedocs.io/en/latest/objects.inv",
Copy link
Member

Choose a reason for hiding this comment

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

yay for getting this to be declarative data in setup.cfg and not stuffed into command-line args!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

@@ -1,19 +0,0 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

Love to see all the ./build/admin scripts get replaced with tox environments. Much more discoverable and less error-prone.

Comment on lines +226 to +250
HIDDEN:twisted.words.test
HIDDEN:twisted.web.test
HIDDEN:twisted.spread.test
HIDDEN:twisted.scripts.test
HIDDEN:twisted.runner.test
HIDDEN:twisted.python.test
HIDDEN:twisted.protocols.haproxy.test
HIDDEN:twisted.protocols.test
HIDDEN:twisted.positioning.test
HIDDEN:twisted.persisted.test
HIDDEN:twisted.pair.test
HIDDEN:twisted.names.test
HIDDEN:twisted.mail.test
HIDDEN:twisted.logger.test
HIDDEN:twisted.cred.test
HIDDEN:twisted.conch.test
HIDDEN:twisted.application.runner.test
HIDDEN:twisted.application.twist.test
HIDDEN:twisted.application.test
HIDDEN:twisted._threads.test
HIDDEN:twisted.trial._dist.test
HIDDEN:twisted.trial.test
HIDDEN:twisted.internet.test
HIDDEN:twisted.test.*
PUBLIC:twisted.test.proto_helpers
Copy link
Member

Choose a reason for hiding this comment

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

Beautiful. Self-documenting!

@glyph
Copy link
Member

glyph commented Jul 29, 2022

Auto-merge set so we can land this as soon as static check and api doc build issues are corrected. (Looks like maybe an intersphinx thing for the docs build?)

@tristanlatr
Copy link
Contributor

Looks like maybe an intersphinx thing for the docs build?

No, it looks like months of not checking the pydoctor warnings ;-)

should we fix the docstring links in this PR?

@adiroiban adiroiban changed the title #10066 Update pydoctor and fail CI on pydoctor errors. #10066 Update pydoctor version and remove Twisted pydoctor custom code that is now available as standard features. Jul 29, 2022
@adiroiban
Copy link
Member Author

Ok. The plan is to land this PR asap.

The remaining pydoctor "compilation" error should be fixed as a followup in #11590

Those errors already exist in trunk, so they are not errors introduced in this PR.

Is just that with this PR, pydoctor is detecting more errors.

@glyph glyph merged commit ff2ea61 into trunk Jul 29, 2022
@glyph glyph deleted the 10066-pydoctor-and-docs-ci branch July 29, 2022 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update pydoctor version and remove Twisted pydoctor custom code that is now available as standard features.
4 participants