Require Sphinx 6.2 to build Python 3.13 documentation #104818
Labels
type-bug
An unexpected behavior, bug, or error
Comments
|
I think this is fine: it can be only for 3.13, not due out until October 2024, and Sphinx 6.2 will be 18 months old by then. |
Sphinx 6.2.0 was released at Apr 23, 2023. Today, it's only 1 month old. But when Python 3.13 final will be released (October 2024), it will be 1 year and 6 months old. |
vstinner
added a commit
to vstinner/cpython
that referenced
this issue
May 25, 2023
Would you mind to officially approve the PR #104819 in that case? |
|
We don't normally build the docs with the Python built from the current repo do we? The Makefile seems to say |
|
No, we don't build the docs with Python built from the current repo, but we do run doctest on the CI which needs that Python to actually test that Python. For example, see the imghdr removal PR (#104777) that fails with old Sphinx: https://github.com/python/cpython/actions/runs/5082380617/jobs/9132034271?pr=104777 |
|
According to packages.debian.org, Debian provides:
I see that Sphinx is slowly being upgraded from 3.4 to 5.3, and then later a migration to 7.0 may happen. While Sphinx is upgrade to 5.3 in testing, Python is upgraded from 3.9 to 3.11. I imagine that in the future, a similar move will happen with the migration from Sphinx 5.3 to 6.2 (or newer) and Python from 3.11 to 3.13 (or maybe first to 3.12). Again, if Debian is stuck with an older Sphinx version, it's easy to backport the change which removed the dependency to the imghdr dependency. Or someone can provide imghdr on PyPI, Debian can create a new dedicated package for it, and then add a build dependency on it (it's not needed to run Python, obviously). Note: a quick & dirty hack is to just copy the Lib/imghdr.py file from Python 3.12 to the right place in Sphinx. But it should not land in production, right? ;-) |
|
I looked again at the whole problem. I understood that Sphinx 6.2 was stricky required to build the Python documentation on Python 3.13. I was wrong. It's only required for the specific GHA Doctest CI job: only this job runs with Python 3.13. In fact, the GHA Doc job which only builds the documentation runs with Sphinx 3.2 which remains compatible with Python 3.11. Since upgrading Sphinx is not strictly required to remove the imghdr module, I close this issue. Obviously, if someone has another good rationale to upgrade Sphinx, please open a new separated issue with that rationale. But again, the imghdr removal is not a good rationale :-) |
|
Ah yes, that's a good solution in PR #104777 to unblock imghdr removal: Sphinx 6.2 in And maintain support for 3.2 by keeping We can consider bumping the minimum Sphinx more incrementally. For example, can we already move to Sphinx 5.3 for main/3.13, 3.12 and 3.11? |
|
I'd think that would think 5.3 as the minimum for 3.11, and maybe 3.12 too would be too aggressive, as it was released a few days before the final release of Python 3.11.0 itself, and it is an already released feature version. For 3.12 we could perhaps bump it to 4.5.0 though, and maybe go 5.3.0 with 3.13. On the other hand, the normal Also, re-closing the issue for now with the correct close state. |
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this issue
Aug 17, 2024
The old pinned version and its explicitly constrained dependencies are retained for now for Python 3.7 so that documentation can be built even with 3.7. (This could maybe be removed soon as a preliminary step toward evenutally dropping 3.7 support.) For Python 3.8 and higher, version 7.1.2 is used, allowing later patch versions but constrained to remain 7.1.*. This is so the same versions are likely to be selected on all Python version from 3.8 and higher, to minimize small differences in generated documentation that different versions could give, and also to simplify debugging. The reason to upgrade Sphinx now is to suppport Python 3.13, which shall be (and, in the pre-releases available, is) incompatible with versions of Sphinx below 6.2. This is because those earlier Sphinx versions use the deprecated `imghdr` module, which 3.13 removes: - https://docs.python.org/3.13/whatsnew/3.13.html#whatsnew313-pep594 - python/cpython#104818 On old versions of Sphinx, that gives the error: Extension error: Could not import extension sphinx.builders.epub3 (exception: No module named 'imghdr') Using Sphinx 6.2 is sufficient to avoid this, but there do not seem to be any disadvantages for GitPython to remain below 7.1.2. The reason we did not upgrade Sphinx before is that, last time we considered doing so, we ran into a problem of new warnings (that we treat as errors). This is detailed in the "Can we upgrade Sphinx?" section of gitpython-developers#1802, especially the "What Sphinx 5 is talking about" subsection. The problem is warnings about `Actor` when it comes in through type annotations: WARNING: more than one target found for cross-reference 'Actor': git.objects.util.Actor, git.util.Actor So this includes other changes to fix that problem as well. The solution used here is to import `Actor` even when `TYPE_CHECKING` is `false`, and write it unquoted in annotations, as `Actor` rather than `"Actor"`. This allows Sphinx to discern where it should consider it to be located, for the purpose of linking to its documentation. This does not incur overhead, because: - The affected modules already have imports from `git.util`, so also importing `Actor` from `git.util` does not cause any modules to be imported that were not imported otherwise, nor even to be imported at a different time. - Even if that that had not been the case, most modules in GitPython including `git.util` have members imported them into the top-level `git` module in `git.__init__` when `git` is imported (and thus when any Python submodule of `git` is imported). The only disadvantage is the presence of the additional name in those modules at runtime, which a user might inadvertently use and thus write brittle code that could break if it is later removed. But: - The affected modules define `__all__` and do not include `"Actor"` in `__all__`, so it is non-public. - There are many places in GitPython (and most Python libraries) where the onus is already on the author of code that uses the library to avoid doing this. The reasons for this approach, rather than any of several others, were: 1. I did not write out the annotations as `git.util.Actor` to resolve the ambiguity because annotations should, and for some uses must, also be interpretable as expressions. But while `from git.util import Actor` works and makes `Actor` available, `git.util.Actor` cannot be used as an expression even after `import git.util`. This is because, even after such an import, `git.util` actually refers to `git.index.util`. This is as detailed in the warnings issued when it is accessed, originally from an overly broad `*` import but retained because removing it could be a breaking change. See `git/__init__.py` for details. 2. The reason I did not write out the annotations as `git.objects.util.Actor` to resolve the ambiguity is that not all occurrences where Sphinx needed to be told which module to document it as being from were within the `git.objects` Python submodule. Two of the warnings were in `git/objects/tag.py`, where annotating it that way would not be confusing. But the other two were in `git/index/base.py`. 3. Although removing `Actor` from `git.objects.util.__all__` would resolve the ambiguity, this should not be done, because: - This would be a breaking change. - It seems to be there deliberately, since `git.objects.util` contains other members that relate to it directly. 4. The reasons I did not take this opportunity to move the contents of `git/util.py` to a new file in that directory and make `git/util.py` re-export the contents, even though this would allow a solution analogous to (1) but for the new module to be used, while also simplifying importing elsewhere, were: - That seems like a change that should be done separately, based either on the primary benefit to users or on a greater need for it. - If and when that is done, it may make sense to change the interface as well. For example, `git/util.py` has a number of members that it makes available for use throughout GitPython but that are deliberately omitted from `__all__` and are meant as non-public outside the project. One approach would be to make a module with a leading `_` for these "internal" members, and another public ones with everything else. But that also cannot be decided based on the considerations that motivate the changes here. - The treatment of `HIDE_WINDOWS_KNOWN_ERRORS`, which is public in `git/util.py` and which currently *does* have an effect, will need to be considered. Although it cannot be re-bound by assigning to `git.util.HIDE_WINDOWS_KNOWN_ERRORS` because the `git.util` subexpression would evaluate to `git.index.util`, there may be code that re-binds it in another way, such as by accessing the module through `sys.modules`. Unlike functions and classes that should not be monkey-patched from code outside GitPython's test suite anyway, this attribute may reasonably be assigned to, so it matters what module it is actually in, unless the action of assigning to it elsewhere is customized dynamically to carry over to the "real" place. 5. An alternative solution that may be reasonable in the near future is to modify `reference.rst` so duplicate documentation is no longer emitted for functions and classes that are defined in one place but imported and "re-exported" elsewhere. I suspect this may solve the problem, allowing the `Actor` imports to go back under `if TYPE_CHECKING:` and to annotate with `"Actor"` again while still running `make -C doc html` with no warnings. However, this would entail design decisions about how to still document those members. They should probably have links to where they are fully documented. So an entry for `Actor` in the section of `reference.rst` for `git.objects.util` would still exist, but be very short and refer to the full autodoc item for `Actor` the section for `git.util`. Since a `:class:` reference to `git.objects.util.Actor` should still go to the stub that links to `git.util.Actor`, it is not obvious that solving the duplication in documentation generated for `reference.rst` ought to be done in a way that would address the ambiguity Sphinx warns about, even if it *can* be done in such a way. Therefore, that should also be a separate consideration and, if warranted, a separate change.
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this issue
Aug 17, 2024
The old pinned version and its explicitly constrained dependencies are retained for now for Python 3.7 so that documentation can be built even with 3.7. (This could maybe be removed soon as a preliminary step toward evenutally dropping 3.7 support.) For Python 3.8 and higher, version 7.1.2 is used, allowing later patch versions but constrained to remain 7.1.*. This is so the same versions are likely to be selected on all Python version from 3.8 and higher, to minimize small differences in generated documentation that different versions could give, and also to simplify debugging. The reason to upgrade Sphinx now is to suppport Python 3.13, which shall be (and, in the pre-releases available, is) incompatible with versions of Sphinx below 6.2. This is because those earlier Sphinx versions use the deprecated `imghdr` module, which 3.13 removes: - https://docs.python.org/3.13/whatsnew/3.13.html#whatsnew313-pep594 - python/cpython#104818 On old versions of Sphinx, that gives the error: Extension error: Could not import extension sphinx.builders.epub3 (exception: No module named 'imghdr') Using Sphinx 6.2 is sufficient to avoid this, but there do not seem to be any disadvantages for GitPython to remain below 7.1.2. The reason we did not upgrade Sphinx before is that, last time we considered doing so, we ran into a problem of new warnings (that we treat as errors). This is detailed in the "Can we upgrade Sphinx?" section of gitpython-developers#1802, especially the "What Sphinx 5 is talking about" subsection. The problem is warnings about `Actor` when it comes in through type annotations: WARNING: more than one target found for cross-reference 'Actor': git.objects.util.Actor, git.util.Actor So this includes other changes to fix that problem as well. The solution used here is to import `Actor` even when `TYPE_CHECKING` is `false`, and write it unquoted in annotations, as `Actor` rather than `"Actor"`. This allows Sphinx to discern where it should consider it to be located, for the purpose of linking to its documentation. This does not incur overhead, because: - The affected modules already have imports from `git.util`, so also importing `Actor` from `git.util` does not cause any modules to be imported that were not imported otherwise, nor even to be imported at a different time. - Even if that that had not been the case, most modules in GitPython including `git.util` have members imported them into the top-level `git` module in `git.__init__` when `git` is imported (and thus when any Python submodule of `git` is imported). The only disadvantage is the presence of the additional name in those modules at runtime, which a user might inadvertently use and thus write brittle code that could break if it is later removed. But: - The affected modules define `__all__` and do not include `"Actor"` in `__all__`, so it is non-public. - There are many places in GitPython (and most Python libraries) where the onus is already on the author of code that uses the library to avoid doing this. The reasons for this approach, rather than any of several others, were: 1. I did not write out the annotations as `git.util.Actor` to resolve the ambiguity because annotations should, and for some uses must, also be interpretable as expressions. But while `from git.util import Actor` works and makes `Actor` available, `git.util.Actor` cannot be used as an expression even after `import git.util`. This is because, even after such an import, `git.util` actually refers to `git.index.util`. This is as detailed in the warnings issued when it is accessed, originally from an overly broad `*` import but retained because removing it could be a breaking change. See `git/__init__.py` for details. 2. The reason I did not write out the annotations as `git.objects.util.Actor` to resolve the ambiguity is that not all occurrences where Sphinx needed to be told which module to document it as being from were within the `git.objects` Python submodule. Two of the warnings were in `git/objects/tag.py`, where annotating it that way would not be confusing. But the other two were in `git/index/base.py`. 3. Although removing `Actor` from `git.objects.util.__all__` would resolve the ambiguity, this should not be done, because: - This would be a breaking change. - It seems to be there deliberately, since `git.objects.util` contains other members that relate to it directly. 4. The reasons I did not take this opportunity to move the contents of `git/util.py` to a new file in that directory and make `git/util.py` re-export the contents, even though this would allow a solution analogous to (1) but for the new module to be used, while also simplifying importing elsewhere, were: - That seems like a change that should be done separately, based either on the primary benefit to users or on a greater need for it. - If and when that is done, it may make sense to change the interface as well. For example, `git/util.py` has a number of members that it makes available for use throughout GitPython but that are deliberately omitted from `__all__` and are meant as non-public outside the project. One approach would be to make a module with a leading `_` for these "internal" members, and another public ones with everything else. But that also cannot be decided based on the considerations that motivate the changes here. - The treatment of `HIDE_WINDOWS_KNOWN_ERRORS`, which is public in `git/util.py` and which currently *does* have an effect, will need to be considered. Although it cannot be re-bound by assigning to `git.util.HIDE_WINDOWS_KNOWN_ERRORS` because the `git.util` subexpression would evaluate to `git.index.util`, there may be code that re-binds it in another way, such as by accessing the module through `sys.modules`. Unlike functions and classes that should not be monkey-patched from code outside GitPython's test suite anyway, this attribute may reasonably be assigned to, so it matters what module it is actually in, unless the action of assigning to it elsewhere is customized dynamically to carry over to the "real" place. 5. An alternative solution that may be reasonable in the near future is to modify `reference.rst` so duplicate documentation is no longer emitted for functions and classes that are defined in one place but imported and "re-exported" elsewhere. I suspect this may solve the problem, allowing the `Actor` imports to go back under `if TYPE_CHECKING:` and to annotate with `"Actor"` again while still running `make -C doc html` with no warnings. However, this would entail design decisions about how to still document those members. They should probably have links to where they are fully documented. So an entry for `Actor` in the section of `reference.rst` for `git.objects.util` would still exist, but be very short and refer to the full autodoc item for `Actor` the section for `git.util`. Since a `:class:` reference to `git.objects.util.Actor` should still go to the stub that links to `git.util.Actor`, it is not obvious that solving the duplication in documentation generated for `reference.rst` ought to be done in a way that would address the ambiguity Sphinx warns about, even if it *can* be done in such a way. Therefore, that should also be a separate consideration and, if warranted, a separate change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I propose to require Sphinx 6.2.0 to build Python 3.13 documentation.
PEP 594 scheduled the removal of the imghdr module in Python 3.13. Problem: Sphinx EPUB builder used it until Sphinx 6.2.
Sphinx 6.2 got fixed: it no longer uses this deprecated module:
This change is ok for Fedora and RHEL will follow Fedora, and it's ok for OpenSuSE.
In May 2022, Debian Stable used Sphinx 3.4 and Ubuntu LTS used Sphinx 3.5.
In May 2022, Python was modified to require Sphinx 3.2 or newer: see issue #86986.
See also details in the imghdr module removal PR.
Alternatives:
Linked PRs
The text was updated successfully, but these errors were encountered: