Constrain GDAL to one minor version for wheel builds

[emlys]

Description

Fixes #2206, #2483
Added an extra GDAL constraint to the wheel build environment and package dependencies. Removed the auditwheel step from the wheel builds on linux.

Checklist

• Updated HISTORY.rst and link to any relevant issue (if these changes are user-facing)
• Updated the user's guide (if needed)
• Tested the Workbench UI (if relevant)

[emlys]

This is not a necessary change for this PR, but we might as well take advantage of the support for reading a requirements file directly in pyproject.toml.

[phargogh]

Yep, makes sense to consolidate towards pyproject.toml wherever possible!

[emlys]

We're not using optional-dependencies (though maybe we should!), so I removed it for now.

[phargogh]

Thanks @emlys ! I really like the solution of modifying requirements.txt, but wouldn't doing so also cause the version string to be modified since there would now be uncommitted changes? I offered a possible alternative that might be the kind of thing that would modify the library requirements before the wheel is built, which is the kind of thing we used to have to do when setuptools was in its heyday. Looking ahead, there's probably a more modern way to do this since setuptools is probably past its prime.

[phargogh]

Yep, makes sense to consolidate towards pyproject.toml wherever possible!

[phargogh]

Won't this direct modification of requirements.txt result in a .d<today> dirty tag in the version?

Personally, I am hopeful that we might be able to write a new custom command for setup.py since we're still using the setuptools build backend. Something like this, maybe? I think doing this should result in the version being unchanged.

diff --git a/setup.py b/setup.py
index 8dcb559d1..35bca5ca1 100644
--- a/setup.py
+++ b/setup.py
@@ -5,6 +5,7 @@ import subprocess
 import numpy
 from Cython.Build import cythonize
 from setuptools import setup
+from setuptools.command.bdist_wheel import bdist_wheel
 from setuptools.command.build_py import build_py as _build_py
 from setuptools.extension import Extension

@@ -58,6 +59,12 @@ class build_py(_build_py):
         # then execute the original run method
         _build_py.run(self)

+class CustomWheel(bdist_wheel):
+    def run(self):
+        # Dynamically modify requirements before the wheel is packed
+        self.distribution.install_requires.append("gdal==3.10.*")
+        bdist_wheel.run(self)
+

 setup(
     install_requires=_REQUIREMENTS,
@@ -92,6 +99,8 @@ setup(
     ], compiler_directives={'language_level': '3'}),
     include_dirs=[numpy.get_include()],
     cmdclass={
-        'build_py': build_py
+        'build_py': build_py,
+        'build_wheel': CustomWheel,
+        'bdist_wheel': CustomWheel,
     }
 )

[phargogh]

And also, I'm sorry about the deep setuptools stuff! It was always arcane, and I've spent way too much time mired in setuptools subclasses over the years, so this kind of modification seems like it should work, but there is probably a way better way to to it.

[phargogh]

Huh, looks like it didn't post my response as commented, github is saying that I saw the review but didn't actually post my response? I'll try resubmitting here.

[emlys]

@phargogh that's a very good point, thanks for catching that! The custom build class you suggest does seem like a valid solution. A couple other possibilities come to mind:

• We could put the gdal constraint in an optional dependency group called something like wheel - then pip install natcap.invest[wheel] would require that specific version, but pip install natcap.invest would not.
• We could put the gdal constraint in a separate requirements file e.g. wheel-constraints.txt and tell users to install as pip install natcap.invest -c wheel-constraints.txt. I think this is worse than the optional-dependency approach because it's not embedded in the package metadata.

I do want to prioritize moving in the direction of using pyproject.toml, and I think there's a good case to be made that our requirements should eventually live in [project.dependencies] (and generate requirements.txt from that as needed, not the other way around). Particularly because pyproject.toml is a setuptools concept that's specified in multiple PEPs, whereas requirements.txt is ultimately a pip-specific format. And TOML has been supported in the standard library since python 3.11. With that in mind, I kind of like the optional-dependency approach because it could be fully static in pyproject.toml. But, it only works if the user knows to install natcap.invest[wheel] and not natcap.invest. What do you think?

[phargogh]

Of the two options you mention, I too prefer the pip install natcap.invest[wheel'] approach, but I also worry that it's already far too easy for someone to pip install natcap.invest, get a wheel and then wonder why it doesn't work.

On the specific point about requirements.txt being a pip format and not in setuptools, that is a really good point! And I agree with you that the dependencies should be defined in pyproject.toml instead of in requirments.txt. But for the case where we are modifying the wheel build config, we can still remove the install_requires parameter to setup() in setup.py and setuptools still reads the parameters from pyproject.toml and we can still modify them that way for the wheel build. Would that change things? Or do you think the extras approach would still be preferable?

[emlys]

Ok, you've convinced me that the requirement belongs in the regular wheel without needing a modifier. The custom wheel class you suggest seems like a good solution! I'm just a little hesitant to add more code to setup.py.

A possible alternative I found in PEP 517 would be to implement an in-tree build backend that overrides prepare_metadata_for_build_wheel. This approach comes recommended by the setuptools docs, and it has the advantage of not further tying us to setup.py. It also supports passing in custom configuration options to the build frontend. So we could do something like python -m build --wheel -Cwheel_requires='gdal=3.10.*', which seems like a neat alternative to reading from an env variable.

However, this is at least as arcane as the the custom class approach, and requires slightly more code and an additional file. What do you think?

[phargogh]

I think this is a great solution! Definitely still a little arcane, but I have to say that it is so nice to have officially-supported build hooks like this that are well defined, well-documented and unlikely to break in a bugfix release of some distutils derivative that you didn't know your builds depended on. It's still a little obscure, but SO much better than the blog-based rumors and old stackoverflow posts that we had to rely on for setup.py modifications.

So yes, I think this would be AWESOME and I totally agree that it'd be way better to move this away from setup.py. hopefully the new file won't be a big burden to write, but I don't imagine it would be for this particular case.