Remove old assets inclusion/generation at build process

[humitos]

We are generating some .js files and we are including some other static files. Mainly in MkDocs.

We should remove them from the build process once we are using the new addons approach. See the code to remove at https://github.com/readthedocs/readthedocs.org/blob/6f394fd89a34e636de30fc23268bb51b7c85028b/readthedocs/doc_builder/backends/mkdocs.py#L170-L180

Note there are a bunch of tests and other related things we can remove as well.

Issues:

• Closes MkDocs 1.5 and the !relative YAML tag readthedocs.org#10803
• Closes https://github.com/readthedocs/meta/issues/85
• Related Help ReadTheDocs to better integrate with MkDocs mkdocs/mkdocs#3413

[humitos]

I took a look at the conf.py.tmpl that we append on Sphinx projects and I was thinking on a way to detect what projects are using what variables from the context. Is there any way we can achieve this?

[ericholscher]

I think we can probably wrap the function in some hacky way so that it logs a message or something? It would definitely be a hack or a monky patch though 🙈

[humitos]

I was thinking something similar yesterday, but I wasn't sure how to implement it and I ended up reading about metaclasses (https://stackoverflow.com/questions/11349183/how-to-wrap-every-method-of-a-class) which I wanted to avoid.

Today, I arrived at a lot simpler solution: wrap the context dictionary variable we create into a class that inherit from dict that we control and log something when any of the injected keys are accessed:

READTHEDOCS_INJECTED_KEYS = [
    'html_theme',
    'current_version',
    'version_slug',
    'MEDIA_URL',
    'STATIC_URL',
    'PRODUCTION_DOMAIN',
    'proxied_static_path',
    'versions',
    'downloads',
    'subprojects',
    'slug',
    'name',
    'rtd_language',
    'programming_language',
    'canonical_url',
    'analytics_code',
    'single_version',
    'conf_py_path',
    'api_host',
    'github_user',
    'proxied_api_host',
    'github_repo',
    'github_version',
    'display_github',
    'bitbucket_user',
    'bitbucket_repo',
    'bitbucket_version',
    'display_bitbucket',
    'gitlab_user',
    'gitlab_repo',
    'gitlab_version',
    'display_gitlab',
    'READTHEDOCS',
    'using_theme',
    'new_theme',
    'source_suffix',
    'ad_free',
    'docsearch_disabled',
    'user_analytics_code',
    'global_analytics_code',
    'commit',
]

class ReadTheDocsSphinxContext(dict):

    def __getitem__(self, name):
        if name in READTHEDOCS_INJECTED_KEYS:
            print("Read the Docs value from injected context called.")
        return super().__getitem__(name)

    def __getattribute__(self, name):
        if name in READTHEDOCS_INJECTED_KEYS:
            print("Read the Docs value from injected context called.")
        return super().__getattribute__(name)

Then, when the user access any of these keys we will get an extra output in the console (we can perform an internal API call or even call NR to log the line somehow). The example shows just an output in the console:

