Do not assign html_theme_path

[benjaoming]

I would like to have a second 🧠 and 👀 on this:

• Confirmation that defining this value will skip calls to sphinx_rtd_theme.setup(app) and thus the logic there is lost
• Confirmation that removing this configuration value doesn't break anything?

Longer story: readthedocs/sphinx_rtd_theme#1355

For reference, here's the full sphinx_rtd_theme.setup(app) which we are not calling AFAICT.

https://github.com/readthedocs/sphinx_rtd_theme/blob/20607e85259f5a1885505807f932fc16cdad9449/sphinx_rtd_theme/__init__.py#L41-L72

---

📚 Documentation previews 📚

• User's documentation (docs): https://docs--9654.org.readthedocs.build/en/9654/

• Developer's documentation (dev): https://dev--9654.org.readthedocs.build/en/9654/

[benjaoming]

I asked on IRC #sphinx-doc and got this crystal-clear response:

because html_theme only calls the entry point if the theme is not yet in html_theme_path

[humitos]

I want to mention some things here:

• I don't have a full understanding of this template conf.py file
• This file is used for all the projects on Read the Docs that uses Sphinx to build their docs (it's appended to their own conf.py file)
• Touching this file will have a pretty big impact on all those projects

That said, is this change required? What are the benefits of it? I'd not jump into this deep research that involves some risk of breaking people's builds if it's not strongly required for a particular reason 😄

[benjaoming]

Touching conf.py.tmpl can certainly have an impact if assumptions are incorrect. At the same time, it would also be weird if we didn't fix things in conf.py.tmpl that are already there because of such wrong assumptions. I'm waiting for someone else to analyze this... as mentioned in the description, the entire sphinx_rtd_theme.setup(app) is most likely not called.

I think it's a bit of a messy situation to not know if setup(app) is called or skipped. For instance, it means that we cannot rely on the theme's jQuery injection being launched from its own setup(app).

For the theme's setup(app) to be called, it's required that current users of RTD would explicitly unset the variable, html_theme_path=[].

[benjaoming]

Another note on this: There seems to be an instruction for sphinx_rtd_theme to add it as an extension and also import the module in conf.py.

See: https://sphinx-rtd-theme.readthedocs.io/en/1.0.0/installing.html#installation

The instructions mention translations as a reason to include it in extensions. Let's investigate if that's redundant as well.

[benjaoming]

I think it's a bit of a messy situation to not know if setup(app) is called or skipped. For instance, it means that we cannot rely on the theme's jQuery injection being launched from its own setup(app).

For Sphinx 6 setups, the theme is now dependent on setup(app) being called. We should figure out if there's any normalized patterns of adding the theme in conf.py where the call doesn't happen, i.e. what this PR is about.

[benjaoming]

On a local build, I tested that the theme is correctly enabled with a configuration that sets html_theme="sphinx_rtd_theme" on Sphinx 1.6.7 with sphinx-rtd-theme 0.5.1

The configuration did not add sphinx_rtd_theme to extensions in conf.py (the workaround).

[benjaoming]

Tested on a local RTD server with tutorial-template -- this project has neither .readthedocs.yaml nor requirements.txt. Its conf.py is minimal.

• Python 3: Gets Sphinx 5.3.0 (because it's Python 3.7 on the default image). Build works ✔️
• Python 2: Gets Sphinx 1.8.6 (the last version to support Python 2.7. Build works (needed to adjust the template project itself for Python 2 compatibility) and the result looks fine, all template assets are there ✔️

[humitos]

@benjaoming I understand that we need this PR for Sphinx 6.x to work properly with our own theme because we depend on calling sphinx_rtd_theme.setup(app) to inject jQuery, right? What happen currently when building on Read the Docs with Sphinx 6.x and sphinx-rtd-theme==1.2.0? Does everything work?

My concerns raised in #9654 (review) are still valid. If we want to move forward with these changes, we should make them in a progressive way, starting by making them under a feature flag.

Also, in case this change only affect people with Sphinx >6.x, we can add that restriction as well. My point here is to reduce the risk/impact of something that looks like a simple change but affect all the builds for all the projects.

[benjaoming]

@benjaoming I understand that we need this PR for Sphinx 6.x to work properly with our own theme because we depend on calling sphinx_rtd_theme.setup(app) to inject jQuery, right?

That is the most urgent part for sure!

There have been suggestions to use setup(app) to add Jinja context variables which broke things. This originally happened here: readthedocs/sphinx_rtd_theme#1355

Also, in case this change only affect people with Sphinx >6.x, we can add that restriction as well. My point here is to reduce the risk/impact of something that looks like a simple change but affect all the builds for all the projects.

Nothing about this is simple, in fact it has already taken a lot of time :) It'd be great to remove this bit-by-bit with a feature flag. I'd like to end up having it completely removed, though.

Could launch with a DONT_OVERWRITE_HTML_THEME_PATH feature flag:

• Enabled on all Sphinx 6+ builds
• Enabled on all future projects
• Deprecation notice*)

Deprecation notice *)

