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

Improve support for deprecated builders without env arg #10702

Merged
merged 5 commits into from
Jul 25, 2022
Merged

Improve support for deprecated builders without env arg #10702

merged 5 commits into from
Jul 25, 2022

Conversation

jdknight
Copy link
Contributor

Feature or Bugfix

  • Bugfix

Purpose

After upgrading to Sphinx 5.1.x, some third-party builders may fail to load. For example:

$ python -m sphinx -b confluence . _build/confluence -E -a
Running Sphinx v5.1.0

Exception occurred:
  File "C:\Users\...\AppData\Roaming\Python\Python38\site-packages\sphinx\registry.py", line 169, in create_builder
    builder = self.builders[name](app, env=...)  # type: ignore[arg-type]
TypeError: __init__() got an unexpected keyword argument 'env'
The full traceback has been saved in C:\Users\...\AppData\Local\Temp\sphinx-err-nvacn3g4.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!

It appears the env change (6ef22d2) for builders using the older cannot handle the new Sphinx engine.

These changes help support the older environment initialization, for builders that have yet to upgrade their definitions.

Relates

@jdknight
do not pass env argument into deprecated builders
7a39a30 
Changes to the builder to move towards passing an environment into the
builder [1] can cause some legacy builder types to fail to load [2].
For example:

    Traceback (most recent call last):
      File "/home/runner/work/confluencebuilder/confluencebuilder/.tox/py38-sphinx51/lib/python3.8/site-packages/sphinx/registry.py", line 162, in create_builder
        return self.builders[name](app, env)
    TypeError: __init__() takes 2 positional arguments but 3 were given

The fallback case for preparing deprecated builders should avoid setting
`env`.

[1]: 6ef22d2
[2]: sphinx-contrib/confluencebuilder#691

Signed-off-by: James Knight <james.d.knight@live.com>
@jdknight
always default create optional environment for deprecated builders
c83c83c 
Builder implementation would originally always create an optional
environment instance, which an extended builder's implementation could
reference in their own `__init__` call. However, with the change [1] to
support an `env` argument, there is no longer a guarantee that
`self.env` will exist. This commit changes this to restore the creating
of `self.env` if `env` if not passed in.

[1]: 6ef22d2

Signed-off-by: James Knight <james.d.knight@live.com>
@jdknight jdknight mentioned this pull request Jul 24, 2022
Closed
@AA-Turner
Copy link
Member

AA-Turner commented Jul 24, 2022 •
edited
Loading

Thank you for the quick PR--this is annoying, I thought I'd handled all the cases.

Can you confirm the PR works for sphinxcontrib confluence please? I plan to include in 5.1.1.

A

@AA-Turner AA-Turner added this to the 5.1.1 milestone Jul 24, 2022
@AA-Turner AA-Turner assigned jdknight and AA-Turner and unassigned jdknight and AA-Turner Jul 24, 2022
@jdknight
Copy link
Contributor Author

Here is a sanity check with various versions of Sphinx:

$ python -m sphinx -b confluence . _build/confluence -E -a
Running Sphinx v5.0.2
...
build succeeded, 9 warnings.


$ pip install sphinx==5.1.0
...
Successfully installed sphinx-5.1.0

$ python -m sphinx -b confluence . _build/confluence -E -a
Running Sphinx v5.1.0

Exception occurred:
  File "...\sphinx\registry.py", line 169, in create_builder
    builder = self.builders[name](app, env=...)  # type: ignore[arg-type]
TypeError: __init__() got an unexpected keyword argument 'env'
...


$ pip install git+https://github.com/sphinx-doc/sphinx.git@refs/pull/10702/merge
...
Successfully installed Sphinx-5.1.0.dev20220724

$ python -m sphinx -b confluence . _build/confluence -E -a
Running Sphinx v5.1.0
...
build succeeded, 9 warnings.

@AA-Turner AA-Turner merged commit 9845500 into sphinx-doc:5.1.x Jul 25, 2022
@AA-Turner AA-Turner mentioned this pull request Jul 25, 2022
Closed
@jdknight jdknight deleted the improve-support-for-deprecated-builders-without-env-arg branch July 26, 2022 00:12
This was referenced Jul 31, 2022
Closed
Merged
attakei added a commit to attakei/sphinx-revealjs that referenced this pull request Aug 1, 2022
@attakei
chore: Merge pull request #124 from attakei/fix/issue-123
939e66b 
Initialize of builder classes support create_builder for Sphinx 5.x and later

Refs: #123
Refs: sphinx-doc/sphinx#10702
attakei added a commit to attakei/sphinx-revealjs that referenced this pull request Aug 1, 2022
@attakei
fix: Initialize of builders accepts app and env for Sphinx>5.1
e20cabd 
Refs: #123
Refs: #124
Refs: sphinx-doc/sphinx#10702
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AA-Turner AA-Turner AA-Turner left review comments

Assignees
No one assigned
Labels
builder type:bug
Projects
None yet
Milestone
5.1.1
Development

Successfully merging this pull request may close these issues.
2 participants
@jdknight @AA-Turner