Fix install_requires following PEP-508 #3114

item4 · 2018-06-30T06:06:55Z

What do these changes do?

pipenv can not support code-based deps list.
this PR change install requires list following PEP-508

Please see pypa/pipenv#2473 for details.

Are there changes in behavior for the user?

no, there is only change to installation

Related issue number

pypa/pipenv#2473

Checklist

Please help me to check below. I do not check it myself

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

pipenv can not support code-based deps list Please see pypa/pipenv#2473 for details.

codecov-io · 2018-06-30T06:35:27Z

Codecov Report

Merging #3114 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3114   +/-   ##
=======================================
  Coverage   98.06%   98.06%           
=======================================
  Files          43       43           
  Lines        7837     7837           
  Branches     1354     1354           
=======================================
  Hits         7685     7685           
  Misses         56       56           
  Partials       96       96

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bbc255...3a17df4. Read the comment docs.

uranusjr · 2018-06-30T20:21:17Z

Another related issue: #2703. The approach used here is considered the best way currently available to specify environment-specific requirements. Both the Python-based (currently in use) and extra_requires (mentioned in #2703) are no longer recommended, as they may cause problems in dependency resolution in package managers, and could provide incorrect metadata reporting on PyPI.

webknjaz · 2018-06-30T21:37:22Z

setup.py

+    'multidict>=4.0,<5.0',
+    'async_timeout>=3.0,<4.0',
+    'yarl>=1.0,<2.0',
+    'idna-ssl>=1.0;python_version<"3.7"',


AFAIR pip (setuptools?) doesn't support this kind of notation in install_requires (at least in certain range of versions).
I'd say it's better to go for extras-based syntax:

extra_requires = { ':python_version<"3.7"': [ 'idna-ssl>=1.0', ], }

Is there any way to test this? There have been quite a few improvements in this area in pip v10.x, and support for PEP 508 came up more than once.

@webknjaz it does today. And at least bundled pip for 3.5 support that. For older I think we shouldn't care.

In anyway, rule of thumb is: before install anything in new virtualenv, upgrade pip and setuptools first.

webknjaz · 2018-06-30T21:40:55Z

@uranusjr while it might be not recommended we target a wider range of envs and extras-based notation is better in this case. I'll need to check feature-support first.

uranusjr · 2018-07-01T07:00:07Z

@webknjaz Unfortunately extra_requires also has issues during detection. I generally do not recommend sticking to the old syntax, since the current recommendation has been stable for more than a year, and newer versions of Setuptools is available to everyone to upgrade.

If Setuptools compatibility is an issue, Pytest’s approach may be more appropriate:
https://github.com/pytest-dev/pytest/blob/master/setup.py#L26

I would also like to mention that I have not seen any real-life reports on Setuptools support on installs_require markers being an issue.

Victor-Savu · 2018-07-01T07:27:21Z

@uranusjr I only saw the warning at the bottom of this wheel documentation section but I don't know if it is still valid.

uranusjr · 2018-07-01T08:59:44Z

@Victor-Savu The warning only applies for extra; environment markers do work in pip 9.

Update: There is a table in the middle of the PEP document listing available markers. All of them except extra work in relatively up-to-date Setuptools and pip.

https://www.python.org/dev/peps/pep-0508/

kxepal · 2018-07-01T09:16:24Z

I'm +1 this patch. While I share @webknjaz concerns about old setuptools/pip, today that shouldn't be a problem and, in general, I think we shouldn't let them slow down us, because users may easily upgrade them at any moment for free.

-1 for extras - that's not right solution, because:

It's easy to forget about that extra (how would you know about it without reading the sources?)
You will break all the packages that depends on aiohttp - they don't have any extras in own install_requires list, so they wouldn't get that package
All downstreams would have to maintain own if-condition: if 3.5, then use aiohttp's extra, if not - don't. Not cool.

item4 · 2018-07-05T16:51:06Z

I changed code. please review again.

kxepal

This iteration requires some additional work, but, honestly, we don't have to:

Copy code from third-party project and deal with licenses;
Have such wide/universal setuptools support - modern ones are enough and their upgrade is not a big deal for users.
Complicated setup.py means complicated support of it. I think we shouldn't add additional rare cases by our own unless there would be bug reports about.

kxepal · 2018-07-05T18:10:51Z

setup.py

+    * https://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-platform-specific-dependencies
+    """
+    try:
+        version = pkg_resources.parse_version(setuptools.__version__)


You forgot the import

I will fix it

kxepal · 2018-07-05T18:11:18Z

setup.py

+        version = pkg_resources.parse_version(setuptools.__version__)
+        if version >= pkg_resources.parse_version("36.2.2"):
+            return 2
+        if version >= pkg_resources.parse_version("0.7.2"):


Should we care about such old versions? I think we shouldn't.

kxepal · 2018-07-05T18:13:50Z

setup.py

-    install_requires.append('idna-ssl>=1.0')
+def get_environment_marker_support_level():
+    """
+    Copied from https://github.com/pytest-dev/pytest/blob/master/setup.py