In [14]: context = MyClass({"name": "Read the Docs", "slug": "read-the-docs", "html_theme": "sphinx_rtd_theme", "non-RTD-injected": "
    ...: a users's value"})

In [15]: context["name"]
Read the Docs value from injected context called.
Out[15]: 'Read the Docs'

In [16]: context["html_theme"]
Read the Docs value from injected context called.
Out[16]: 'sphinx_rtd_theme'

In [17]: context["non-RTD-injected"]
Out[17]: "a users's value"

In [18]:

I think something like this could work. We will need to define this class in the conf.py.tmpl and wrap the context variable we define with this class. Honestly, it doesn't sound terrible to me and I don't think it will affect people's build in a bad way. This solution will allow us to know exactly what projects are using what injected keys and be able to talk to them about next steps. @ericholscher What do you think about this idea?

[humitos]

I tested this locally and it fails when pickling the Sphinx environment at https://github.com/sphinx-doc/sphinx/blob/master/sphinx/builders/__init__.py#L330

 _pickle.PicklingError: Can't pickle : attribute lookup ReadTheDocsSphinxContext on builtins failed 

I think there is some modifications to the code we can make to make this object picklable. I tried to disable pickling the environment on Sphinx but I didn't find a configuration for that. I'm not sure to understand why it fails when pickling this object inside of Sphinx but it doesn't when I pickle it directly from the terminal.

[ericholscher]

This is what I was imagining when I said a hack or monkey patch :) It's a reasonable way to get that data. 💯

Could perhaps use this solution which sounds similar: https://stackoverflow.com/a/21145155

[humitos]

Interesting, I've tried the __getstate__ and __setstate__ already, but I wasn't aware of that __reduce__. I can try that as well and see what happens.

[humitos]

I tried that and I wasn't able to make it work. There is something else happening here withe Sphinx environment that contains this object. I can pickle ReadTheDocsSphinxContext without problems by calling pickle.dump(...), as Sphinx calls, and it works. However, when it's called from Sphinx itself it doesn't work 🤷🏼

[humitos]

I'm pretty close, but it seems because of this line: https://github.com/sphinx-doc/sphinx/blob/fa290049515c38e68edda7e8c17be69b8793bb84/sphinx/builders/html/__init__.py#L561

a new object is created and we lost control of it. I tried to override the __or__ in our object, but since it's a the right of the operation our method is not called, but the other object instead (a real dict).

I still think this approach could work, but it definitely needs more testing.

[humitos]

This is the code I've been playing with:

class ReadTheDocsSphinxContext(dict):

    # obj[key]
    def __getitem__(self, name):
    # def __class_getitem__(cls, key):
        READTHEDOCS_INJECTED_KEYS = [
            'html_theme',
            'current_version',
            'version_slug',
            'MEDIA_URL',
            'STATIC_URL',
            'PRODUCTION_DOMAIN',
            'proxied_static_path',
            'versions',
            'downloads',
            'subprojects',
            'slug',
            'name',
            'rtd_language',
            'programming_language',
            'canonical_url',
            'analytics_code',
            'single_version',
            'conf_py_path',
            'api_host',
            'github_user',
            'proxied_api_host',
            'github_repo',
            'github_version',
            'display_github',
            'bitbucket_user',
            'bitbucket_repo',
            'bitbucket_version',
            'display_bitbucket',
            'gitlab_user',
            'gitlab_repo',
            'gitlab_version',
            'display_gitlab',
            'READTHEDOCS',
            'using_theme',
            'new_theme',
            'source_suffix',
            'ad_free',
            'docsearch_disabled',
            'user_analytics_code',
            'global_analytics_code',
            'commit',
        ]

        if name in READTHEDOCS_INJECTED_KEYS:
            print(f"INFO: Read the Docs value from injected context called. {name=}")

            breakpoint()
        return super().__getitem__(name)

    # obj.key
    def __getattribute__(self, name):
        READTHEDOCS_INJECTED_KEYS = [
            'html_theme',
            'current_version',
            'version_slug',
            'MEDIA_URL',
            'STATIC_URL',
            'PRODUCTION_DOMAIN',
            'proxied_static_path',
            'versions',
            'downloads',
            'subprojects',
            'slug',
            'name',
            'rtd_language',
            'programming_language',
            'canonical_url',
            'analytics_code',
            'single_version',
            'conf_py_path',
            'api_host',
            'github_user',
            'proxied_api_host',
            'github_repo',
            'github_version',
            'display_github',
            'bitbucket_user',
            'bitbucket_repo',
            'bitbucket_version',
            'display_bitbucket',
            'gitlab_user',
            'gitlab_repo',
            'gitlab_version',
            'display_gitlab',
            'READTHEDOCS',
            'using_theme',
            'new_theme',
            'source_suffix',
            'ad_free',
            'docsearch_disabled',
            'user_analytics_code',
            'global_analytics_code',
            'commit',
        ]

        if name in READTHEDOCS_INJECTED_KEYS:
            print(f"INFO: Read the Docs value from injected context called. {name=}")
            breakpoint()

        return super().__getattribute__(name)

    def __or__(self, other):
        print("__or__ called")
        breakpoint()

        result = super().__or__(other)
        return ReadTheDocsSphinxContext(result)


html_context = ReadTheDocsSphinxContext({"slug": "project-slug", "commit": "a1b2c3"})

templates_path = [
    "templates",
]

# Remove the following warning
#
# WARNING: The config value `html_context' has type `ReadTheDocsSphinxContext', defaults to `dict'.
#


# def env_updated(app, env):
def html_page_context(app, pagename, templatename, context, doctree):
    app.builder.globalcontext = ReadTheDocsSphinxContext(app.builder.globalcontext)
    breakpoint()


def readthedocs_context(app, config):
    config._raw_config["__builtins__"]["ReadTheDocsSphinxContext"] = ReadTheDocsSphinxContext


def setup(app):
    app.connect("config-inited", readthedocs_context)
    app.connect("html-page-context", html_page_context)
    # app.connect("env-updated", env_updated)

[ericholscher]

Yea, I think it likely needs to be monkeypatched or similar, which is what I was worried about. Will be hard to maintain it across Sphinx versions... and getting pretty complex for what we're trying to do. If we aren't planning to remove these config items, I'm not 100% sure we need to know which are being used. We can always just fall back to alerting everyone.

[humitos]

If we aren't planning to remove these config items, I'm not 100% sure we need to know which are being used.

Not sure what you mean exactly with this. My goal here to remove the injection of the conf.py.tmpl completely eventually and have parity between build.jobs and build.commands: Read the Docs does not inject anything behind the scenes with this magic approach.

Instead, all the important/useful variables are passed via environment variables (as we are doing with READTHEDOCS_ ones) to all the commands executed.

The work I was describing with this approach was to know who was using these variables to:

• be able to only send an email to those users
• know what are those important/useful variables to convert them into environment variables

[ericholscher]

Gotcha -- if the goal is to know who to email, I think we can just email all active Sphinx/Mkdocs projects. It's a big enough change it's likely worth letting people know about, likely at the same time as Addons becoming default.

In terms of "which data is important", that is harder to get without this approach.

[humitos]

OK, let's say this approach is complex to implement and fragile to make it work in all versions, discard it for a moment and talk about a different one: "don't add any magic to our build process and evaluate these variables one by one, checking if they are used in code we control (readthedocs.org, readthedocs-sphinx-ext and sphinx_rtd_theme):

Variable	Used	Where
html_theme	✅	data.js.tmpl: won't be used since we are removing that file anyways. readthedocs.py: inject CSS based on theme
current_version	✅	data.js.tmpl: add it to READTHEDOCS_DATA. versions.html: render the flyout statically at build time. layout.html: used under undocumented variable theme_display_version. external_version_warning.py: replaced by addons
version_slug	✅	readthedocs.py: dump it into js file
MEDIA_URL	❌
STATIC_URL	✅	readthedocs.py: generate CSS URL
PRODUCTION_DOMAIN	✅	versions.html: generate links on the flyout
proxied_static_path	✅	readthedocs.py: generate proxied URLs. readthedocs-doc-embed.js: automatically inject jQuery. rtd-data.js: modifies READTHEDOCS_DATA js object
versions	✅	versions.html: generate flyout statically
downloads	✅	versions.html: generate flyout statically
subprojects	✅	readthedocs.py: dump into js object
slug	✅	versions.html: generate flyout statically
name	❌
rtd_language	✅	readthedocs.py: dump into js object
programming_language	✅	readthedocs.py: dump into js object
canonical_url	✅	readthedocs.py: dump into js object
analytics_code	❌
single_version	❌
conf_py_path	✅	readthedocs.py: dump into js object. breadcrumbs.html: generate "Edit on.." links
api_host	❌
github_user	✅	breadcrumbs.html: generate "Edit on.." links
proxied_api_host	✅	readthedocs.py: dump into js object.
github_repo	✅	breadcrumbs.html: generate "Edit on.." links
github_version	✅	breadcrumbs.html: generate "Edit on.." links
display_github	✅	breadcrumbs.html: generate "Edit on.." links
bitbucket_user	✅	breadcrumbs.html: generate "Edit on.." links
bitbucket_repo	✅	breadcrumbs.html: generate "Edit on.." links
bitbucket_version	✅	breadcrumbs.html: generate "Edit on.." links
display_bitbucket	✅	breadcrumbs.html: generate "Edit on.." links
gitlab_user	✅	breadcrumbs.html: generate "Edit on.." links
gitlab_repo	✅	breadcrumbs.html: generate "Edit on.." links
gitlab_version	✅	breadcrumbs.html: generate "Edit on.." links
display_gitlab	✅	breadcrumbs.html: generate "Edit on.." links
READTHEDOCS	✅	Everywhere 😄
using_theme	❌
new_theme	❌
source_suffix	✅
ad_free	✅	readthedocs.py: dump into js object.
docsearch_disabled	✅	readthedocs.py: dump into js object.
user_analytics_code	✅	readthedocs.py: dump into js object.
global_analytics_code	✅	readthedocs.py: dump into js object.
commit	✅	footer.html: display the commit at bottom

[humitos]

I can get some information from the previous table:

• Lot of these context variables won't be needed when migrating to addons
• All the ones that are dumped into the js file aren't required when migrating to addons
• A few of them are used directly by our theme
• We can start by removing the ones that we know we are not using and see what happens
• I'd propose to immediately pass the useful ones as environment variables and adapt our theme to start using them:
  • READTHEDOCS
  • READTHEDOCS_GIT_COMMIT_HASH

Proposed Sphinx migration plan

1. Remove the ones we already know we are not using
2. Update our theme to use environment variables instead of context variables (READTHEDOCS and READTHEDOCS_GIT_COMMIT_HASH)
3. Implement new addons on our theme (Addons: integrate with new beta addons flyout sphinx_rtd_theme#1526)
4. Release a new version of our theme
5. Figure it out how to generate "Edit on..." links in a clean way (I doubt there is one without too much magic, see https://github.com/readthedocs/readthedocs.org/blob/5cc6cf40663b36c5325aa52e4bf825951894be3f/readthedocs/proxito/views/hosting.py#L377-L392). As a example, furo uses our own magic if the variables are available --removing these variables won't break docs at least 👍🏼 ¹
6. Write a blog post explaining this deprecation to communicate this to our users
7. Enable addons by default on new projects together with this behavior (don't install readthedocs-sphinx-ext and don't append conf.py.tmpl to them)

At this point, all new projects will be using the latest version of our theme that implements the flyout using addons and we won't be adding any magic behind the scenes (not installing readthedocs-sphinx-ext and not appending conf.py.tmpl)

Proposed MkDocs migration plan

1. Write a blog post communicating this new behavior
2. Enable addons by default on new MkDocs projects and stop calling append_conf to modify their mkdocs.yml

At this point, we won't be manipulating mkdocs.yml (which will immediately solve readthedocs/readthedocs.org#8529 as well) and building on Read the Docs will be exactly the same as building locally.

Footnotes

1. I may be 👍🏼 on dropping this feature if we don't find a clean/generic way of supporting this, since I don't know how useful it is anyways. ↩

[ericholscher]

We are already setting the READTHEDOCS & READTHEDOCS_GIT_COMMIT_HASH env var along with some others: https://docs.readthedocs.io/en/stable/reference/environment-variables.html

Overall this plan sounds good. 👍

[humitos]

I think we can make the first step starting by executing the "Proposed MkDocs migration plan" that it's a lot less work than the Sphinx one 😄 . It won't give us a lot, but we will be moving forward in the direction we want and allowing users to use MkDocs without hitting any issues with the YAML file. Thoughts?

[ericholscher]

Seems like a good test case for doing the Sphinx migration later 👍

[humitos]

The BaseMkDocs.append_conf method checks for Feature.MKDOCS_THEME_RTD and inject our own theme if it's not explicitly defined in the project. We tried to removed this feature flag in the past but we found there were some projects still depending on us setting the readthedocs theme --so we added those projects to the new feature flag to keep this behavior, but removed this magic from the builder for all new projects.

We added a log line in https://github.com/readthedocs/readthedocs.org/blob/217c5bc65fe551b264dc11608f263b68e2f652af/readthedocs/doc_builder/backends/mkdocs.py#L52-L66 to know what are those projects.

I used this log line to check New Relic at https://onenr.io/0BQroa8ZnwZ and we only have only 2 projects using this feature flag that belong to the same organization. So, this change will be a pretty low impact change and easy to fix by the customer. They just need to define theme.name: readthedocs in their mkdocs.yml file: https://www.mkdocs.org/user-guide/configuration/#theme

[humitos]

Small updates here:

• The MkDocs plan was already executed, Build: remove append_conf _magic_ from MkDocs readthedocs.org#11206 is merged and we are defaulting to addons for MkDocs projects
• I opened a discussing on Furo's Sphinx theme explaining that we plan to remove the conf.py manipulation at Read the Docs integration may potentially break in the future pradyunsg/furo#785 to coordinate the work with them

[humitos]

I tried to override the __or__ in our object, but since it's a the right of the operation our method is not called, but the other object instead (a real dict).

I think we need to use __ior__ here since the left operand will return NotImplemented and call ours after that. 🚧 I want to give this another try and double check if it works.