In order to figure out what is being deprecated and give users the right notice, we need to figure out why html_theme_path was necessary. What environment needs it? Why was it added?

[agjohnson]

@benjaoming and I chatted briefly on this one. The big thing is that this issue probably should block a sphinx_rtd_theme 1.2.0 release, as anyone using Sphinx 6 will immediately hit this issue. The effect is JS silently breaking, so our flyout will just stop working -- very easy to miss.

A couple more notes:

I don't have a full understanding of this template conf.py file

Me neither 😬

+1 on being careful here. I think this logic was added/removed because Sphinx included rtd_theme as a dependency for a few releases -- without looking, 1.3-2.0?

Gating this logic change on Sphinx version seems wise 👍 That might be enough for me, but I'm not opposed to a feature flag too.

I'll add: we might want to just ensure sphinx_rtd_theme is in extensions instead. I don't know if it's enough to test if sphinx_rtd_theme can be imported, but that could be an option. I'm guessing we'd have to inspect extensions after the configuration is loaded and ensure sphinx_rtd_theme was loaded as an extension.

Would that make us feel better about this change?

I'm not sure exactly what this looks like, but it would be more than relying on Sphinx to do the right thing. Without some additional logic, my guess is there would still be some Sphinx versions that can't find sphinx_rtd_theme without the html_theme_path addition.

[agjohnson]

In order to figure out what is being deprecated and give users the right notice, we need to figure out why html_theme_path was necessary

My memory is fuzzy here, but for Sphinx releases where sphinx_rtd_theme was not a dependency, Sphinx did not know how to find the theme path. This was likely before we suggested executing the theme as an extension, and so the logic in our theme extension code wouldn't have been used:

https://github.com/readthedocs/sphinx_rtd_theme/blob/master/sphinx_rtd_theme/__init__.py#L62-L63

[humitos]

@agjohnson

I'll add: we might want to just ensure sphinx_rtd_theme is in extensions instead. I don't know if it's enough to test if sphinx_rtd_theme can be imported, but that could be an option. I'm guessing we'd have to inspect extensions after the configuration is loaded and ensure sphinx_rtd_theme was loaded as an extension.

This confuses me a little. We are not currently doing this check, so this will be a new approach, right?

I'm just proposing to keep the same code we have now for all the projects except they have a feature flag enabled or they are using Sphinx >= 6:

diff --git a/readthedocs/doc_builder/templates/doc_builder/conf.py.tmpl b/readthedocs/doc_builder/templates/doc_builder/conf.py.tmpl
index 05bd3f0e0..fe3ba6734 100644
--- a/readthedocs/doc_builder/templates/doc_builder/conf.py.tmpl
+++ b/readthedocs/doc_builder/templates/doc_builder/conf.py.tmpl
@@ -58,10 +58,10 @@ using_rtd_theme = (
     ) or 'html_theme' not in globals()
 )
 if using_rtd_theme:
-    theme = importlib.import_module('{{ html_theme_import }}')
     html_theme = '{{ html_theme }}'
     html_style = None
     html_theme_options = {}
+    if ({{ project.has_feature(Feature.OVERWRITE_HTML_THEME_PATH) }} and not sphinx.version_info >= (6, 0, 0)) or sphinx.version_info < (6, 0, 0):
     if 'html_theme_path' in globals():
         html_theme_path.append(theme.get_html_theme_path())
     else:

OVERWRITE_HTML_THEME_PATH will be default True for all past projects. This way, I think we are not changing any current behavior unless the project is using Sphinx >= 6; which is what we want.

[benjaoming]

This confuses me a little. We are not currently doing this check, so this will be a new approach, right?

Yes - while it might work, I also find it hacky and potentially having other side-effects. Ideally, we take an approach that can give us less headache in the future.

I would prefer what you suggest @humitos , essentially gating the Sphinx version -- it's much more clear what is happening in this case. It also acts as a parameter -- as our knowledge of the issue is more firm and while we might add deprecation announcements or warnings, we can lower our threshold to include Sphinx 5.. and then ultimately eliminate setting html_theme_path as well as the feature flag.

[agjohnson]

