Deprecate the custom RTD builders (except local media) #88

davidfischer · 2020-05-27T22:55:46Z

~~Removes~~ Deprecates the custom RTD-specific builders (readthedocs, readthedocsdirhtml, and readthedocssinglehtml) in favor of just using regular Sphinx builders.

In the 2.x branch, Sphinx made some changes (sphinx-doc/sphinx#6741) to their searchtools that caused problems with users who opt-out of Read the Docs' search improvements and use the dirhtml builder.

The changes in this PR:

~~Uses~~ Favors using default Sphinx builders instead (except for zipped media). Other extensions and added custom files will work on the default Sphinx builders.
Use a Sphinx extension and Sphinx event hooks to add additional JS/CSS files that RTD uses
~~This will require a simultaneous release on RTD to use the default builders~~ At a later date, we will add a feature flag to use default Sphinx builders instead of the custom builders but that change will be small (eg. here).
We should probably increment a ~~major~~ minor version of this package for releasing this

Refs: pallets/click#1513, pallets-eco/wtforms#599

- Uses default Sphinx builders instead (except for zipped media) - Use a Sphinx extension and events to add additional JS/CSS files - We should probably increment a major version for releasing this

ericholscher

This is definitely a solid improvement, but it's backwards incompatible in an annoying way. I don't really know how to handle shipping it without breaking things, but it's definitely going to break something.

ericholscher · 2020-05-27T23:09:21Z

readthedocs_ext/_templates/readthedocs-insert.html.tmpl

@@ -18,16 +18,17 @@ http://docs.readthedocs.org/en/latest/canonical.html

 <link rel="stylesheet" href="{{ rtd_css_url }}" type="text/css" />

-<script type="text/javascript" src="{{ pathto('_static/readthedocs-data.js', 1) }}"></script>
+<script type="application/json" id="READTHEDOCS_DATA">{{ rtd_data | tojson }}</script>


I'm a little worried about this possibly breaking things that users are depending on (eg. they are including this file themselves to get our data back out). I wonder if we should continue to build the file but not link it, at least for some time.

While it is possible that this will break things for some users, I don't think it will do so in a systemic way for many users (say by breaking a theme). We maintain the global variable which was set in readthedocs-data.js for backwards compatibility and given that some page and suffix data was modified in that global below, nobody could correctly read the variable without parsing both the HTML and JS.

Sure, but only the page was set in the HTML instead of the JS, so if they just were reading eg. the project or version, it would have worked fine. I think continuing to ship the file for now seems easy enough and minimal risk.

I will see if it's possibly to produce the file from an extension rather than from a builder. When producing the file, perhaps I'll add a note into the file noting that it is deprecated and will be removed.

Don't spend too much time if it's a pain, but shipping static files is pretty standard for extensions, so I think it should be simple enough.

I just want to use the same function where the inline JSON is generated so they're the same.

It's actually a little difficult to do this in a consistent way. I might skip it.

I want to write that file, but only once per build

I need the context which is in the html-page-context but I don't want to rewrite the same file for every page.

I can't just detect if the file exists because the file might be there if the environment is re-used.

ericholscher · 2020-05-27T23:12:02Z

readthedocs_ext/readthedocs.py

 ONLINE_BUILDERS = [
-    'readthedocs', 'readthedocsdirhtml', 'readthedocssinglehtml'
+    'html', 'dirhtml', 'singlehtml'


I am worried this is going to break some of our users customizations, eg something like this: https://github.com/Invoca/developer-docs/blob/a1b05592d95fbdfee2f2360a3c2da9c427669c4c/source/conf.py#L158

I didn't see a ton of these in a GH search, and it's never been a properly supported way of finding out if you're running on RTD, but it's definitely going to break :/

What's the alternative? If we want to use the regular Sphinx builders then people depending on us using a different builder are going to have something break on them.

Another thing, sphinx creates tags with the name of the builder, users may be relying on that too (but probably this one is easy to fix). I'm on the phone, but I'll link to the issue and sphinxs docs in the morning.

https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-only

The format and the name of the current builder (html, latex or text) are always set as a tag 4. To make the distinction between format and name explicit, they are also added with the prefix format_ and builder_, e.g. the epub builder defines the tags html, epub, format_html and builder_epub.
These standard tags are set after the configuration file is read, so they are not available there.

And users already depending on that readthedocs/readthedocs.org#4765 (comment)

If we want to perhaps we could deprecate the builders and roll using the stock ones out slowly. However I think we want to use the regular builders and not custom ones as much as possible.

The users in the linked issue should be ok since they're using .. only :: builder_html or readthedocs

Yea, I think there's no way around breaking this, but perhaps we should communicate it prior to doing it, but I don't know a great way to communicate it, since most users won't care and it's super deep and technical.

I don't think there's a great way to communicate it either. It was never exactly supported so trying to tell people that the undocumented thing they were relying on is changing is hard. One possibility if we're really concerned about this is to keep the custom builders and have a feature flag that determines whether somebody uses the stock builders or the custom ones.

Yea, I think we can probably just ship it and see what happens.

readthedocs_ext/readthedocs.py

ericholscher

I'd like to get this merged and shipped 👍

davidfischer · 2020-06-03T22:58:23Z

Sounds good. After merging, there's a few steps:

Release the latest readthedocs-sphinx-ext to PyPI
Try the newer version on one of our projects
Reference using the new version here which will roll it out widely

ericholscher · 2020-06-15T22:43:46Z

We could ship this this week, if you're ready @davidfischer

stsewd · 2020-06-15T23:15:34Z

Just a note here, when rebasing this PR, we need to make sure we are including the change from #85

davidfischer · 2020-06-16T00:01:44Z

We could ship this this week, if you're ready @davidfischer

It's ready and I'll try to merge the changes from #85 into here.

davidfischer · 2020-06-16T00:18:40Z

@stsewd if you want to review 6698995, I added the feature flag you added there into the new JSON.

Remove the custom RTD builders

c8c0b54

- Uses default Sphinx builders instead (except for zipped media) - Use a Sphinx extension and events to add additional JS/CSS files - We should probably increment a major version for releasing this

davidfischer requested a review from a team May 27, 2020 22:55

ericholscher reviewed May 27, 2020

View reviewed changes

davidfischer changed the title ~~Remove the custom RTD builders~~ Deprecate the custom RTD builders (except local media) May 28, 2020

Re-add the builders as deprecated

a9ac695

davidfischer mentioned this pull request May 29, 2020

Add feature flag to use the stock Sphinx builders readthedocs/readthedocs.org#7141

Merged

Merge branch 'master' into davidfischer/remove-custom-builders

0c08264

ericholscher approved these changes Jun 3, 2020

View reviewed changes

ericholscher mentioned this pull request Jun 5, 2020

Allow to disable server side search #85

Merged

davidfischer added 2 commits June 15, 2020 17:16

Move the feature flags added in #85

6698995

Merge branch 'master' into davidfischer/remove-custom-builders

0eb5114

stsewd approved these changes Jun 16, 2020

View reviewed changes

ericholscher merged commit fb7b506 into master Jun 16, 2020

ericholscher deleted the davidfischer/remove-custom-builders branch June 16, 2020 15:39

amotl mentioned this pull request Mar 21, 2022

Using RTD's "Version warning" feature crate/crate-docs-theme#323

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate the custom RTD builders (except local media) #88

Deprecate the custom RTD builders (except local media) #88

davidfischer commented May 27, 2020 •

edited

Loading

ericholscher left a comment

ericholscher May 27, 2020

davidfischer May 28, 2020

ericholscher May 28, 2020

davidfischer May 28, 2020

ericholscher May 28, 2020

davidfischer May 28, 2020

davidfischer May 28, 2020

ericholscher May 27, 2020

davidfischer May 28, 2020

stsewd May 28, 2020

stsewd May 28, 2020

davidfischer May 28, 2020

ericholscher May 28, 2020

davidfischer May 28, 2020

ericholscher May 28, 2020

ericholscher left a comment

davidfischer commented Jun 3, 2020

ericholscher commented Jun 15, 2020

stsewd commented Jun 15, 2020

davidfischer commented Jun 16, 2020

davidfischer commented Jun 16, 2020

Deprecate the custom RTD builders (except local media) #88

Deprecate the custom RTD builders (except local media) #88

Conversation

davidfischer commented May 27, 2020 • edited Loading

ericholscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher left a comment

Choose a reason for hiding this comment

davidfischer commented Jun 3, 2020

ericholscher commented Jun 15, 2020

stsewd commented Jun 15, 2020

davidfischer commented Jun 16, 2020

davidfischer commented Jun 16, 2020

davidfischer commented May 27, 2020 •

edited

Loading