-
Notifications
You must be signed in to change notification settings - Fork 307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use environment markers to define pyblake2 dependency in setup.py #441
Conversation
Allows removing logic from setup.py, resulting in a more consistent package definition across Python versions. Can remove duplicate information from both setup.cfg and tox.ini to use setup.py as a single source of dependency truth. To remove from tox.ini, use the "extras" feature, which bumps the tox min version to 2.4. https://tox.readthedocs.io/en/latest/config.html#conf-extras > A list of “extras” to be installed with the sdist or develop install. > For example, extras = testing is equivalent to [testing] in a pip > install command.
Codecov Report
@@ Coverage Diff @@
## master #441 +/- ##
=======================================
Coverage 78.29% 78.29%
=======================================
Files 14 14
Lines 751 751
Branches 108 108
=======================================
Hits 588 588
- Misses 129 130 +1
+ Partials 34 33 -1
Continue to review full report at Codecov.
|
This is going the opposite direction that I find helpful. I'd rather see everything possible in the setup.cfg akin to rush. Would you be willing to take this in that direction? |
Sure thing. I'll look into it and report back when ready. |
The work porting to setup.cfg stalled due to a pip or setuptools bug: pypa/setuptools#1608 |
@@ -88,7 +78,9 @@ | |||
"tqdm >= 4.14", | |||
], | |||
extras_require={ | |||
'with-blake2': blake2_requires, | |||
'with-blake2': [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this syntax is less compatible than:
'with-blake2:sys_version<"3.6"...': ['pyblake2']
which is less compatible than:
'with-blake2:sys_version=="3.5" or sys_version=="2.7" or ...': ['pyblake2']
The last version supports very old versions of setuptools. Personally, I'm fine with the proposed change - we should just recognize that it's increasing the minimum setuptools version required to build from sdist... and should probably declare that in pyproject.toml.
I see this PR not as going in the opposite direction of rush, but actually a partial step in that direction, and not blocked by the upstream issue. I recommend accepting this PR as presented, perhaps with an additional pyproject.toml to declare the new dependency. |
Allows removing logic from setup.py, resulting in a more consistent
package definition across Python versions.
Can remove duplicate information from both setup.cfg and tox.ini to use
setup.py as a single source of dependency truth. To remove from tox.ini,
use the "extras" feature, which bumps the tox min version to 2.4.
https://tox.readthedocs.io/en/latest/config.html#conf-extras