Repository: gitpython-developers/GitPython
Pull request #2054 (2025-06-16 05:06:42)

Fail `test_installation` on warnings, and remove deprecated license classifier
### The license trove classifier was imprecise and deprecated

The main way we indicate the license in the project's metadata is:

https://github.com/gitpython-developers/GitPython/blob/46c439b5d3b8c8fa26d5b305b31d6cd189e42210/setup.py#L72

However, we have for some time, until this PR, also had a trove classifier indicating "BSD License":

https://github.com/gitpython-developers/GitPython/blob/46c439b5d3b8c8fa26d5b305b31d6cd189e42210/setup.py#L98

This has a couple of problems. One is that there is no specific "BSD License", but instead various BSD licenses, of which two are very popular: BSD-3-Clause, which this project uses, and BSD-2-Clause, which is not what this project uses. (No PyPI-recognized trove classifiers for those specific BSD licenses exist.)

The other problem is that *all* usage of `License :: ...` trove classifiers has been deprecated. When the package is installed, the build backend generates a warning. The warning is typically not shown when using `pip`, but it will be shown if `-v` is passed, and it will be shown and cause an error if `PYTHONWARNINGS=error` is set in the environment. (Running `python -Werror -m pip …` is not sufficient to produce it, because it is not passed down to the subprocess running `setuptools`, which is where the warning occurs.)

```text
SetuptoolsDeprecationWarning: License classifiers are deprecated.
    !!
        ********************************************************************************
        Please consider removing the following classifiers in favor of a SPDX license expression:

        License :: OSI Approved :: BSD License

        See https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license for details.
        ********************************************************************************
    !!
```

The `"BSD-3-Clause"` value of the `license` argument we pass to `setuptools.setup` is already such an SDPX license expression. So all that is needed to fix both these problems is to remove the deprecated trove classifier. However, there are other related problems, some of which make sense to fix at the same time.

### The installation test didn't detect warnings, nor report errors clearly

`test_installation` seeks to detect problems installing GitPython, and with #2053 it is back running and passing on all platforms we test. But it has not treated warnings during installation as errors.

Especially considering the small number of dependencies GitPython has (when not installing the `test` or `doc` extra), we shouldn't ordinarily have any warnings on any supported platform and Python version, and anytime we do, we probably want to know about it. I think it would not be reasonable to set `PYTHONWARNINGS=error` in the top-level `pip install '.[test]'` commands in the CI test workflows, but that this is a good thing to do in `test_installation`.

Setting it in `test_installation` does reveal that an error occurs, but without making other changes to the test, it does not reveal in a readily understandable way *what* error occurs. `test_installation` contains this code (and some other code like it):

https://github.com/gitpython-developers/GitPython/blob/46c439b5d3b8c8fa26d5b305b31d6cd189e42210/test/test_installation.py#L16-L25

The intent of `result.stderr or result.stdout or "Can't install project"` is to show what, if anything, was written to stderr; otherwise show what, if anything, was written to stdout; and otherwise show the prewritten message. It's true that the most important information to understand an error is often on stderr.

The problem is that it will never show what was written to stderr, because the call to `subprocess.run` captures only stdout, then attempts to access stderr as though it had been captured too. Here's a simplified demonstration of that problem:

```text
>>> from subprocess import run, PIPE
>>> result = run(["sh", "-c", "echo 'this is stdout'; echo 'this is stderr' >&2"], stdout=PIPE)
this is stderr
>>> result.stdout
b'this is stdout\n'
>>> result.stderr
>>>
```

Depending on how the test runner (usually `pytest`, which is the only runner the whole test suite supports) is run, the stderr message may be captured by the test runner and not shown, captured by the test runner and shown later where it may not be readily identified, or not captured by the test runner and possibly seen at some point either at the intuitive position or interleaved elsewhere.

### The fixes

This PR:

- Refactors `test_installation` and its helpers to decrease code duplication and to clarify what the steps that set up the new virtual environment are doing, fixes the test bug where stderr was not captured, interacts with the `python` and `pip` subprocesses assuming text-based streams and includes in the assertion message the prewritten summary as well as stdout and stderr (labeling each), and runs the subprocesses with `PYTHONWARNINGS=error`.

  This surfaces the deprecation warning and causes it to fail `test_installation` with fully detailed output reporting the problem.

- Removes the deprecated trove classifier from `setup.py`. This is sufficient to make the test pass, as there are no other diagnostic messages of warning or higher level.

### Outscoped

[Improve license metadata](https://hugovk.dev/blog/2025/improving-licence-metadata/) by [Hugo van Kemenade](https://github.com/hugovk), though not at all specific to GitPython, covers both the problem with some license classifiers (including "BSD License") being unclear and the deprecation, pointing out that an SPDX string should be used to identify in metadata what license a project has chosen.

However, it recommends that one also specify the paths to one's license files using `license-files`. This is already something we were not doing, and removing the deprecated classifier does not worsen that situation. Our `LICENSE` file also does not have customized elements besides a listed author, who is also one of the authors listed in metadata about authorship. Most importantly, this is just about metadata: the `LICENSE` file is part of the project and listed in `MANIFEST.in` (and various files in the project also carry notices). Nonetheless, it would be good to do something like this.

I don't know if we can straightforwardly achieve the effect of `license-files` in `setup.py`. That blog post covers static project definitions in `pyproject.toml`. In addition, that meaning in `pyproject.toml` of `license` and `license-files` is only recognized in sufficiently new versions of build backends. Usually this isn't a problem, but some of these changes to the way licenses are expected to be indicated are actually fairly recent. GitPython supports some old versions of Python that are older than supported by the oldest version of `setuptools` that recognizes `pyproject.toml` with a `license` value that is an SPDX string (even aside from supporting `license-files`).

The packaging guide shows the [current status](https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license-and-license-files)  for this ([PEP 639](https://peps.python.org/pep-0639/)). One implication here is that it looks like we will have to choose between:

- Not using `pyproject.toml` to specify `license` as an SPDX identifier (and I think maybe not switching to `pyproject.toml` at all, and maybe not being able to specify `license-files` or equivalent at all).
- Dropping Python 3.7 and 3.8 support. This is not unreasonable and will definitely happen eventually--they are end-of-life, after all--but I'd rather the reason we stop supporting them relate either to something that matters at runtime, broader maintainability considerations, or major benefits of unconditionally using newer features. GitPython has a lot of users, and while hopefully just about everyone is using a supported Python, I'd be interested in moving slowly on dropping old versions, at least as far as 3.8 is concerned.
- Using `pyproject.toml` with an old, less useful `license` value, which will generate deprecation warnings, and which will eventually--sometime in 2026, I believe--stop the project from being able to be installed and/or published properly. (This deprecation is a more serious deprecation than the one fixed here with classifiers.)
- Switching from the `setuptools` build backend to another build backend that continues to support older Python versions and also supports modern `license` and `license-files`, such as `flit-core` or (if we drop 3.7 by then) `hatchling`.

Although the original plan was to stick with `setuptools` per https://github.com/gitpython-developers/GitPython/issues/1716#issuecomment-1773746757, some time has passed since then and I suspect there may end up being other reasons to switch build backends. I think it may be best to decide all this at the same time, and when actually making the project definition declarative. Therefore, I have not attempted to add `license-files` or equivalent metadata in this PR.


Patch:
diff --git a/setup.py b/setup.py
index f28fedb8..a7b1eab0 100755
--- a/setup.py
+++ b/setup.py
@@ -95,7 +95,6 @@ setup(
         # "Development Status :: 7 - Inactive",
         "Environment :: Console",
         "Intended Audience :: Developers",
-        "License :: OSI Approved :: BSD License",
         "Operating System :: OS Independent",
         "Operating System :: POSIX",
         "Operating System :: Microsoft :: Windows",


Test patch:
diff --git a/test/test_installation.py b/test/test_installation.py
index ae6472e9..7c82bd40 100644
--- a/test/test_installation.py
+++ b/test/test_installation.py
@@ -2,6 +2,7 @@
 # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
 
 import ast
+import functools
 import os
 import subprocess
 
@@ -11,50 +12,22 @@ from test.lib import TestBase, VirtualEnvironment, with_rw_directory
 class TestInstallation(TestBase):
     @with_rw_directory
     def test_installation(self, rw_dir):
-        venv = self._set_up_venv(rw_dir)
+        venv, run = self._set_up_venv(rw_dir)
 
-        result = subprocess.run(
-            [venv.pip, "install", "."],
-            stdout=subprocess.PIPE,
-            cwd=venv.sources,
-        )
-        self.assertEqual(
-            0,
-            result.returncode,
-            msg=result.stderr or result.stdout or "Can't install project",
-        )
+        result = run([venv.pip, "install", "."])
+        self._check_result(result, "Can't install project")
 
-        result = subprocess.run(
-            [venv.python, "-c", "import git"],
-            stdout=subprocess.PIPE,
-            cwd=venv.sources,
-        )
-        self.assertEqual(
-            0,
-            result.returncode,
-            msg=result.stderr or result.stdout or "Self-test failed",
-        )
+        result = run([venv.python, "-c", "import git"])
+        self._check_result(result, "Self-test failed")
 
-        result = subprocess.run(
-            [venv.python, "-c", "import gitdb; import smmap"],
-            stdout=subprocess.PIPE,
-            cwd=venv.sources,
-        )
-        self.assertEqual(
-            0,
-            result.returncode,
-            msg=result.stderr or result.stdout or "Dependencies not installed",
-        )
+        result = run([venv.python, "-c", "import gitdb; import smmap"])
+        self._check_result(result, "Dependencies not installed")
 
         # Even IF gitdb or any other dependency is supplied during development by
         # inserting its location into PYTHONPATH or otherwise patched into sys.path,
         # make sure it is not wrongly inserted as the *first* entry.
-        result = subprocess.run(
-            [venv.python, "-c", "import sys; import git; print(sys.path)"],
-            stdout=subprocess.PIPE,
-            cwd=venv.sources,
-        )
-        syspath = result.stdout.decode("utf-8").splitlines()[0]
+        result = run([venv.python, "-c", "import sys; import git; print(sys.path)"])
+        syspath = result.stdout.splitlines()[0]
         syspath = ast.literal_eval(syspath)
         self.assertEqual(
             "",
@@ -64,10 +37,37 @@ class TestInstallation(TestBase):
 
     @staticmethod
     def _set_up_venv(rw_dir):
+        # Initialize the virtual environment.
         venv = VirtualEnvironment(rw_dir, with_pip=True)
+
+        # Make its src directory a symlink to our own top-level source tree.
         os.symlink(
             os.path.dirname(os.path.dirname(__file__)),
             venv.sources,
             target_is_directory=True,
         )
-        return venv
+
+        # Create a convenience function to run commands in it.
+        run = functools.partial(
+            subprocess.run,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            universal_newlines=True,
+            cwd=venv.sources,
+            env={**os.environ, "PYTHONWARNINGS": "error"},
+        )
+
+        return venv, run
+
+    def _check_result(self, result, failure_summary):
+        self.assertEqual(
+            0,
+            result.returncode,
+            msg=self._prepare_failure_message(result, failure_summary),
+        )
+
+    @staticmethod
+    def _prepare_failure_message(result, failure_summary):
+        stdout = result.stdout.rstrip()
+        stderr = result.stderr.rstrip()
+        return f"{failure_summary}\n\nstdout:\n{stdout}\n\nstderr:\n{stderr}"