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

Bump RTD builder Python version #2817

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Oct 9, 2023

This is erroring on master due to (AFAICT) trio doing magic to make Sphinx realize link_to exists, but then using Python 3.12's Sphinx index to find it (which obviously doesn't work).

This does raise the interesting question of whether functions that are removed will show up in the output, as we cannot expect everyone to be using Python 3.12. I'm not sure what to do about this.

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a10f94b) 99.68% compared to head (2445a55) 99.68%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2817   +/-   ##
=======================================
  Coverage   99.68%   99.68%           
=======================================
  Files         117      117           
  Lines       17546    17546           
  Branches     3152     3152           
=======================================
  Hits        17490    17490           
  Misses         38       38           
  Partials       18       18           

@TeamSpen210
Copy link
Contributor

What might be better is to just hardcode the older URL for this specific link. I just disabled the link and added a deprecation warning in #2815...

@A5rocks
Copy link
Contributor Author

A5rocks commented Oct 10, 2023

Ack I didn't read that PR yet. Ideally we would use an old intersphinx inventory but still link to the latest version of the Python documentation... but I'm not sure if that's possible.

Personally I would super duper rather just build our docs on 3.12 but unfortunately RTD doesn't support that yet. I think your solution is more pragmatic though (while it's probably more correct to not have a mismatch... it's not worth linking to older docs). I have to think about this a bit xd

Copy link
Member

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

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

Looks simple enough, but I feel like @TeamSpen210's solution in #2815 might be better in the end, and then we aren't dependent on "has to run docs on py3.11 only"

@A5rocks
Copy link
Contributor Author

A5rocks commented Oct 10, 2023

I've gone ahead and submitted readthedocs/readthedocs.org#10808 which might help things along

@TeamSpen210
Copy link
Contributor

Figured out an even better solution. We can just add an event listener which manually adds the definition into intersphinx's list of objects, with a 3.11 URL. Actually that probably would work for most of the other entries in nitpick_ignore.

@A5rocks
Copy link
Contributor Author

A5rocks commented Oct 11, 2023

That sounds like a plan. I'm not comfortable enough with Sphinx to do that though... maybe you could? :P

@A5rocks A5rocks marked this pull request as draft October 12, 2023 06:52
@webknjaz
Copy link
Member

/home/docs/checkouts/readthedocs.org/user_builds/trio/checkouts/2817/trio/__init__.py:docstring of trio.Path:1: WARNING: py:meth reference target not found: pathlib.Path.link_to

Didn't it get removed? It was deprecated since 3.10.

@webknjaz
Copy link
Member

How about replacing the reference with symlink_to?

https://webknjaz.github.io/intersphinx-untangled/docs.python.org/

@webknjaz
Copy link
Member

What might be better is to just hardcode the older URL for this specific link.

FYI it is entirely possible to add several intersphinx entries against different python version docs.

@A5rocks A5rocks changed the title Attempt to fix Sphinx build Bump RTD builder Python version Oct 17, 2023
@A5rocks A5rocks marked this pull request as ready for review October 17, 2023 23:21
@A5rocks
Copy link
Contributor Author

A5rocks commented Oct 17, 2023

TBF this isn't really needed anymore due to fixes elsewhere, but this also (should, IIRC RTD releases on Tuesdays) bumps our builder to 3.12.

@TeamSpen210
Copy link
Contributor

We should wait until RTD does run on 3.12 - link_to will probably disappear at that point. What we could do is add some special case code to _path.py - if sphinx is in sys.modules or an environment var is set, unconditionally define the method?

@CoolCat467
Copy link
Member

Only one issue from read the docs build:

src/trio/__init__.py:docstring of trio.Path:1: WARNING: py:obj reference target not found: .

@A5rocks
Copy link
Contributor Author

A5rocks commented Dec 19, 2023

Yeah I'm not sure if I even want to merge this PR anymore because it brings up so many possible issues (such as old forwarded methods not appearing in documentation). I'll just set it to WIP for a bit at least.

@A5rocks A5rocks marked this pull request as draft December 19, 2023 06:55
@jakkdl
Copy link
Member

jakkdl commented Dec 23, 2023

(such as old forwarded methods not appearing in documentatio

old forwarded issues are relatively straightforward to add exceptions for in add_mapping in docs/source/conf.py now. Though Buffer is failing here now

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