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

exclude legacy dependencies when using python3 #85

Merged
merged 2 commits into from Apr 14, 2018

Conversation

Projects
None yet
4 participants
@foxxx0
Contributor

foxxx0 commented Apr 13, 2018

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Fix Python 3 compatibility

  • What is the related issue number (starting with #)

I have not opened an issue for this and went straight ahead to fix it.

However, this was reported in the Arch Linux bugtracker as:
https://bugs.archlinux.org/task/58204

  • What is the current behavior? (You can also link to an open issue here)

Currently the setup.py has some dependencies that are incompatible with
Python 3.
Both functools_lru_cache and unittest_mock are embedded in Python 3 by default.
Requiring these backports when using python3 will fail and render the build
unusable.

  • What is the new behavior (if this is a feature change)?

Setuptools provides a nice way to deal with this by using the proper
annotations for the dependencies.
This PR introduces said annotations for dependencies that are incompatible
with Python 3.

  • Other information:
exclude legacy dependencies when using python3
Both functools_lru_cache and unittest_mock are
embedded in python3 by default.
Requiring these backports when using python3
will fail and render the build unusable.

Setuptools provides a nice way to deal with
this by using the proper annotations for
the dependencies.

Signed-off-by: Thore Bödecker <me@foxxx0.de>
@codecov

This comment has been minimized.

codecov bot commented Apr 13, 2018

Codecov Report

Merging #85 into master will increase coverage by 0.32%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
+ Coverage   65.99%   66.32%   +0.32%     
==========================================
  Files          17       17              
  Lines        2941     3014      +73     
==========================================
+ Hits         1941     1999      +58     
- Misses       1000     1015      +15

@webknjaz webknjaz requested a review from jaraco Apr 13, 2018

@webknjaz

Also, there's literally nothing in those packages preventing them from being installed under Python 3:

It's rather a problem with Arch not packaging it.

setup.py Outdated
@@ -45,7 +45,7 @@
),
python_requires='>=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*',
install_requires=[
'backports.functools_lru_cache',
'backports.functools_lru_cache;python_version<"3"',

This comment has been minimized.

@webknjaz

webknjaz Apr 13, 2018

Member

AFAIR env markers support is broken in install_requires= within setuptools or pip and the recommended alternative is to use extras.

This comment has been minimized.

@eli-schwartz

eli-schwartz Apr 13, 2018

env markers work just fine in the setup.py for other python modules, why do you think they exist?

This comment has been minimized.

@webknjaz

webknjaz Apr 14, 2018

Member

Please keep yourself calm. I didn't say that env markers don't work at all. However they only work in certain places, like extras names. Even if this was fixed in install_requires= it doesn't mean that we can use this specific notation, because users still may have outdated setuptools, where it's broken.

This comment has been minimized.

@webknjaz
@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 13, 2018

Still, I think this could be accepted if changed to use env markers properly.

/cc: @jaraco

@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 13, 2018

Just to make it clear again: those dependencies are python 3 compatible

@webknjaz webknjaz added the invalid label Apr 13, 2018

@eli-schwartz

This comment has been minimized.

eli-schwartz commented Apr 13, 2018

Also, there's literally nothing in those packages preventing them from being installed under Python 3:

Did anyone say there was? It is, however, completely useless bloat and possible saves users some time when not having to download and install something unnecessary.

It's rather a problem with Arch not packaging it.

Why should we package completely useless bloat because you don't know how to dependency?

We need to patch this on our side anyway, so we wanted to contribute that back upstream. I think it is rather odd that you're treating our efforts with such derision.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 14, 2018

Did anyone say there was?

Yes, @foxxx0 did:

for dependencies that are incompatible with Python 3.

And I've corrected him, because such things must be documented for historical purposes.

it is rather odd that you're treating our efforts with such derision.

Sorry, I didn't mean to do this. I just want to point out that (1) this patch will break things and I will accept a better version of it, also (2) those packages handle feature-detection and whether to patch imports themselves.

P.S.

you don't know how to dependency

This is mean and totally not true. The fact that we left it this way means that it works in the packaging ecosystem we support officially: pypi+pip+setuptools.

@webknjaz webknjaz merged commit 34664ef into cherrypy:master Apr 14, 2018

7 of 8 checks passed

lgtm analysis: Python Running analyses for revisions
Details
WIP ready for review
Details
ci/circleci: linux-build Your tests passed on CircleCI!
Details
ci/circleci: macos-build Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codecov/patch Coverage not affected when comparing 93bf2e7...34664ef
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@foxxx0 foxxx0 deleted the foxxx0:python3-compat branch Apr 14, 2018

# however it's only got support for ``typed`` arg in Python 3.3
'backports.functools_lru_cache',
],
'testing:python_version<"3.3"': [

This comment has been minimized.

@jaraco

jaraco Apr 14, 2018

Member

I've rejected this change before, because backports.unittest_mock is intended to be unconditionally included and it provides the conditionality (only including mock on the platforms where needed). In addition to making these dependency declarations more complicated in this project, it also implies that this is the rule that should be copied into every project that uses it. I reject that notion in that it violates DRY and adds to the cognitive burden of every project that uses it.

This comment has been minimized.

@webknjaz

webknjaz Apr 14, 2018

Member

Are you saying that we should revert it?

@@ -72,6 +71,13 @@
'codecov',
'pytest-cov',
],
':python_version<"3.3"': [

This comment has been minimized.

@jaraco

jaraco Apr 14, 2018

Member

This introduction of ranged comparators in extras will create a (possibly backward-incompatible) expectation on the project to require Setuptools 17.1 or later. That's probably acceptable at this stage, but it should probably be declared in the changelog, or even better in a pyproject.toml. Personally, I'd like for these backports modules to be simply declared/included unconditionally.

completely useless bloat and possible saves users some time when not having to download and install something unnecessary.

The time to download a tiny module like this is small and it's mostly machine time, whereas the added complexity of the package declaration and the additional cognitive burden of that complexity consumes real human work.

I think I'd prefer the simpler declaration that avoids the need for environment markers and comments, even if it doesn't precisely define where the package is absolutely needed.

This comment has been minimized.

@webknjaz

webknjaz Apr 14, 2018

Member

@jaraco We install setuptools>=28.2 in Travis: https://github.com/cherrypy/cheroot/blob/6821264/.travis.yml#L156

Also Python 2.7.14 is shipped with setuptools==28.8.0: https://github.com/python/cpython/tree/v2.7.14/Lib/ensurepip/_bundled
(next patch release of 2.7.x should have https://github.com/python/cpython/tree/2.7/Lib/ensurepip/_bundled >=39.0.1)

So it looks okay to already use these features. (of course for envs where OS package manager controls things it might differ)

@jaraco

This comment has been minimized.

Member

jaraco commented Apr 14, 2018

My inclination is to revert this change wholesale. I'd like to review the upstream ticket first to understand better what failure it caused.

@jaraco

This comment has been minimized.

Member

jaraco commented Apr 14, 2018

Indeed, the upstream ticket only states "cheroot requires.txt includes backports.functools_lru_cache which is a python2 package included in python3." That statement is subtly incorrect as the package is a universal Python 2 and Python 3 package. And as far as I can tell, no error or failure was reported by anyone.

@webknjaz If you're okay with it, I'd like to revert this change.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 14, 2018

@jaraco

Regarding Arch's issue, it's a bit different from our standard ecosystem we have to deal with, since when some other package manager kicks in it controls dependencies differently, which implies also wrapping all deps of the target package into their package format as well.
I can understand why they would want to simplify this on their side.

Also, it looks like there's two system installations of Python (2 and 3) living alongside and for each python distribution there's a separate pacman package for each of these installations, like it should be python2-backports.functools_lru_cache and python3-backports.functools_lru_cache. First is being installed into python2's site-packages and second should go to python3's one.
And it looks like they don't have this second package in their repos, but their cheroot module only refers to python2-backports.functools_lru_cache, which may lead to having dependency installed in different env, meaning broken installation.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 14, 2018

I'm not opposed to reverting it, but I also wanted to share my vision/understanding/guesses regarding topicstarter's motivation here.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 16, 2018

@jaraco so based on gitter discussion feel free to revert it.

jaraco added a commit that referenced this pull request Apr 16, 2018

jaraco added a commit that referenced this pull request Apr 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment