fix: syntax error in setup.py + modernize #237
Conversation
|
Let me know when you merge this @lindstro ? |
|
LGTM |
| class NumpyImport: | ||
| def __repr__(self): | ||
| import numpy as np | ||
|
|
||
| return np.get_include() |
There was a problem hiding this comment.
I don't understand the point of this class. I think it's meant to defer the import of numpy, but you call str(NumpyImport()) in the arguments to setup, so it will be evaluated and import numpy at startup anyway. Was this change made redundant by adding pyproject.toml with build system requires?
There was a problem hiding this comment.
@william-silversmith Can you please address these questions?
There was a problem hiding this comment.
Yes, the point of the script is to delay the import of numpy. Originally, importing numpy concurrently with installation would cause the setup script to crash as numpy would not be found. This was a solution I found on StackOverflow.
Later on, new python versions would not properly render the repr, so it was necessary to add str to get it to evaluate properly, but you might be right that it has rendered the delay mechanism superfluous.
There was a problem hiding this comment.
@william-silversmith Thanks for explaining (and for contributing this solution).
| [build-system] | ||
| requires = [ | ||
| "setuptools", | ||
| "wheel", |
There was a problem hiding this comment.
wheel is from some outdated documentation; you probably don't need it for your build. It should be automatically installed where necessary.
There was a problem hiding this comment.
@william-silversmith Here's a second question that you may be able to answer. I unfortunately have minimal knowledge of Python, though I suspect it is related to our zfpy-wheels repo for building and uploading wheels to PyPI.
Fixes the syntax error, cythonizes dynamically (may make this run on more platforms), sets language level to 3 (Python 2 is EOL for years now) to remove a warning, and adds license and classifiers so PyPI can categorize it better.
Loads numpy dynamically so it won't crash if numpy was installed at the same time as the setup call is run.
Uses numpy for installation, but oldest-supported-numpy for isolated compilation for py38 via pyproject.toml
Adds MANIFEST.in to ensure sdist includes source files, LICENSE, etc.
Resolves #233
This was moved from #236