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

Sphinx doc builds recently fail on Python 3.9 and higher #1802

Closed
EliahKagan opened this issue Jan 15, 2024 · 5 comments · Fixed by #1803
Closed

Sphinx doc builds recently fail on Python 3.9 and higher #1802

EliahKagan opened this issue Jan 15, 2024 · 5 comments · Fixed by #1803

Comments

@EliahKagan
Copy link
Contributor

EliahKagan commented Jan 15, 2024

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__:

tagger: Union[None, "Actor"] = None,

In git.index.base.IndexFile.commit:

GitPython/git/index/base.py

Lines 1082 to 1083 in 53ddf38

author: Union[None, "Actor"] = None,
committer: Union[None, "Actor"] = None,

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, the git.util module is git/util.py, as one would expect.
  • In an expression like git.util, or a statement like import git.util, the git.util module is git/index/util.py.

This happens because git/__init__.py rebinds the git.util attribute:

from git.index import * # noqa: F403 # @NoMove @IgnorePep8

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:

  1. pin them at their latest versions that actually work with Sphinx 4.3.0, or
  2. 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).

EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Jan 15, 2024
This fixes version incompatibilities with major version 4 of
Sphinx, which GitPython is still using for the time being. Some
plugins that previously had depended back on Sphinx have had those
dependencies removed to avoid dependency cycles, but the effect is
that `pip` no longer is aware of or able to enforce version
compatibility, and newer plugin versions than can actually run with
Sphinx 4 are installed instead of older, usable versions.

This fixes the problem by pinning each of the affected Sphinx
plugins' latest actually compatible versions, i.e., latest versions
that do not need Sphinx 5 or higher.

The alternative of moving to Sphinx 5 or higher should probably be
done eventually, but will require addressing a cross-reference
ambiguity in type annotations for the `Actor` class.

For details on the bug, see:

- gitpython-developers#1799 (comment)
- gitpython-developers#1802
EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Jan 15, 2024
Although 4.4.0 or greater do not seem usable for the time being due
to finding "more than one target found for cross-reference 'Actor'"
as examined in gitpython-developers#1802, bugfixes from 4.3.* patch releases are okay.
@Byron
Copy link
Member

Byron commented Jan 16, 2024

Thanks so much for the detailed analysis!

Somehow I feel that it's OK to keep the docs duct-taped into place and now with the version-pinning, I'd expect it to keep working for a while longer as well.

The auto-generated docs that are having issues now never were desirable - they looked messy, were partial, and often the formatting is off as it's incredibly hard to get these RST doc-strings right. Even when I knew what I was looking for, I would have difficulty finding it and it was best to resort to a code-search.

Maybe it's just me, but based on my experiences and unless Sphinx 5 yields significantly better docs for classes, including search, then it's probably not worth to invest anymore time than to keep it working.

Also, I acknowledge that my writing might be affected as I seem to hold a grudge against it for breaking first the doc-builds on readthedocs and a little later, CI here, without any change of myself. With that said, it was running for what feels like a decade, so overall it's still a great job 😁.

@EliahKagan
Copy link
Contributor Author

Maybe it's just me, but based on my experiences and unless Sphinx 5 yields significantly better docs for classes, including search, then it's probably not worth to invest anymore time than to keep it working.

That could be investigated. In my mind, the main benefit of using a later version of Sphinx is that fewer issues like this would occur and, when they do, they could be reported as bugs against the relevant Sphinx package. Because Sphinx 4 is no longer supported, the effects of changes in Sphinx-related packages and their dependencies on it are, as I understand it, not generally being considered to be bugs.

As far as I have found, there are no security advisories for any of the versions of Sphinx-related packages that GitPython is using. Taken together with the points you've raised, I think this suggests that the benefit of upgrading may be low at this time.

The best thing to do may be to wait until GitPython drops support for Python 3.7 -- which when it happens should, I think, be for unrelated reasons -- and then revisit whether Sphinx can and should be upgraded. That would simplify some issues for development dependencies.

@Byron
Copy link
Member

Byron commented Jan 16, 2024

That sounds good - let's revisit the upgrade to Sphinx 5 once doing so is potentially cheap as at the very least we can hope for another decade of 'it just works'.

@EliahKagan
Copy link
Contributor Author

and often the formatting is off as it's incredibly hard to get these RST doc-strings right.

It occurs to me that it may be possible to fix that efficiently, with some information about intended style. (This is separate from what version of Sphinx is used.) For example, in #1725 I mentioned:

In cases where the intent was less clear, such as non-wrapping newlines conveying a break more major than that between sentences but less major than that between paragraphs, I preserved the style where possible but did not make changes to cause it to be newly reflected in rendered documentation.

But that's actually feasible to search for (albeit imprecisely). So if it turns out that there is a simple pattern for what is intended, for example if a paragraph break in the rendered documentation is always or usually intended in such cases, then I think those improvements could be made without difficulty.

Another example is the formatting of literals, especially True, False, None. These are easy to search for. Because code spans are often used for references, I've sometimes seen these formatted with emphasis (e.g., True, coded as *True*), but they can also be formatted as non-reference code spans (True, coded as ``True``).

Even when I knew what I was looking for, I would have difficulty finding it and it was best to resort to a code-search.

I'm wondering if that may already have improved, even by the version of Sphinx that GitPython is using. I am usually able to find classes and their methods by typing their name into the search box. This works for me on readthedocs as well as in local builds. (On the other hand, editor-based symbol lookup has also advanced, so the relative value of searching the documentation and search the code may have remained the same.)

The aspect of the rendered API documentation that I find most confusing is the way the sections, as listed in the sidebar, are capitalized. This often differs from the capitalization of the identifiers they refer to or most closely relate to.

@Byron
Copy link
Member

Byron commented Jan 16, 2024

I think if Sphinx 5 becomes an option and generally works, it might be worth trying to figure out how to configure it for best effect. Maybe by now Sphinx would suffer from the very old configuration and maybe plugins, and setting it up freshly might naturally bring forward great features.

Currently the look also isn't to my liking, and I am sure there are great themes that would be a better fit if one took the time to evaluate and configure them.

I am hopeful that in the end, the docs will look much better than I thought possible, maybe with not even that great of an effort.

EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Feb 23, 2024
This configures Read the Docs builds to be more like local builds
in two ways:

- Pass "-W", as is done in a local build with "make -C doc html",
  so that if there are broken references, the build fails.

- Install dependencies. This configures the Python environment, via
  the python.install key, so that RTD builds install requirements.

More specifically on dependency installation, it does two things:

1. The equivalent of "pip install .", which installs the project
   and its dependencies (though not any extras). This includes the
   gitdb dependency, which is needed to import GitPython's git
   module to populate sections in the API Reference page (gitpython-developers#1840).

2. The equivalent of "pip install -r doc/requirements.txt", which
   installs the additional Sphinx-related dependencies used when
   building documentation locally.

Installing Sphinx-related dependencies is useful for three reasons:

a. Least importantly, it should increase consistency between remote
   (RTD) and local documentation builds.

b. It may be needed to avoid warnings that are not being fixed at
   this time, while still allowing the build to succeed with the
   "-W" option (see above on that change) that causes failure for
   immediately addressable problems. The effect of newer versions
   of Sphinx carrying a few extra hard-to-fix warnings for
   GitPython is noted in gitpython-developers#1802 (and is why they are not upgraded
   in gitpython-developers#1803).

c. One of the documentation build dependencies listed in
   doc/requirements.txt is sphinx_rtd_theme. In 634151a (for gitpython-developers#1794)
   the line specifying this theme was commented out, since it
   apparently broke in the build. This may allow it to be used
   again (or can be replaced with another custom theme if desired).

This also reenables the sphinx_rtd_theme theme disabled in 634151a.

Finally, this makes minor changes to .readthedocs.yml's comments
and formatting so the comments are accurate for GitPython details
and so the file is formatted in the same style as other YAML here.
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
Development

Successfully merging a pull request may close this issue.

2 participants