Finish pyproject.toml migration
#382
Conversation
|
@Saransh-cpp @DimitriPapadopoulos, could both of you please take a look? 🙂 |
|
This pull request introduces 2 alerts when merging ac5ccf0 into 552f437 - view on LGTM.com new alerts:
|
This comment was marked as resolved.
This comment was marked as resolved.
jakirkham
force-pushed
the
update_bld
branch
5 times, most recently
from
f5c469b to
1625526
Compare
|
Note RTD needs fixes outside of this. Handling in PR ( #383 ). |
|
This pull request introduces 2 alerts when merging 1625526 into 552f437 - view on LGTM.com new alerts:
|
As `setuptools` will handle this for us, go ahead and drop the `cythonize` calls.
Since these build requirements are already in `pyproject.toml` and that is the preferred way forward, drop them from `setup.py`.
jakirkham
changed the title
pyproject.tomlpyproject.toml migration
Windows defaults to `int32` so won't display those `dtype`s. Whereas UNIX defaults to `int64` and so won't display those `dtype`s. This creates mismatches between the representations in docstrings between the two platforms. For simplicity just use `int16`, which both platforms will represent the same. This should ensure consistency with doctests.
This was used to configure compilers on Windows in the past. Particularly it was used to configure Python 2.7 Windows builds. However as of Python 3.5 and UCRT on Windows, this is unnecessary. Further Python 2.7 was improved by a one off Windows Python compiler installer. In any event this can safely be dropped. It is not used in this repo and shouldn't be needed outside of it.
|
Please let me know if there's anything else needed here |
There was a problem hiding this comment.
Thanks, @jakirkham and @DimitriPapadopoulos! No more comments from my side 🚀
jakirkham
requested review from
DimitriPapadopoulos and
joshmoore
and removed request for
DimitriPapadopoulos
| "version", | ||
| ] | ||
| classifiers = [ | ||
| "Development Status :: 4 - Beta", |
There was a problem hiding this comment.
Perhaps outwith this PR but is "Beta" still appropriate?
There was a problem hiding this comment.
Good question. Filed as issue ( #394 )
| @@ -99,16 +99,11 @@ the repository, you can do something like the following:: | |||
| $ mkdir -p ~/pyenv/numcodecs-dev | |||
| $ virtualenv --no-site-packages --python=/usr/bin/python3.9 ~/pyenv/numcodecs-dev | |||
| $ source ~/pyenv/numcodecs-dev/bin/activate | |||
| $ pip install -r requirements_dev.txt | |||
| $ python setup.py build_ext --inplace | |||
| $ pip install -e .[docs,test,msgpack,zfpy] | |||
There was a problem hiding this comment.
Would it be "normal" to also have an "all" alias to cover these?
There was a problem hiding this comment.
Yeah agree that would be useful, but unless we write our own (which doesn't seem very DRY friendly) or handle this in setup.py. Am not aware of any existing option. Also please see this upstream issue ( pypa/pip#4340 ).
This should always be available as `cython` is a build requirement. So simplify to just use Cython and skip `setuptools` here.
|
Thanks all! 🙏 Going to go ahead and merge this in. Happy to follow up on anything else after |
Moves the bulk of metadata, requirements, etc. to
pyproject.toml. As there are still extensions that need to be built, we still rely onsetuptoolsto do that. Also skip runningcythonizeon extensions assetuptoolsalready does this for us. Additionally try usingbuild_extfrom Cython to perform the build. This way we can even do parallel builds when passing-j <#>tobuild_ext. Finally switch to latest (as opposed to legacy)setuptools, since the legacy case should no longer be needed.Closes #364
TODO: