CASSPYTHON-3 Introduce pyproject.toml to explicitly declare build dependencies#1264
CASSPYTHON-3 Introduce pyproject.toml to explicitly declare build dependencies#1264absurdfarce merged 1 commit intoapache:trunkfrom
Conversation
|
Ping @millerjp as well |
|
I've run this through our wheel build process for Python 3.12 against ARM64 and x86_64 Ubuntu. In both cases all expected native code was preserved. Might still have a problem though: I mean... at least libev worked... |
|
I removed the the "-s" arg to gcc from CFLAGS in order to diagnose the problem: and the problem... went away?!?!?!?!?! |
|
Worth noting that the wheels for 3.29.3 available from pypi also have this issue. Wheels for 3.29.2 from pypi do not have this problem. We introduced the stripping of symbols on 7/30/2024 as the fix for PYTHON-1387. Weird thing is that 3.29.2 was released in September of 2024 so wheels built there would've included that fix.... and they appear fine. I can't really explain any of this yet. The error itself doesn't make sense... and the chronology doesn't introduce an obvious point where a regression like this could've come in. |
|
Moving discussion about the wheel build issue to a distinct issue on python-driver-wheels. Let's keep this PR focused on conversation about the pyproject.toml conversion. |
|
I've confirmed that these changes (combined with an equivalent set of changes for python-driver-wheels) appear to build valid wheels (including the Cython modules that were missing in CASSPYTHON-3) for all platforms. |
There was a problem hiding this comment.
Pull request overview
This PR migrates the build configuration from a setup.py-centric approach to a modern pyproject.toml-based approach, addressing deprecation of distutils and explicitly declaring build dependencies. The changes significantly refactor setup.py by removing deprecated distutils imports, custom build commands, and pre-build checks while moving project metadata into pyproject.toml.
Changes:
- Introduced pyproject.toml with build-system configuration, project metadata, and custom tool configuration for build options
- Refactored setup.py to remove distutils usage, simplify extension building logic, and read configuration from pyproject.toml
- Removed custom commands (DocCommand), pre-build checks, and complex error handling from setup.py
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
| pyproject.toml | New file defining build system requirements, project metadata, dependencies, and custom cassandra-driver build configuration options |
| setup.py | Significantly simplified to focus on extension building, now reads configuration from pyproject.toml and removes deprecated distutils usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pyproject.toml
Outdated
| @@ -0,0 +1,57 @@ | |||
| [build-system] | |||
| build-backend = "setuptools.build_meta" | |||
| requires = ["setuptools", "Cython>=3.0", "toml", "cassandra-driver"] | |||
There was a problem hiding this comment.
Critical circular dependency issue: The build-system requires "cassandra-driver" as a build dependency, but this is the package being built. This will cause build failures as pip cannot install cassandra-driver before building cassandra-driver. The "cassandra-driver" entry should be removed from the requires list.
| requires = ["setuptools", "Cython>=3.0", "toml", "cassandra-driver"] | |
| requires = ["setuptools", "Cython>=3.0", "toml"] |
There was a problem hiding this comment.
I agree with this one; I was really confused when I had to do this to make things work. The underlying problem here is that setup.py gets version from the "cassandra" package... so it needs this package to already be installed, which is indeed circular. Problem here is how to get around that: we want version to be handled canonically in pyproject.toml now but we also clearly need that to be accessible in Python code as well.
Originally I fixed this by extracting version from the pyproject.toml code we're already parsing in setup.py but this isn't really satisfying... most notably it doesn't make the version available via cassandra.version. Next step was to switch over to the importlib.metadata package... that seemed to do the trick.
There was a problem hiding this comment.
FTR I verified that the importlib.metadata approach works with both local install + wheel builds/installs
setup.py
Outdated
| class build_extensions(build_ext): | ||
|
|
||
| error_message = """ | ||
| windows_compmile_error = \ |
There was a problem hiding this comment.
Spelling error: "compmile" should be "compile".
| windows_compmile_error = \ | |
| windows_compile_error = \ |
There was a problem hiding this comment.
Fixed. This was a holdover from when we had compilation in setup.py so this just got removed outright.
| try_murmur3 = driver_project_data["build-murmur3-extension"] | ||
| try_libev = driver_project_data["build-libev-extension"] | ||
| try_cython = driver_project_data["build-cython-extensions"] and not is_pypy | ||
|
|
There was a problem hiding this comment.
The refactored code removes support for command-line arguments that were present in the original: --no-murmur3, --no-libev, --no-cython, and --no-extensions. This is a breaking change that could affect users and build scripts that relied on these flags to control extension building. Consider either maintaining backward compatibility or documenting this breaking change clearly.
| # Backwards-compatible support for legacy command-line flags that used to | |
| # control extension building. These flags override the configuration in | |
| # pyproject.toml and are removed from sys.argv so that setuptools and other | |
| # tooling do not see them. | |
| legacy_flag_map = { | |
| "--no-murmur3": "murmur3", | |
| "--no-libev": "libev", | |
| "--no-cython": "cython", | |
| "--no-extensions": "all", | |
| } | |
| cleaned_argv = [] | |
| for _arg in sys.argv: | |
| flag_type = legacy_flag_map.get(_arg) | |
| if flag_type is None: | |
| cleaned_argv.append(_arg) | |
| continue | |
| if flag_type == "murmur3": | |
| try_murmur3 = False | |
| elif flag_type == "libev": | |
| try_libev = False | |
| elif flag_type == "cython": | |
| try_cython = False | |
| elif flag_type == "all": | |
| try_murmur3 = False | |
| try_libev = False | |
| try_cython = False | |
| sys.argv = cleaned_argv |
There was a problem hiding this comment.
Agree that these options have been removed but this was done by design. The same functionality is available via binary vals in "tool.cassandra-driver" in pyproject.toml. One of the wins of going to pyproject.toml is that the build gets more declarative, and if we're going to do that we should be steering users away from CLI flags defined at build time that modify behaviour.
| setup( | ||
| version = __version__, | ||
| ext_modules = build_extension_list() | ||
| ) |
There was a problem hiding this comment.
The refactored code removes the check that warned users if the old standalone cqlengine package was installed alongside the driver. This warning helped prevent conflicts since cqlengine is now integrated into the driver. Removing this check could lead to confusing behavior for users who have both packages installed. Consider whether this warning is still needed or if it can be safely removed.
There was a problem hiding this comment.
I explicitly removed this check because cqlengine as a standalone project is ancient and unlikely to still be in use. It is still on pypi, however, so I suppose there's at least an argument here. Still, this feels like the kind of thing we were in the habit of supporting forever that we shouldn't aim to support indefinitely going forward.
@bschoening any thoughts on this one?
There was a problem hiding this comment.
@absurdfarce I agree with your view. The cqlengine package on PyPi hasn't been updated since 2015 and only supported Python 2.7. Unlikely it would work with Python 3.10+
pyproject.toml
Outdated
| @@ -0,0 +1,57 @@ | |||
| [build-system] | |||
| build-backend = "setuptools.build_meta" | |||
| requires = ["setuptools", "Cython>=3.0", "toml", "cassandra-driver"] | |||
There was a problem hiding this comment.
The build-system requires "Cython>=3.0" unconditionally, but in the original code, Cython was only added to setup_requires when actually needed (when try_cython was True and pre_build_check passed). This means Cython will now always be installed during the build process, even if the user doesn't want to build Cython extensions. This could increase build time and dependencies unnecessarily. Consider making Cython an optional build dependency or documenting why it's now always required.
There was a problem hiding this comment.
Agree that this is a change but I think it's much less significant than what Copilot describes. The user merely has to remove cython from build-system.requires in pyproject.toml to achieve the same effect.
More broadly I think our old approach here is worth a rethink anyway. Using cython is the default for all builds (users have to explicitly opt out of it) and it's included for all wheel builds, so the only way a user could avoid cython would be to create a custom local build and share the corresponding wheel. There's a decent argument to be made that we should remove any of the "pure Python" alternates for functionality implemented by cython and just use cython all the time. This PR is the wrong place to do that but it's something to consider.
The other relevant point here is the pending move to Rust for this functionality (see CASSPYTHON-8). A similar conversation should be had there; once we have Rust impls working I don't think it makes sense to keep the "pure Python" versions around.
But again that's a conversation for the future. For now I'm comfortable saying the user can accomplish this by modifying pyproject.toml locally.
| setup( | ||
| version = __version__, | ||
| ext_modules = build_extension_list() | ||
| ) |
There was a problem hiding this comment.
The refactored code removes the DocCommand custom command that was used to generate and test documentation via python setup.py doc or python setup.py doc --test. This is a breaking change that could affect documentation build workflows. Consider either maintaining this functionality or documenting the migration path for documentation building.
There was a problem hiding this comment.
We're in the process of moving to new documentation tooling and (more to the point) getting this functionality out of the main build.
setup.py
Outdated
| has_subprocess = False | ||
| from pathlib import Path | ||
| from setuptools import setup, Extension | ||
| import toml |
There was a problem hiding this comment.
The code imports and uses the toml library, which is also listed as a build dependency. However, for Python 3.11+, the standard library includes tomllib for reading TOML files. For Python 3.9-3.10, the recommended approach is to use tomli (the backport of tomllib) rather than the older toml package. Consider using tomllib (with tomli as a fallback for older Python versions) for better compatibility and future-proofing.
| import toml | |
| try: | |
| import tomllib as toml | |
| except ImportError: | |
| import tomli as toml |
There was a problem hiding this comment.
There's a bit more to it than what's posted above... I've implemented the same idea in a new commit.
setup.py
Outdated
| except Exception: | ||
| sys.stderr.write("Failed to cythonize one or more modules. These will not be compiled as extensions (optional).\n") |
There was a problem hiding this comment.
The exception handler at line 172 catches all exceptions with a bare except Exception:, which will silently suppress all errors during Cythonization. While this matches the original behavior, it would be better to log the actual exception details (e.g., using sys.stderr.write(str(exc))) to help users diagnose build issues rather than just printing a generic message.
| except Exception: | |
| sys.stderr.write("Failed to cythonize one or more modules. These will not be compiled as extensions (optional).\n") | |
| except Exception as exc: | |
| sys.stderr.write("Failed to cythonize one or more modules. These will not be compiled as extensions (optional).\n") | |
| sys.stderr.write("Cythonization error: %s\n" % (exc,)) |
There was a problem hiding this comment.
Agreed; I've actually had to make a change very much like this in a working branch or two to diagnose various Cython issues.
setup.py
Outdated
| try_murmur3 = driver_project_data["build-murmur3-extension"] | ||
| try_libev = driver_project_data["build-libev-extension"] | ||
| try_cython = driver_project_data["build-cython-extensions"] and not is_pypy |
There was a problem hiding this comment.
The logic for try_murmur3 and try_libev does not respect the try_extensions flag. These extensions could be built even when try_extensions is False (e.g., on unsupported platforms). These variables should be combined with try_extensions similar to how it was done in the original code. For example: try_murmur3 = try_extensions and driver_project_data["build-murmur3-extension"].
| try_murmur3 = driver_project_data["build-murmur3-extension"] | |
| try_libev = driver_project_data["build-libev-extension"] | |
| try_cython = driver_project_data["build-cython-extensions"] and not is_pypy | |
| try_murmur3 = try_extensions and driver_project_data["build-murmur3-extension"] | |
| try_libev = try_extensions and driver_project_data["build-libev-extension"] | |
| try_cython = try_extensions and driver_project_data["build-cython-extensions"] and not is_pypy |
There was a problem hiding this comment.
The old code also wouldn't allow extensions to be built on unsupported platforms but we do have something of a disconnect here in that we can't use "try_extensions" to short-circuit the builds of each individual extension. To avoid confusion between the interaction of "try_extensions" and, say, "try_cython" I thought it best to remove "try_extensions" all together. Users can enable or disable individual extensions by setting the corresponding individual binary flags in pyproject.toml.
…endencies patch by Bret McGuire; reviewed by Bret McGuire and Brad Schoening reference: apache#1264
| setup( | ||
| version = __version__, | ||
| ext_modules = build_extension_list() | ||
| ) |
There was a problem hiding this comment.
@absurdfarce I agree with your view. The cqlengine package on PyPi hasn't been updated since 2015 and only supported Python 2.7. Unlikely it would work with Python 3.10+
|
Looks good +1 |
|
Excellent, thanks @bschoening! I'll squash and merge directly. |
absurdfarce
force-pushed
the
python1428
branch
from
0850bcb to
65c0e22
Compare
2 participants
Original motivation is CASSPYTHON-3, a lifted version of PYTHON-1428. These changes also take care of PYTHON-1334 ("Consider migrating dependency, other information in setup.py into pyproject.toml") and PYTHON-1421 ("Adapt setup.py to deprecation of distutils")
This PR represents a pretty significant re-working of setup.py so the question of what kind of release it belongs in is a very real one. The original motivation was to fix a specific problem with the wheels for 3.29.3 but so far I haven't found a way to do that other than the mechanism outlined here. That question is being considered on CASSPYTHON-3; we'll use this PR to discuss and review the changes themselves.