Remove pip install setuptools-rust from setup.py #533
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The setup.py has long carried around a pip install fallback if setuptools-rust can't be imported. This was necessary as prior to the introduction of pep-517 there was no way to express a build-system requirement in python packaging. There is the setup_requires field in the setuptools metadata, but this would both trigger the install too late (as we need to import setuptools-rust prior to running setuptools.setup()) and also fallback to using easy_install instead of pip which complicates installation. With pep-517 we list setuptools-rust as a build system requirement in the pyproject.toml and pip will just install that prior to trying to run the setup.py in the sdist. But, for users on older versions of pip, before pep-517 was supported, this caused a cryptic error as there would just be an import error that setuptools_rust can't be found. The pip install fallback on import error was added to try and make installation seamless for those users. However, pep-517 support has been around in pip for sufficiently long at this point I think we can safely remove this fallback as it provides more harm than good now. In it's place the README is updated to document that you need a newer version of pip or to manually install setuptools-rust prior to trying to build retworkx from source. This should hopefully be sufficient for users on older versions of pip where they still might encounter issues.
Closed
40 tasks
CI is failing without the pip install fallback because tox is calling `python setup.py sdist` to build the sdist prior to installing. But this isn't processing the pyproject.toml because nothing is checking for it. In an attempt to make tox try and respect the pyproject.toml this turns on the isolated_build option which creates a venv for building which hopefully will mean tox looks at the pyproject.toml prior to running `python setup.py sdist`.
Using build isolation tox is failing because we don't explicitly list setuptools as the build system backend in the pyproject.toml. This isn't an issue with pip because it implicitly uses setuptools as the backend (although via a legacy interface) if the build-backend isn't specified. However, tox doesn't have this default and requires the build system backend be specified. This commit corrects this oversight and explicitly lists setuptools as the build-backend field in the pyproject.toml.
Pull Request Test Coverage Report for Build 1594098689
💛 - Coveralls |
IvanIsCoding
approved these changes
Dec 17, 2021
There was a problem hiding this comment.
Overall this LGTM and I am in favor of merging.
However, if we have the time to spin up virtual environments and discover the minimum version of pip that can be helpful to people that build retworkx from source. This kind of information can be useful for the more advanced users that build from source or maintain things like conda-forge/staged-recipes#17137
| @@ -1,6 +1,7 @@ | |||
| [build-system] | |||
| requires = ["setuptools", "wheel", "setuptools-rust"] | |||
| build-backend = "setuptools.build_meta" | |||
There was a problem hiding this comment.
For those reviewing this PR in the future: build-backend refers to https://github.com/pypa/setuptools/blob/main/setuptools/build_meta.py
Document that at least pip 19.0.0 is needed to install from sdist. Version 19.0.0 added pep 517 support: https://pip.pypa.io/en/stable/news/#v19-0 which is the required feature for pip to parse the pyproject.toml and properly install setuptools-rust before processing the setup.py.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The setup.py has long carried around a pip install fallback if
setuptools-rust can't be imported. This was necessary as prior to the
introduction of pep-517 there was no way to express a build-system
requirement in python packaging. There is the setup_requires field in
the setuptools metadata, but this would both trigger the install too
late (as we need to import setuptools-rust prior to running
setuptools.setup()) and also fallback to using easy_install instead of
pip which complicates installation. With pep-517 we list setuptools-rust
as a build system requirement in the pyproject.toml and pip will just
install that prior to trying to run the setup.py in the sdist.
But, for users on older versions of pip, before pep-517 was supported,
this caused a cryptic error as there would just be an import error that
setuptools_rust can't be found. The pip install fallback on import error
was added to try and make installation seamless for those users. However,
pep-517 support has been around in pip for sufficiently long at this
point I think we can safely remove this fallback as it provides more
harm than good now. In it's place the README is updated to document
that you need a newer version of pip or to manually install
setuptools-rust prior to trying to build retworkx from source. This
should hopefully be sufficient for users on older versions of pip where
they still might encounter issues.