If you copy code from third party project, you need to add this reference to NOTICE file as ASF license stands for.

I will drop this copied code

kxepal · 2018-07-08T09:03:41Z

setup.py

-
-if sys.version_info < (3, 7):
-    install_requires.append('idna-ssl>=1.0')
+if setuptools_version >= (36, 2, 2):


environment markers are supported since 20.5 version, but really get worked since 21.0. No need to require such high setuptools version.

Also, version 21.0 released two years ago. Once again, do we need any checks against setuptools version at all? Unlikely any sane user will install latest aiohttp having legacy package management toolset.

kxepal · 2018-07-08T09:04:36Z

setup.py

+    'async_timeout>=3.0,<4.0',
+    'yarl>=1.0,<2.0',
+]
+extras_require = {}


extras seems to be unused.

item4 · 2018-07-08T17:37:00Z

@kxepal I think checking setupools version is useless and I want to reset --hard to first commit. but someone commented setuptools versioni issue. I want to hear opinion of member of aiohttp

kxepal · 2018-07-08T17:41:49Z

@webknjaz
Are you ok to not check setuptools version?

asvetlov · 2018-07-09T18:48:10Z

Dropping setuptools version check is ok for me: env markers support is pretty old.

outdated

webknjaz · 2018-07-10T00:24:34Z

Alright, I'm okay with that. It's just me having a habit to target maximum compatibility :)

Since we chose to not care about compatibility with ancient setuptools.

lock · 2019-10-28T03:04:13Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

Fix install_requires following PEP-508

2c3bcbf

pipenv can not support code-based deps list Please see pypa/pipenv#2473 for details.

sdispater mentioned this pull request Jun 30, 2018

SolverProblemError when installing aiohttp python-poetry/poetry#274

Closed

3 tasks

webknjaz reviewed Jun 30, 2018

View reviewed changes

webknjaz self-assigned this Jun 30, 2018

Check setuptools version to change setting (using pytest's way)

10cc785

kxepal requested changes Jul 5, 2018

View reviewed changes

item4 added 2 commits July 6, 2018 13:56

Change logic to just check setuptools version

c87fbaa

Fix wrong tuple definition

74e1041

kxepal previously requested changes Jul 8, 2018

View reviewed changes

Change code to apply @kxepal review

3a17df4

Revert to just env marker in install_requires

e0de8fd

Since we chose to not care about compatibility with ancient setuptools.

webknjaz approved these changes Jul 10, 2018

View reviewed changes

webknjaz merged commit ce50b03 into aio-libs:master Jul 10, 2018

item4 deleted the pep508 branch July 10, 2018 05:32

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix install_requires following PEP-508 #3114

Fix install_requires following PEP-508 #3114

item4 commented Jun 30, 2018

codecov-io commented Jun 30, 2018 •

edited

uranusjr commented Jun 30, 2018

webknjaz Jun 30, 2018

Victor-Savu Jul 1, 2018 •

edited

kxepal Jul 1, 2018

webknjaz commented Jun 30, 2018

uranusjr commented Jul 1, 2018

Victor-Savu commented Jul 1, 2018

uranusjr commented Jul 1, 2018 •

edited

kxepal commented Jul 1, 2018

item4 commented Jul 5, 2018

kxepal left a comment

kxepal Jul 5, 2018

item4 Jul 6, 2018

kxepal Jul 5, 2018

item4 Jul 6, 2018

kxepal Jul 5, 2018

item4 Jul 6, 2018

kxepal Jul 8, 2018

kxepal Jul 8, 2018

kxepal Jul 8, 2018

item4 commented Jul 8, 2018

kxepal commented Jul 8, 2018

asvetlov commented Jul 9, 2018

webknjaz commented Jul 10, 2018

lock bot commented Oct 28, 2019

Fix install_requires following PEP-508 #3114

Fix install_requires following PEP-508 #3114

Conversation

item4 commented Jun 30, 2018

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

codecov-io commented Jun 30, 2018 • edited

Codecov Report

uranusjr commented Jun 30, 2018

Choose a reason for hiding this comment

Victor-Savu Jul 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz commented Jun 30, 2018

uranusjr commented Jul 1, 2018

Victor-Savu commented Jul 1, 2018

uranusjr commented Jul 1, 2018 • edited

kxepal commented Jul 1, 2018

item4 commented Jul 5, 2018

kxepal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

item4 commented Jul 8, 2018

kxepal commented Jul 8, 2018

asvetlov commented Jul 9, 2018

webknjaz commented Jul 10, 2018

lock bot commented Oct 28, 2019

codecov-io commented Jun 30, 2018 •

edited

Victor-Savu Jul 1, 2018 •

edited

uranusjr commented Jul 1, 2018 •

edited