Remove old assets inclusion/generation at build process #72
Comments
This comment was marked as outdated.
This comment was marked as outdated.
humitos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
I took a look at the |
|
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 🙈 |
|
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 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 |
|
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 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. |
|
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 |
|
Interesting, I've tried the |
|
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 |
|
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 I still think this approach could work, but it definitely needs more testing. |
|
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) |
|
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. |
Not sure what you mean exactly with this. My goal here to remove the injection of the Instead, all the important/useful variables are passed via environment variables (as we are doing with The work I was describing with this approach was to know who was using these variables to:
|
|
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. |
|
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 (
|
|
I can get some information from the previous table:
Proposed Sphinx migration plan
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 Proposed MkDocs migration plan
At this point, we won't be manipulating Footnotes
|
|
We are already setting the Overall this plan sounds good. 👍 |
|
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? |
|
Seems like a good test case for doing the Sphinx migration later 👍 |
|
The 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 |
* Build: remove `append_conf` _magic_ from MkDocs Delete all the magic around MkDocs YAML that processes the `mkdocs.yml`. file and define `readthedocs` theme automatically based on a feature flag, `MKDOCS_THEME_RTD`. This PR removes: - automatically defining `readthedocs` as theme when `MKDOCS_THEME_RTD` feature flag is defined. - processing `mkdocs.yml` to add internal JS and CSS (embed and analytics) files automatically This is another step forward on removing all the magic we perform on behalf the users and being more explicit about how to configure each doctool. Reference: * readthedocs/addons#72 (comment) Closes #8529 * Enable Addons if project is MkDocs Listen to `Project.post_save` signal and enable Addons if the project was created after a specific date, it's MkDocs and it's the first time the `AddonsConfig` is created. * Listen to Version instead * Typo * Update readthedocs/projects/signals.py Co-authored-by: Santos Gallegos <[email protected]> * Remove datetime restriction --------- Co-authored-by: Santos Gallegos <[email protected]>
|
Small updates here:
|
I think we need to use |
|
My plan here is to recover the work done in #72 (comment) and create a small I know this is a little hacky, but it's better to be able to get this data and contact users than just remove everything without contacting anybody and breaking people's documentation without advice. With that data, we will be ready to move forward with the plan. Footnotes
|
|
I jumped into this again and I found it's pretty hard to make our original idea to work. Summarizing, we won't be able to inject or mock Sphinx code to know who is using our injected context. Why? Because, Sphinx and Jinja2 perform a lot of operations on the context and many of them are casting the object to a regular Some code I found useful while doing the research:
This is the patch I was testing: https://gist.github.com/humitos/fc79732425c2d8d7d026d8fab3e52919 So, that said, I think we should just follow our deprecation path here: contact users (all of them? 🤷🏼 , blog post, brown dates, etc). Thoughts? |
|
Seems like we have work to do to move forward here, since we're still depending on these variables in a bunch of places? (#72 (comment)) I'm not 100% clear on what we're deprecating here, so we should probably make that explicit if possible. |
|
We are deprecating the manipulation of the Sphinx's
I think the deprecation path here consists in:
Note that we won't have a 1:1 feature parity with this migration and I don't pretend that since it will be definitely impossible. We are deprecating old features we won't maintain anymore. Users will still be able to implement some of them by themselves by adding extra configurations to their |
|
Another idea here is to disable |
Add a feature flag called `DISABLE_SPHINX_MANIPULATION` that: - don't install `readthedocs-sphinx-ext` Python package - don't append anything to the Sphinx's `conf.py` file - enable Read the Docs Addons on these versions automatically The idea behind this is start defaulting new projects to Read the Docs Addons without breaking old projects. This is a potential first step in favor of the full deprecation/removal of the Sphinx manipulation (as we already did for MkDocs). Once this happens, **building on Read the Docs will produce the exact same result than building locally**. Related readthedocs/addons#72
|
During our last 1on1 we talked about developing a Sphinx extension that tries to replicate the behavior we are deprecating. This would allow users/authors/theme developers that really want the context manipulation done by Read the Docs to keep compatibility while they migrate to the new behavior. I created sphinx-build-compatibility to keep this compatibility for now with those projects that want to opt-in. The new plan after our 1on1:
I think this gives us pretty clear steps on how to move forward with this deprecation and tackle the issues we may find as we go. Note that I'm saying to do one step at a time and wait some weeks before performing the next one. @ericholscher thoughts? Footnotes
|
|
@humitos That's awesome you figured out a good way to get this working. Looking at the code, the current integration path for the full historical data is pretty complex, but I imagine most users will only want a subset, so hopefully we can simplify the integration pattern for folks in the future who just want "A list of current versions and translations", which is likely just pulling that from the env. This makes me a lot more confident in pushing forward with deprecation in a faster timeline 💯 |
|
Great! I'd appreciate if you review that PR so we can move forward with the first 3 steps for now to perform our tests. Those steps dont' affect users.
I've done it this way only to keep backward compatibility; but we won't maintain this extension too longer since it's a deprecated behavior we want to get rid of completely. That's the idea.
Ideally, they should use the APIv3 for this (https://docs.readthedocs.io/en/stable/api/v3.html#versions-listing) with an API Token 1. I'd prefer to not inject all this data by default because those are expensive queries for our database depending on the size of the project and 99% of users don't use/need it. Footnotes
|
* Build: disable Sphinx manipulation Add a feature flag called `DISABLE_SPHINX_MANIPULATION` that: - don't install `readthedocs-sphinx-ext` Python package - don't append anything to the Sphinx's `conf.py` file - enable Read the Docs Addons on these versions automatically The idea behind this is start defaulting new projects to Read the Docs Addons without breaking old projects. This is a potential first step in favor of the full deprecation/removal of the Sphinx manipulation (as we already did for MkDocs). Once this happens, **building on Read the Docs will produce the exact same result than building locally**. Related readthedocs/addons#72 * Build: add `READTHEDOCS_REPOSITORY_PATH` environment variable This is a useful variable that we will require during the deprecation of the Sphinx context manipulation. Besides, it seems a useful variable that we should expose to users anyways so they can use it as relative to calculate other useful paths. * Build: add `READTHEDOCS_PRODUCTION_DOMAIN` as environment variable * Document `READTHEDOCS_PRODUCTION_DOMAIN` env var * Update tests
Follows the plan we described in: readthedocs/addons#72 (comment)
Follows the plan we described in: readthedocs/addons#72 (comment)
humitos
added
Improvement
* Post: enable addons by default Follows the plan we described in: readthedocs/addons#72 (comment) * Apply suggestions from code review Co-authored-by: Eric Holscher <[email protected]> * Add a TL;DR note at the beginning * Improve text for the link * Add Python code for canonical URL and READTHEDOCS context variable * Update content/posts/addons-by-default.md Co-authored-by: Eric Holscher <[email protected]> --------- Co-authored-by: Eric Holscher <[email protected]>
|
All the projects have been migrated to the new addons and the Sphinx context is not injected anymore 🎉 . This is a great and big step. Now, be prepared to attend users and customers support requests during the following days/weeks 😄 |
We are generating some
.jsfiles 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:
The text was updated successfully, but these errors were encountered: