Move to a fully pyproject.toml based build #878
Conversation
vyasr
requested review from
b-butler and
klywang
and removed request for
a team
pyproject.toml
Outdated
| # This software is licensed under the BSD 3-Clause License. | ||
| [build-system] | ||
| build-backend = "setuptools.build_meta" | ||
| requires = ["setuptools"] |
There was a problem hiding this comment.
setuptools >= 64 is required for editable installs (pip install -e) with pyproject.toml-only builds. I don't think we need to encode that requirement here, just wanted to mention it.
There was a problem hiding this comment.
OK, this is quite odd. I tried changing our CI to do a non-editable pip install of the package, and I started seeing failures in test_single_writer_multiple_reader_same_instance. Here's one such example. The problem seems isolated to specific combinations in the test matrix, so I suspect that the issue is in some invalid assumption of the test. It's completely unrelated to this PR's changes, so I just bumped the setuptools requirement to move forward and continue doing an editable install. Just wanted to document the issue here in case someone sees it elsewhere.
Codecov Report
@@ Coverage Diff @@
## master #878 +/- ##
=======================================
Coverage 86.64% 86.64%
=======================================
Files 41 41
Lines 4575 4575
Branches 900 900
=======================================
Hits 3964 3964
Misses 431 431
Partials 180 180 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This reverts commit 9e03309.
vyasr
changed the title
| @@ -45,6 +43,8 @@ repos: | |||
| ^doc/| | |||
| ^tests/| | |||
| ) | |||
| additional_dependencies: | |||
| - toml | |||
There was a problem hiding this comment.
Is there a newer pydocstyle that doesn’t require this additional dependency to be specified?
There was a problem hiding this comment.
There is not. I opened an issue about this a while ago but never received a response from the devs. Maybe worth another ping.
There was a problem hiding this comment.
I would add a comment linking to this issue.
There was a problem hiding this comment.
| - toml | |
| - toml # See https://github.com/PyCQA/pydocstyle/issues/603 |
| description-file = README.md | ||
|
|
||
| [bdist_wheel] | ||
| python-tag = py3 |
There was a problem hiding this comment.
Did we lose the python tag specifier?
There was a problem hiding this comment.
This is outdated. I found a few different random threads indicating that this section is no longer necessary, but the most authoritative source is the PyPA's wheel documentation and the only usage of this section I found anywhere was around explicitly declaring that a wheel is universal. I also tested building wheels both with and without this section prior to moving config info from setup.cfg to pyproject.toml, and then I build wheels again after the switch. In all cases the produced wheel had the same tag: signac-1.8.0-py3-none-any.whl.
Just to clarify, are you saying you don't want to approve because 1) you don't have the context for the change to make a call, 2) you don't think it's quite ready yet and won't have time to come back to it, or 3) you have reservations about the change? In either case 1 or 2 I'm fine moving forward (I think I've tackled everything needed here), but if it's 3 please do stop me. Even if you don't plan to follow up much yourself, I can touch base with other devs who have more bandwidth at the moment. I don't want to force such a change unilaterally. I do think it is important for us to keep up with changes in this space though; I have found encountered various minor incompatibilities around edge cases of tool interop, and I only expect those to grow as the ecosystem's preferred solutions mature and the old tools are slowly phased out (even if they continue mostly working for years). |
Co-authored-by: Kelly Wang <[email protected]> Co-authored-by: Bradley Dice <[email protected]>
There was a problem hiding this comment.
@vyasr My reasons for not approving earlier were both (1) and (2). I haven't done much with non-setup.py builds and lack deep (and up to date) expertise in this area. I am deferring to your knowledge and I am mostly reviewing this code for perceived completeness. I also don't think I can allocate the time to learn that at the level that I would need at this exact moment (because of holidays and I'm recovering from illness). I trust your judgment and appreciate your responses to my questions, so I'm approving.
I saw you opened a PR to signac-flow. Please consider making the same kinds of changes for signac-dashboard, if you are able. That would be ideal for consistency.
Description
This PR removes setup.py and setup.cfg entirely, migrating all project and build configuration information into pyproject.toml. In the process, all linter configs have also been moved into pyproject.toml. The exception is flake8, which does not (and will not) support pyproject.toml, so the flake8 configuration is now stored in the .flake8 file, which is specific to this linter. Additionally, bump2version also does not support pyproject.toml (although unlike flake8 the proposal has not been entirely rejected, so it may eventually), so that configuration has also been moved to a project-specific .bumpversion file.
Motivation and Context
Various changes to Python packaging over the last 6 or 7 years have moved towards more static packaging and towards storing data in a backend-agnostic format. These changes allow these of setuptools alternatives (like flit) as well as more reproducible builds based on build isolation into virtual environments that provide all necessary build dependencies. Direct invocation of setup.py has been deprecated in the process. The changes in this PR modernize signac's build system for compatibility with these new approaches.
Checklist: