I'm pretty sure I inadvertently introduced this bug in #1693, when I had init-tests-after-clone.sh refrain from cloning submodules when it detected it was running on CI, giving the current code:
|
# The tests need submodules. (On CI, they would already have been checked out.) |
|
if ! ci; then |
|
git submodule update --init --recursive |
|
fi |
I reasoned, incorrectly, that this was safe, due to the CI test workflows in this repository (and thus forks, reuploads, etc.) taking care of cloning submodules (this shows pythonpackage.yml, but cygwin-test.yml has the same thing):
|
- uses: actions/checkout@v4 |
|
with: |
|
fetch-depth: 0 |
|
submodules: recursive |
The problem is that checking for CI--even conservatively, as done here--is not the same as checking for CI workflows originating in this repository:
|
ci() { |
|
# For now, check just these, as a false positive could lead to data loss. |
|
test -n "${TRAVIS-}" || test -n "${GITHUB_ACTIONS-}" |
|
} |
Confusing the two does not cause data loss--any CI will want to skip the interactive prompt--but it does cause brittleness. GitPython's tests can be run on CI through workflows unrelated to the ones we have here, and projects distributing downstream packages of GitPython, including operating systems such as Arch Linux, are examples of where it makes sense to do that.
init-tests-after-clone.sh is intended to be sufficient, after an ordinary (non-shallow) clone, to get a cloned repository to a state where the tests are ready to be run. I think this is both an intended and reasonable assumption from:
|
_Important_: Right after cloning this repository, please be sure to have executed |
|
the `./init-tests-after-clone.sh` script in the repository root. Otherwise |
|
you will encounter test failures. |
The need to clone submodules is intentionally not mentioned in README.md, because it is intended that the init script take care of that whenever it is necessary. Therefore, this really is a regression in GitPython's tests--or more precisely in code that supports them--which I introduced in #1693 due to a mistake about what we are in practice checking for when we check for CI.
The fix should be simple: the init script should unconditionally clone submodules. More granular solutions that still avoid cloning submodules on some CI configurations or if they appear already present are possible, but if that is to be done then I suggest deferring it, so the simpler fix can be applied sooner. I've proposed that fix in #1715, tested it (though not with Arch Linux), and included a more comprehensive analysis in the description.