This confuses me a little. We are not currently doing this check, so this will be a new approach, right?

Correct. Instead of implicitly addressing the Spinx 6 behavior change, we'd explicitly address it. That is, removing html_theme_path tricks Sphinx into executing our theme as an extension, I'm suggesting instructing Sphinx to do this directly instead of relying on side effects. We can still gate on Sphinx version or whatever we're comfortable with.

Removing this logic when using Sphinx 6 is definitely fine for now, but I do think we'll want a longer term solution here. I'm doubtful we can get away with removing this logic for all Sphinx versions.

So, I'd lean towards using Sphinx version as the conditional here I suppose. The problem is that we likely can't remove html_theme_path for older releases of Sphinx, or we can't expect the same implicit behavior from older releases. So we will likely will have long-lived conditionals against the Sphinx version in this code. This is the scenario where we've been trying to get away from using feature flags.

[humitos]

@agjohnson

We can proceed with the hack here for now, but I wanted to note that we'll be back on this bug rather quickly, as projects upgrade to Sphinx 6 with old instantiation methods.

If you can describe what are those installation methods, we can probably use Telemetry to track them to eventually communicate with the authors and start deprecating old patterns. We've hit this problem about "avoid moving forward" or "moving forward is so complex" because supporting pretty old stuffs.

(note that I don't have a full understanding of the whole situation here --theme, conf.py, installation methods, etc-- but I think we've bitten by this situation multiple times)

[benjaoming]

If you can describe what are those installation methods, we can probably use Telemetry to track them to eventually communicate with the authors and start deprecating old patterns.

@humitos it's because essentially the same broken config containing html_theme_path has also been added because of legacy install methods.

It would be really nice if you can see how many conf.py files that contain r"html_theme_path.*get_html_theme_path.*".

Such setups won't appear broken before they start building with Sphinx 6 and jQuery is missing -- but in principle they are already broken, the effects are very very insignificant (example is the pilcrow, see this pr)

In the code comment, I try to describe this with a link to an example of where this setup originates from: https://github.com/readthedocs/readthedocs.org/pull/9654/files#diff-184ae94fa10f0076a33752ab9d4c4e9fc5081d754a158cd5c3f903637e32c935R64-R79

[benjaoming]

@agjohnson in case you haven't noticed, this PR has been updated.

[agjohnson]

If you can describe what are those installation methods, we can probably use Telemetry to track them

The case is anyone setting the html_theme_path option. It would be great to have a list of these values from projects. I suspect the ratio will be quiet high, and we'll have to face a decision to hack in a fix or raise errors on a fair amount of builds or something similar.

[benjaoming]

The case is anyone setting the html_theme_path option.

+ Using Sphinx unpinned in Python 3.8+ or Sphinx>=6.. at least for now, while we're mostly concerned with jquery.js missing.

[agjohnson]

Using Sphinx unpinned in Python 3.8+ or Sphinx>=6

Why limit the amount of telemetry data we're collecting to this? Our telemetry data already stores package versions, it just doesn't store this configuration option, that I could find. With the addition of tracking html_theme_path values in our telemetry, we can correlate Python/Sphinx versions if it's really necessary.

[benjaoming]

Why limit the amount of telemetry data we're collecting to this?

Not meant to limit it - it just seems to be the most important segment, since those are the intersections where we know things will be broken.

[humitos]

@agjohnson

With the addition of tracking html_theme_path values in our telemetry, we can correlate Python/Sphinx versions if it's really necessary.

I'm not sure how to track this, since we are already defining it. So, I'm not sure how we can differentiate if it's the user who declared the variable or if it's us. If you have an idea for this, I'm happy to expand our telemetry to save it somehow.

[agjohnson]

I don't want to continue sidetracking this issue, we can discuss metrics later. This bug will be one we need to monitor anyways. We can return to metrics collection next.

I'm not sure how to track this, since we are already defining it

I wasn't sure either. If we're getting similar values after the configuration is loaded in Sphinx, we could store a copy of the original value of html_theme_path, before we alter the option in our conf.py template. I'm not familiar with the telemetry code here though, so 🤷

Not meant to limit it - it just seems to be the most important segment

We can just query this segment in Metabase, we have the data to correlate versions. We're better off with more data collection around builds in general.

[benjaoming]

@agjohnson I believe we should also do this, just want to make sure that you have seen it. New projects won't follow strange old install guides, and they are likely on later versions of Sphinx.

[benjaoming]

Aligned with @agjohnson about merging this now 👍

[agjohnson]

Updates look good to go! We have next steps planned already too, we should get an issue up and in our roadmap for these.