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.

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

UndefinedError("'sphinx_version_info' is undefined") - Error in current master #1355

Closed
benjaoming opened this issue Oct 10, 2022 · 7 comments
Assignees
Labels
Accepted Accepted issue on our roadmap Bug A bug
Milestone

Comments

@benjaoming
Copy link
Contributor

benjaoming commented Oct 10, 2022

Problem

This must have been introduced in #1345

Building for instance the Read the Docs user docs with the current theme gives this error:

building [mo]: targets for 0 po files that are out of date
building [html]: targets for 96 source files that are out of date
updating environment: [config changed ('version')] 97 added, 3 changed, 0 removed
reading sources... [100%] versions                                                                                                                                                                         
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [  1%] abandoned-projects                                                                                                                                                                
Theme error:
An error happened in rendering the page abandoned-projects.
Reason: UndefinedError("'sphinx_version_info' is undefined")
make: *** [Makefile:62: html] Error 2
@benjaoming benjaoming added Bug A bug Accepted Accepted issue on our roadmap labels Oct 10, 2022
@benjaoming benjaoming added this to the 1.1 milestone Oct 10, 2022
@benjaoming benjaoming self-assigned this Oct 10, 2022
@benjaoming benjaoming changed the title Error in current master UndefinedError("'sphinx_version_info' is undefined") - Error in current master Oct 10, 2022
@benjaoming
Copy link
Contributor Author

I would want to close this as "invalid" as somehow the entire sphinx_rtd_theme.setup(app) isn't called from this documentation set. But I think it's necessary to find out why, since this update would seem to break the documentation if left un-adressed.

@benjaoming
Copy link
Contributor Author

benjaoming commented Oct 10, 2022

The problem building docs.readthedocs.org is in conf.py:

https://github.com/readthedocs/readthedocs.org/blob/9e7787ce036c49ea03410bbf77b20ccf1b607873/docs/conf.py#L149

html_theme_path = [sphinx_rtd_theme.get_html_theme_path()]

Commenting out this line makes the documentation project build, calling the theme's entrypoint setup(app). I would suppose that the html_theme_path configuration value isn't intended for themes with entrypoints and instead manages to overwrite the installed sphinx_rtd_theme entrypoint and all the setup(app) logic?

@agjohnson
Copy link
Collaborator

I suspect the main issue is the the sphinx_rtd_theme is missing from extensions = [] in conf.py. The theme won't be executed as an extension in this case (sphinx_rtd_theme.setup() is called on instantiation).

However, this is probably a rather common pattern. We didn't specify this installation pattern for a long while, and it's probably not very obvious that the theme needs to be set up as an extension on top of being set as html_theme.

I think this signals we should go back to the original logic, using template code, and fix the bug with breaking up sphinx_version there instead.

@benjaoming
Copy link
Contributor Author

benjaoming commented Oct 11, 2022

@agjohnson

Adding the theme in html_theme is sufficient to have setup(app) called, but somehow adding the path in html_theme_path kills the call. There's no need to add it to extensions -- or that would be a potential override in case html_theme_path is somehow required (but I don't think so? everything works just fine without it).

Here's a minimal conf.py that will call setup(app)

project = 'Test'
copyright = '2022, Test Person'
author = 'Test Person'

extensions = []

templates_path = ['_templates']
exclude_patterns = ['_build', 'Thumbs.db', '.DS_Store']

html_theme = 'sphinx_rtd_theme'
html_static_path = ['_static']

I think this signals we should go back to the original logic, using template code, and fix the bug with breaking up sphinx_version there instead.

I can try to work on that and see what's in the Jinja toolbox.

@readthedocs readthedocs deleted a comment from kkhuaa Oct 11, 2022
@benjaoming
Copy link
Contributor Author

benjaoming commented Oct 11, 2022

Just to illustrate the point, here is a minimal conf.py that does NOT call setup(app)

project = 'Test'
copyright = '2022, Test Person'
author = 'Test Person'

# -- General configuration ---------------------------------------------------
# https://www.sphinx-doc.org/en/master/usage/configuration.html#general-configuration

extensions = []

templates_path = ['_templates']
exclude_patterns = ['_build', 'Thumbs.db', '.DS_Store']



# -- Options for HTML output -------------------------------------------------
# https://www.sphinx-doc.org/en/master/usage/configuration.html#options-for-html-output

html_theme = 'sphinx_rtd_theme'
html_static_path = ['_static']

# Now break things...
import sphinx_rtd_theme
html_theme_path = [sphinx_rtd_theme.get_html_theme_path()]

This is also important for whatever solution we would choose for a sphinx-jquery extension since we cannot rely on logic inside setup(app) unless we wrestle this.

@benjaoming
Copy link
Contributor Author

Noting that the currently injected Sphinx configuration code for Read the Docs has this:

https://github.com/readthedocs/readthedocs.org/blob/9e7787ce036c49ea03410bbf77b20ccf1b607873/readthedocs/doc_builder/templates/doc_builder/conf.py.tmpl#L60-L68

if using_rtd_theme:
    theme = importlib.import_module('sphinx_rtd_theme')
    html_theme = 'sphinx_rtd_theme'
    html_style = None
    html_theme_options = {}
    if 'html_theme_path' in globals():
        html_theme_path.append(theme.get_html_theme_path())
    else:
        html_theme_path = [theme.get_html_theme_path()]

Which ultimately means that for now, we cannot rely on setup(app) being called in most projects.

@benjaoming
Copy link
Contributor Author

Fixed in #1356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Bug A bug
Projects
None yet
Development

No branches or pull requests

2 participants