Description
This bug was discovered by @et-repositories in #1799. The problem was not introduced in that (currently unmerged) pull request. It affects all branches including the main branch. I've proposed a fix in #1803.
The bug
On Python 3.9 and higher, on all platforms, building the docs has begun to fail:
(.venv) C:\Users\ek\source\repos\GitPython [main ≡]> make -C doc html
make: Entering directory '/c/Users/ek/source/repos/GitPython/doc'
mkdir -p build/html build/doctrees
sphinx-build -b html -d build/doctrees -W source build/html
Running Sphinx v4.3.0
Sphinx version error:
The sphinxcontrib.applehelp extension used by this project needs at least Sphinx v5.0; it therefore cannot be built with this version.
make: *** [Makefile:32: html] Error 2
make: Leaving directory '/c/Users/ek/source/repos/GitPython/doc'
This has happened due to a change in indirect documentation build dependencies whose version numbers are not pinned. It is not due to any code change in GitPython (neither on the main branch nor in any open pull requests).
Analysis
The cause is:
- GitPython's
doc/requirements.txt
file pins Sphinx at version 4.3.0. - For Sphinx 4.3.0, the latest version of sphinxcontrib-applehelp is 1.0.4, due to sphinxcontrib-applehelp 1.0.5 requiring
Sphinx>=5
. No version of sphinxcontrib-applehelp since 1.0.5 is actually compatible with major version 4 of Sphinx. - On 12 January 2024, sphinxcontrib-applehelp 1.0.8 was released. This is not compatible with any earlier versions of Sphinx than its predecessors. However, to eliminate a circular dependency where Sphinx and sphinxcontrib-applehelp would both depend on each other, sphinxcontrib-applehelp 1.0.8 no longer declares a dependency on Sphinx. (More precisely, it moves that declared requirement into a new extra.) This causes
pip
be unaware that sphinxcontrib-applehelp 1.0.8 is not really compatible with Sphinx 4.3.0.
Thus, running pip install -r doc/requirements.txt
now installs both Sphinx 4.3.0 and sphinxcontrib-applehelp 1.0.8, even though they are not actually compatible with each other, and the error shown above occurs when attempting to build documentation.
Note that, separately from the above, the latest version of sphinxcontrib-applehelp that supports Python 3.7 (which GitPython still supports) is sphinxcontrib-applehelp 1.0.2.
Other plugins are likewise affected
The first incompatibility to be identified and reported when running make -C doc html
is with sphinxcontrib-applehelp, and the build terminates immediately. But the problem is not specific to that one plugin. Instead, that plugin's name is alphabetically the earliest, of those that have recently gotten releases that remove the circular dependency. The affected plugins relevant to GitPython's documentation builds are:
Plugin | Change | New release | Sphinx <5 compatible | Python 3.7 compatible (if lower) |
---|---|---|---|---|
sphinxcontrib-applehelp | PR #15 | 1.0.8 | 1.0.4 | 1.0.2 |
sphinxcontrib-devhelp | PR #11 | 1.0.6 | 1.0.2 | — |
sphinxcontrib-htmlhelp | PR #18 | 2.0.5 | 2.0.1 | 2.0.0 |
sphinxcontrib-qthelp | PR #15 | 1.0.7 | 1.0.3 | — |
sphinxcontrib-serializinghtml | PR #10 | 1.1.10 | 1.1.5 | — |
Can we upgrade Sphinx?
Sphinx 4 is quite old, and ideally GitPython would move to a later major version of Sphinx. However, this is not as simple as one might hope. Sphinx 5.0.0 and later will complain about ambiguities in documentation cross-references in more situations, or at least more situations than Sphinx 4.3.0. (As detailed below, this actually seems to start in Sphinx 4.4.0.)
That is a good thing, of course, because usually such ambiguities can be fixed easily, and doing so improves documentation. But it appears that GitPython has a particular kind of ambiguity that is difficult to fix without either making a breaking change or causing the documentation to become confusing.
This is the failure:
(.venv) C:\Users\ek\source\repos\GitPython [sphinx ≡ +0 ~1 -0 ~]> make -C doc html
make: Entering directory '/c/Users/ek/source/repos/GitPython/doc'
mkdir -p build/html build/doctrees
sphinx-build -b html -d build/doctrees -W source build/html
Running Sphinx v5.3.0
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 7 source files that are out of date
updating environment: [new config] 7 added, 0 changed, 0 removed
reading sources... [100%] tutorial
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [100%] tutorial
Warning, treated as error:
C:\Users\ek\source\repos\GitPython\git\objects\tag.py:docstring of git.objects.tag.TagObject:1:more than one target found for cross-reference 'Actor': git.objects.util.Actor, git.util.Actor
make: *** [Makefile:32: html] Error 2
make: Leaving directory '/c/Users/ek/source/repos/GitPython/doc'
That is shown above with Sphinx 5.3.0, the latest version that supports all versions of Python that GitPython supports. But I also tested it on Sphinx 5.0.0, the lowest version compatible with recent versions of sphinxcontrib-applehelp and the other plugins noted above, with the same result.
What Sphinx 5 is talking about
GitPython has Sphinx treat warnings as errors by placing -W
in SPHINXOPTS
, which is good since otherwise one risks building documentation whose text or cross-references are markedly different from what one expects. I recommend against removing or significantly weakening that. However, for testing purposes one can also pass --keep-going
to reveal the other warnings that would occur:
C:\Users\ek\source\repos\GitPython\git\objects\tag.py:docstring of git.objects.tag.TagObject:1: WARNING: more than one target found for cross-reference 'Actor': git.objects.util.Actor, git.util.Actor
C:\Users\ek\source\repos\GitPython\git\objects\tag.py:docstring of git.objects.tag.TagObject.__init__:1: WARNING: more than one target found for cross-reference 'Actor': git.objects.util.Actor, git.util.Actor
C:\Users\ek\source\repos\GitPython\git\index\base.py:docstring of git.index.base.IndexFile.commit:1: WARNING: more than one target found for cross-reference 'Actor': git.objects.util.Actor, git.util.Actor
C:\Users\ek\source\repos\GitPython\git\index\base.py:docstring of git.index.base.IndexFile.commit:1: WARNING: more than one target found for cross-reference 'Actor': git.objects.util.Actor, git.util.Actor
Notwithstanding their inaccurate wording (possibly relating to interaction with sphinx-autodoc-typehints, I am not sure), these warnings are not actually issued for references in docstrings, but instead for references in type annotations. In git.objects.tag.TagObject.__init__
:
Line 47 in 53ddf38
In git.index.base.IndexFile.commit
:
Lines 1082 to 1083 in 53ddf38
I don't know if this is really a sufficient condition to cause the problem, but the reported ambiguity is between the Actor
class in git/util.py
, and the very same class imported into git/objects/util.py
and "exported" from it by listing it in __all__
.
Another ambiguity, possibly relevant
It seems like something more must be going on to cause the problem, since the imports, as a Python implementation must read them, import it from git.util
, which I believe is never git.objects.util
. This is a strange statement to make; shouldn't I be sure? Well, this is hard to reason about, because git.util
already refers to two separate modules:
- In a statement like
from git.util import X
, thegit.util
module isgit/util.py
, as one would expect. - In an expression like
git.util
, or a statement likeimport git.util
, thegit.util
module isgit/index/util.py
.
This happens because git/__init__.py
rebinds the git.util
attribute:
Line 26 in 53ddf38
I think this should be fixed--so it does not do that--in the next major version of GitPython. But I don't think there's any reasonable way to fix it without a breaking change. The workaround is to always use from
imports when accessing things in the original git.util
. (A secondary workaround is to use sys.modules["git.util"]
, which always refers to the original git.util
.) I also do not know if it is a contributing factor in confusing Sphinx about whose util
module's Actor
is being referred to.
I note that issue here for two reasons. One is in case it turns out to be relevant in later investigation. The other is that, even if it is causally unrelated, it seems to exacerbate the problem that there are no completely elegant solutions, as detailed in "Possible fixes" below.
Versions between 4.3.0 and 5
GitPython currently pins Sphinx at 4.3.0, and this could be brought up to 4.3.2 without introducing warnings about the Actor
annotations.
Sphinx also has versions 4.4.0 and 4.5.0. I believe they do not themselves introduce generally breaking changes. However, like the versions of Sphinx 5 I tested (5.0.0 and 5.3.0), they detect the Actor
annotation as ambiguous, failing in the same way. My guess is that this arises as an effect one one of the documented changes in 4.4.0, but I don't know which one or why.
Is it Sphinx, or sphinx-autodoc-typehints?
The sphinx-autodoc-typehints plugin depends on specific versions of Sphinx, such that when Sphinx is upgraded, it must sometimes be upgraded. With Sphinx 4.3.0, sphinx-autodoc-typehints 1.17.1 is used. Later versions of sphinx-autodoc-typehints can be used with later versions of Sphinx. Even Sphinx 4.4.0 allows a later version to be installed.
So is it really newer versions than 4.3.0 of Sphinx that consider the Actor
annotation to be ambiguous, or is it the versions of sphinx-autodoc-typehints that comes with them?
To test this, I pinned recently updated plugins (see below), then also pinned sphinx-autodoc-typehints at 1.17.1 and tried Sphinx 4.4.0 and Sphinx 4.5.0. The failures still occurred. So it is really Sphinx, or at least that is an important part of it. It seems very unlikely that pinning sphinx-autodoc-typehints could solve this.
Possible fixes
Import tweaks?
I have tried changing relative imports to absolute imports, moving imports out of if TYPE_CHECKING:
, and combinations of the two. None of that seems to make any difference. I didn't expect it to, but I figured it was worth a try.
Importing Actor
from where it is re-exported?
Actor
is actually defined in git/util.py
. Ambiguities can often be overcome by making a reference more explicit as to what it means. But replacing Actor
with git.util.Actor
would not be an improvement. The ambiguity is in what git.util
refers to, and at runtime, git.util.Actor
would always be resolved as git.index.util.Actor
, which we do not mean (and which I do not expect to exist).
However, git.objects.util.Actor
is the same class, and it can be referred to explicitly. It may be possible to make the Sphinx build work with no warnings, and without suppressing any warnings that could cause serious problems in generated documentation to go undetected, by annotating it that way, possibly in combination with other tweaks.
I am reluctant to do this. Conceptually, that's not where Actor
is from. As I understand it, it is offered through git.objects.util
for convenience, not based on the idea that it ought really to have been defined there. Furthermore, I'm reluctant to make changes to Python code to get documentation to build, when the changes might make the code itself more confusing. After all, one of the ways the code is documented is the code itself.
Exploring less compatible solutions?
Another possible general approach is to use the latest version of Sphinx, which only supports Python 3.9 and later, and see if there is a way to fix things up with that. I expect that to have the same problems, but I have not investigated this.
We can use different versions of Sphinx on different versions of Python, but the generated documentation may then differ. It is probably undesirable for the documentation to differ based on what version of Python we use to generate it, among versions GitPython documents support for.
Pinning or reconfiguring sphinx-autodoc-typehints?
As noted above, pinning sphinx-autodoc-typehints is unlikely to solve this, but perhaps the plugin can be reconfigured in such a way that little accuracy is lost overall but where Actor
is reported differently to Sphinx. I don't know how fruitful this would be.
Pinning the recently updated plugins (#1803)
A more conservative solution, which should not preclude a better solution later, is to list sphinxcontrib-applehelp and the other affected plugins explicitly in doc/requirements.txt
and either:
- pin them at their latest versions that actually work with Sphinx 4.3.0, or
- pin them at or below those versions, so the same versions are used as had been selected before.
With the first approach, Python 3.7 support would have to be excluded (the documentation build step can be disabled on CI for Python 3.7), or Sphinx plugin dependency versions would have to be special-cased for Python 3.7, or versions compatible with Python 3.7 would have to be used on all Python versions.
The second approach avoids all that, but may make it harder for some automated tools to detect vulnerable versions, in the event that a security vulnerability is ever found and reported in any of these plugins. The second approach might also obscure what versions are actually known to work, but this could be minimized by specifying lower bounds (just low enough for Python 3.7 to have a compatible version) as well as upper bounds.
The 3.7 issue could also be avoided by dropping support for Python 3.7 (as the PSF has done). But since people who use GitPython (i.e., have it as a project dependency) don't generally build the documentation themselves, I don't recommend dropping 3.7 support for this reason.
Even with that Python 3.7 issue to deal with, this is a conceptually straightforward and fairly simple change, which does not require any Python code to be modified. I think it makes sense to do this first to get Sphinx builds working again on the main branch (and any PRs that rebase onto it), and then possibly pursue better solutions.
PR #1803 would fix the bug by pinning the plugins (addressing the Python 3.7 issue using the second approach, with lower as well as upper bounds on the two plugins that would otherwise not install on Python 3.7).