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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Jun 16, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c8c0b54
Remove the custom RTD builders
davidfischer May 27, 2020
a9ac695
Re-add the builders as deprecated
davidfischer May 28, 2020
0c08264
Merge branch 'master' into davidfischer/remove-custom-builders
davidfischer Jun 2, 2020
6698995
Move the feature flags added in #85
davidfischer Jun 16, 2020
0eb5114
Merge branch 'master' into davidfischer/remove-custom-builders
davidfischer Jun 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 0 additions & 25 deletions readthedocs_ext/_static/readthedocs-data.js_t
View file
Open in desktop

This file was deleted.

13 changes: 7 additions & 6 deletions readthedocs_ext/_templates/readthedocs-insert.html.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 just want to use the same function where the inline JSON is generated so they're the same.

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'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.


<!-- Add page-specific data, which must exist in the page js, not global -->
<!--
Using this variable directly instead of using `JSON.parse` is deprecated.
The READTHEDOCS_DATA global variable will be removed in the future.
-->
<script type="text/javascript">
READTHEDOCS_DATA['page'] = {{ pagename|tojson }}
{%- if page_source_suffix %}
READTHEDOCS_DATA['source_suffix'] = {{ page_source_suffix|tojson }}
{%- endif %}
READTHEDOCS_DATA = JSON.parse(document.getElementById('READTHEDOCS_DATA').innerHTML);
</script>

<script type="text/javascript" src="{{ rtd_analytics_url }}" async="async"></script>
<script type="text/javascript" src="{{ rtd_js_url }}" async="async"></script>

<!-- end RTD <extrahead> -->
71 changes: 0 additions & 71 deletions readthedocs_ext/mixins.py
View file
Open in desktop

This file was deleted.

182 changes: 66 additions & 116 deletions readthedocs_ext/readthedocs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@


from .embed import EmbedDirective
from .mixins import BuilderMixin

try:
# Avaliable from Sphinx 1.6
Expand All @@ -25,26 +24,25 @@

try:
# Available from Sphinx 2.0
from sphinx.builders.dirhtml import DirectoryHTMLBuilder
from sphinx.builders.html import StandaloneHTMLBuilder
from sphinx.builders.singlehtml import SingleFileHTMLBuilder
except ImportError:
from sphinx.builders.html import (DirectoryHTMLBuilder,
SingleFileHTMLBuilder,
StandaloneHTMLBuilder)
from sphinx.builders.html import SingleFileHTMLBuilder

log = getLogger(__name__)


DEFAULT_STATIC_URL = 'https://assets.readthedocs.org/static/'

# Exclude the SingleHTML builder that is used by RTD to zip up local media
# That builder is never used "online"
ONLINE_BUILDERS = [
'readthedocs', 'readthedocsdirhtml', 'readthedocssinglehtml'
'html', 'dirhtml', 'singlehtml'
Copy link
Member

Choose a reason for hiding this comment

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

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 :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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

]
# Only run JSON output once during HTML build
# This saves resources and keeps filepaths correct,
# because singlehtml filepaths are different
JSON_BUILDERS = [
'html', 'dirhtml',
'readthedocs', 'readthedocsdirhtml'
]

# Whitelist keys that we want to output
Expand All @@ -59,24 +57,6 @@
]


def finalize_media(app):
"""Point media files at our media server."""

if (app.builder.name == 'readthedocssinglehtmllocalmedia' or
app.builder.format != 'html' or
not hasattr(app.builder, 'script_files')):
return # Use local media for downloadable files
# Pull project data from conf.py if it exists
context = app.builder.config.html_context
STATIC_URL = context.get('STATIC_URL', DEFAULT_STATIC_URL)
js_file = '{}javascript/readthedocs-doc-embed.js'.format(STATIC_URL)
if sphinx.version_info < (1, 8):
app.builder.script_files.append(js_file)
else:
kwargs = {'async': 'async'} # Workaround reserved word in Py3.7
app.add_js_file(js_file, **kwargs)


def update_body(app, pagename, templatename, context, doctree):
"""
Add Read the Docs content to Sphinx body content.
Expand Down Expand Up @@ -132,18 +112,41 @@ def rtd_render(self, template, render_context):
then adds the Read the Docs HTML content at the end of body.
"""
# Render Read the Docs content
template_context = render_context.copy()
template_context['rtd_css_url'] = '{}css/readthedocs-doc-embed.css'.format(STATIC_URL)
template_context['rtd_analytics_url'] = '{}javascript/readthedocs-analytics.js'.format(
STATIC_URL,
)
ctx = render_context.copy()
ctx['rtd_data'] = {
'project': ctx.get('slug', ''),
'version': ctx.get('version_slug', ''),
'language': ctx.get('rtd_language', ''),
'programming_language': ctx.get('programming_language', ''),
'canonical_url': ctx.get('canonical_url', ''),
'theme': ctx.get('html_theme', ''),
'builder': 'sphinx',
'docroot': ctx.get('conf_py_path', ''),
'source_suffix': ctx.get('source_suffix', ''),
'page': ctx.get('pagename', ''),
'api_host': ctx.get('api_host', ''),
'commit': ctx.get('commit', ''),
'ad_free': ctx.get('ad_free', ''),
'global_analytics_code': ctx.get('global_analytics_code'),
'user_analytics_code': ctx.get('user_analytics_code'),
'subprojects': {
slug: url for slug, url in ctx.get('subprojects', [])
},
}
if ctx.get('page_source_suffix'):
ctx['rtd_data']['source_suffix'] = ctx['page_source_suffix']
if ctx.get('proxied_api_host'):
ctx['rtd_data']['proxied_api_host'] = ctx['proxied_api_host']
ctx['rtd_css_url'] = '{}css/readthedocs-doc-embed.css'.format(STATIC_URL)
ctx['rtd_js_url'] = '{}javascript/readthedocs-doc-embed.js'.format(STATIC_URL)
ctx['rtd_analytics_url'] = '{}javascript/readthedocs-analytics.js'.format(STATIC_URL)
source = os.path.join(
os.path.abspath(os.path.dirname(__file__)),
'_templates',
'readthedocs-insert.html.tmpl'
)
templ = open(source).read()
rtd_content = app.builder.templates.render_string(templ, template_context)
rtd_content = app.builder.templates.render_string(templ, ctx)

# Handle original render function
content = old_render(template, render_context)
Expand Down Expand Up @@ -200,6 +203,34 @@ def generate_json_artifacts(app, pagename, templatename, context, doctree):
)


def remove_search_init(app, exception):
"""Remove Sphinx's Search.init() so it can be initialized by Read the Docs."""
if exception:
return

searchtools_file = os.path.abspath(
os.path.join(app.outdir, '_static', 'searchtools.js')
)

if os.path.exists(searchtools_file):
davidfischer marked this conversation as resolved.
Show resolved Hide resolved
replacement_text = '/* Search initialization removed for Read the Docs */'
replacement_regex = re.compile(
r'''
^\$\(document\).ready\(function\s*\(\)\s*{(?:\n|\r\n?)
\s*Search.init\(\);(?:\n|\r\n?)
\}\);
''',
(re.MULTILINE | re.VERBOSE)
)

log.info(bold('Updating searchtools for Read the Docs search... '), nonl=True)
with codecs.open(searchtools_file, 'r', encoding='utf-8') as infile:
data = infile.read()
with codecs.open(searchtools_file, 'w', encoding='utf-8') as outfile:
data = replacement_regex.sub(replacement_text, data)
outfile.write(data)


