jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
177 stars 28 forks source link

sphinx~=7.3.0 breaks sphinx-immaterial #345

Closed JacobChesslo closed 3 weeks ago

JacobChesslo commented 2 months ago

Sphinx recently updated to 7.3.*.

This came with some public interface breakages, which sphinx-immaterial uses, detailed in this issue: https://github.com/sphinx-doc/sphinx/issues/12295.

Additionally, sphinx-immaterial uses some more private-leaning methods and functions from sphinx (https://github.com/sphinx-doc/sphinx/pull/12297), which have the potential to be moved/broken/removed in the future.

2bndy5 commented 2 months ago

sphinx-immaterial uses some more private-leaning methods and functions from sphinx

As for using private APi in sphinx, we really can't avoid this without removing some theme-specific features. It would nice if we didn't have to, but Sphinx domains are a bit rigid. See https://github.com/jbms/sphinx-immaterial/issues/113#issuecomment-1166729283

ImportError: cannot import name 'PyTypedField' from 'sphinx.domains.python'

As for this, I don't see PyTypedField on the list of Deprecated API. I do see that it is now re-exported in v7.3.5.

@AA-Turner I don't think it was a good idea to move public API into private modules. That move tells me that only sphinx.ext.* is allowed to use those API members, which is not very practical. Think about all the other third-party sphinx extensions posing an alternative implementation to autodoc (and not just for the python domain).

AttributeError: module 'sphinx.domains.cpp' has no attribute 'ASTOperator'

It looks like the domain refactor in https://github.com/sphinx-doc/sphinx/compare/v7.2.6...v7.3.5 was not specific to just the python domain. Our docs now fail (with v7.3.5) about cpp domain as well. This one might take us a while to resolve...

JacobChesslo commented 2 months ago

I was able to get a sphinx-immaterial build working nominally with just an import of most/all things removed in pre 7.3.0 sphinx.domains.{c,cop,python}, https://github.com/sphinx-doc/sphinx/pull/12297, so that's hopeful that the issue is restricted to a subset of those changes.

AA-Turner commented 2 months ago

You'll need to change

https://github.com/jbms/sphinx-immaterial/blob/9519601aa71dd47a362d1b639a11cf37a3675536/sphinx_immaterial/apidoc/python/style_default_values_as_code.py#L28

to

try:
    if sphinx.version_info >= (7, 3):
        sphinx.domains.python._annotation._parse_arglist = parse_arglist
        sphinx.domains.python._object._parse_arglist = parse_arglist
    else:
        sphinx.domains.python._parse_arglist = parse_arglist
except AttributeError:
    # log some warning rather than crashing
AA-Turner commented 2 months ago

Sphinx 7.3.6 has been released with fixes.

A

2bndy5 commented 2 months ago

jinja_context conains the module sys as a value https://github.com/jbms/sphinx-immaterial/blob/9519601aa71dd47a362d1b639a11cf37a3675536/docs/conf.py#L353-L354 which is causing:

pickling environment... WARNING: cannot cache unpickable configuration value: 'jinja_contexts' (because it contains a function, class, or module object)

This poses a problem in https://github.com/jbms/sphinx-immaterial/blob/9519601aa71dd47a362d1b639a11cf37a3675536/docs/apidoc/python/apigen.rst#L309-L342 where most of it can be generated from conf.py, but using a function extracted from a sys path is a unique obstacle: https://github.com/jbms/sphinx-immaterial/blob/9519601aa71dd47a362d1b639a11cf37a3675536/docs/apidoc/python/apigen.rst#L326 https://github.com/jbms/sphinx-immaterial/blob/9519601aa71dd47a362d1b639a11cf37a3675536/docs/apidoc/python/apigen.rst#L340-L341

Other jinja_context["sys"] usage is in https://github.com/jbms/sphinx-immaterial/blob/9519601aa71dd47a362d1b639a11cf37a3675536/docs/apidoc/python/index.rst#L52-L56 But that should be an easier fix.

2bndy5 commented 2 months ago

Geez. Even the typing for public API changed quite significantly.

Sphinx.add_config_value (https://github.com/sphinx-doc/sphinx/commit/259118d18237893df27424a46b43cd1f02d8ba4f)

I'm not going to lie. This one is a pain in the ass. There's usually one or two things we need to update for a minor version bump... 😞 Also, v7.3 is ripe with merge commits in the master branch (which makes it harder to git blame a PR or instigating issue when looking for rationality). It would be a good idea to start putting a reference to a discussion (PR or issue) in the merge commit's description.

AA-Turner commented 2 months ago

Thank you for the feedback, Brendan (@2bndy5).

Even the typing for [Sphinx.add_config_value] changed quite significantly.

The intent here was to make the type hints (intended usage) stricter, whilst the runtime acceptable values didn't change. Any examples of mypy failing would be useful.

Sphinx.add_config_value no longer takes a bool | str as the rebuild param

It still does at runtime, the static typing information has become stricter, though, to reflect intended use.

Sphinx.add_config_value now uses narrow typing for the types param. According to mypy, all our specified types are wrong (even though they work with v7.3.x)

All previously valid values should still be valid. I'm happy to tweak the type hints if needed (suggestions welcome).

A

2bndy5 commented 2 months ago

All previously valid values should still be valid. I'm happy to tweak the type hints if needed (suggestions welcome).

I don't think Collection[type] satisfies the previous use cases. I got errors saying

error: Argument "types" to "add_config_value" of "Sphinx" has incompatible
type "tuple[type[bool], type[None]]"; expected "type | Collection[type] | ENUM"  [arg-type]
            types=(bool, type(None)),
                  ^~~~~~~~~~~~~~~~~~

When I change Collection[type] to Sequence[type], I no longer get this error from mypy. For now, I've just type: ignored any of our usage that doesn't satisfy Collection[type].

AA-Turner commented 2 months ago

Collection ought be more general than Sequence (as e.g. a set is a Collection but not a Sequence).

Mypy runs fine for me on the first two of the below three test-cases, and only reports an error for the third -- I suspect this might actually be an error with Mypy's implementation/special casing of NoneType? Certainly from a type theory perspective I think the annotations in add_config_value are correct.

from sphinx.application import Sphinx

def setup(app: Sphinx) -> None:
    app.add_config_value(
        'spam',
        None,
        '',
        (bool, str, type(None))
    )
    app.add_config_value(
        'ham',
        None,
        '',
        (bool, str)
    )
    app.add_config_value(
        'ham',
        None,
        '',
        (bool, type(None))  # mypy error: incompatible type ...
    )

Sorry to cause trouble, though, it isn't intended and thank you for your co-operation in trying to improve the situation.

A

2bndy5 commented 2 months ago

Thanks for your consideration. For passers by, this error was reported with mypy v1.8.0 and v1.9.0.

2bndy5 commented 3 weeks ago

fixes released in v0.11.12