def dump_sphinx_data(app, exception):
"""
Dump data that is only in memory during Sphinx build.
Expand Down Expand Up @@ -261,99 +292,18 @@ def dump_sphinx_data(app, exception):
)


class HtmlBuilderMixin(BuilderMixin):

static_readthedocs_files = [
'readthedocs-data.js_t',
# We patch searchtools and copy it with a special handler
# 'searchtools.js_t'
]

REPLACEMENT_TEXT = '/* Search initialization removed for Read the Docs */'
REPLACEMENT_PATTERN = re.compile(
r'''
^\$\(document\).ready\(function\s*\(\)\s*{(?:\n|\r\n?)
\s*Search.init\(\);(?:\n|\r\n?)
\}\);
''',
(re.MULTILINE | re.VERBOSE)
)

def get_static_readthedocs_context(self):
ctx = super(HtmlBuilderMixin, self).get_static_readthedocs_context()
if self.indexer is not None:
ctx.update(self.indexer.context_for_searchtool())
return ctx

def copy_static_readthedocs_files(self):
super(HtmlBuilderMixin, self).copy_static_readthedocs_files()
self._copy_searchtools()

def _copy_searchtools(self, renderer=None):
"""Copy and patch searchtools

This uses the included Sphinx version's searchtools, but patches it to
remove automatic initialization. This is a fork of
``sphinx.util.fileutil.copy_asset``
"""
log.info(bold('copying searchtools... '), nonl=True)

if sphinx.version_info < (1, 8):
search_js_file = 'searchtools.js_t'
else:
search_js_file = 'searchtools.js'
path_src = os.path.join(
package_dir, 'themes', 'basic', 'static', search_js_file
)
if os.path.exists(path_src):
path_dest = os.path.join(self.outdir, '_static', 'searchtools.js')
if renderer is None:
# Sphinx 1.4 used the renderer from the existing builder, but
# the pattern for Sphinx 1.5 is to pass in a renderer separate
# from the builder. This supports both patterns for future
# compatibility
if sphinx.version_info < (1, 5):
renderer = self.templates
else:
from sphinx.util.template import SphinxRenderer
renderer = SphinxRenderer()
with codecs.open(path_src, 'r', encoding='utf-8') as h_src:
with codecs.open(path_dest, 'w', encoding='utf-8') as h_dest:
data = h_src.read()
data = self.REPLACEMENT_PATTERN.sub(self.REPLACEMENT_TEXT, data)
h_dest.write(renderer.render_string(
data,
self.get_static_readthedocs_context()
))
else:
log.warning('Missing {}'.format(search_js_file))
log.info('done')


class ReadtheDocsBuilder(HtmlBuilderMixin, StandaloneHTMLBuilder):
name = 'readthedocs'


class ReadtheDocsDirectoryHTMLBuilder(HtmlBuilderMixin, DirectoryHTMLBuilder):
name = 'readthedocsdirhtml'


class ReadtheDocsSingleFileHTMLBuilder(BuilderMixin, SingleFileHTMLBuilder):
name = 'readthedocssinglehtml'
class ReadtheDocsSingleFileHTMLBuilderLocalMedia(SingleFileHTMLBuilder):

"""Sphinx builder that builds a single HTML file that will be zipped by Read the Docs."""
davidfischer marked this conversation as resolved.
Show resolved Hide resolved

class ReadtheDocsSingleFileHTMLBuilderLocalMedia(BuilderMixin, SingleFileHTMLBuilder):
name = 'readthedocssinglehtmllocalmedia'


def setup(app):
app.add_builder(ReadtheDocsBuilder)
app.add_builder(ReadtheDocsDirectoryHTMLBuilder)
app.add_builder(ReadtheDocsSingleFileHTMLBuilder)
app.add_builder(ReadtheDocsSingleFileHTMLBuilderLocalMedia)
app.connect('builder-inited', finalize_media)
app.connect('html-page-context', update_body)
app.connect('html-page-context', generate_json_artifacts)
app.connect('build-finished', remove_search_init)
app.connect('build-finished', dump_sphinx_data)

# Embed
Expand Down
Loading