Skip to content
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

Only require backports.functools_lru_cache for python < 3.3 #221

Merged
merged 5 commits into from Sep 10, 2019

Conversation

dcermak
Copy link
Contributor

@dcermak dcermak commented Sep 2, 2019

❓ What kind of change does this PR introduce?

  • 🐞 bug fix
  • 🐣 feature
  • πŸ“‹ docs update
  • πŸ“‹ tests/coverage improvement
  • πŸ“‹ refactoring
  • πŸ’₯ other

πŸ“‹ What is the related issue number (starting with #)

None

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

cheroot requires the backports.functools_lru_cache module even for python versions that ship functools.lru_cache

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

cheroot only requires backports.functools_lru_cache for python < 3.3

πŸ“‹ Other information:

πŸ“‹ Checklist:

  • I think the code is well written
  • I wrote good commit messages
  • I have squashed related commits together after the changes have been approved
  • [-] Unit tests for the changes exist
  • [-] Integration tests for the changes exist (if applicable)
  • [-] I used the same coding conventions as the rest of the project
  • [-] The new code doesn't generate linter offenses
  • [-] Documentation reflects the changes
  • [-] The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences

This change is Reviewable

@codecov
Copy link

codecov bot commented Sep 2, 2019

Codecov Report

Merging #221 into master will decrease coverage by 2.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
- Coverage   74.13%   71.97%   -2.17%     
==========================================
  Files          23       23              
  Lines        3507     3507              
==========================================
- Hits         2600     2524      -76     
- Misses        907      983      +76

@codecov

This comment has been minimized.

@webknjaz webknjaz requested a review from jaraco September 2, 2019 21:18
@webknjaz webknjaz added the wontfix This is not smth we're going to fix label Sep 2, 2019
@smithfarm
Copy link

@webknjaz Was the "wontfix" label added intentionally?

Copy link

@smithfarm smithfarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the following bug - on a system with a recent Python 3 installed:

$ python3
>>> import pkg_resources; pkg_resources.require('cheroot')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.6/site-packages/pkg_resources/__init__.py", line 895, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/usr/lib/python3.6/site-packages/pkg_resources/__init__.py", line 781, in resolve
    raise DistributionNotFound(req, requirers)
pkg_resources.DistributionNotFound: The 'backports.functools_lru_cache' distribution was not found and is required by cheroot

(In other words, without this fix, cheroot is totally broken on recent Python 3.)

@webknjaz
Copy link
Member

webknjaz commented Sep 3, 2019

@smithfarm it's not broken. You didn't install the dependency.

@webknjaz
Copy link
Member

webknjaz commented Sep 3, 2019

I'm leaving this up to @jaraco to decide whether to merge this. It's a duplicate of what he blocked two times already.

@smithfarm
Copy link

smithfarm commented Sep 3, 2019

@smithfarm it's not broken. You didn't install the dependency.

It's not a problem for me, personally. The problem arises in openSUSE Leap 15.1 which does not package "backports.functools_lru_cache" for Python 3 for what seems like a good reason - the code it contains is included in Python 3 core.

@webknjaz
Copy link
Member

webknjaz commented Sep 3, 2019

@smithfarm it's a fallback in runtime. But pkg_resources seems to disagree. That dep is noop under Python 3.

@webknjaz
Copy link
Member

webknjaz commented Sep 3, 2019

Anyway, I'm almost sure that you didn't use Pip to install Cheroot which means that you must blame your OS packager.

@webknjaz
Copy link
Member

webknjaz commented Sep 3, 2019

It's a duplicate of #181 and #203

@smithfarm
Copy link

Anyway, I'm almost sure that you didn't use Pip to install Cheroot which means that you must blame your OS packager.

Right, I am trying to keep this stuff running in the OS.

If this fix helps the OS maintainers and does not hurt folks using pip, I don't understand why it's being blocked?

@webknjaz
Copy link
Member

webknjaz commented Sep 3, 2019

ea8acd0

@webknjaz
Copy link
Member

webknjaz commented Sep 3, 2019

Feel free to read all the discussions in linked tickets. But the bottom line is that we only support Pip and separate ourselves from anything that is related to OS cases because it's something that somebody else who is interested in this should maintain.

@dcermak
Copy link
Contributor Author

dcermak commented Sep 3, 2019

But the bottom line is that we only support Pip and separate ourselves from anything that is related to OS cases because it's something that somebody else who is interested in this should maintain.

I fully understand that.

What I don't understand is why you insist on keeping backports.functools_lru_cache as a direct dependency for Python versions that ship this module in their standard library? Yes, the installation is not broken with pip (because it just downloads the package but never uses it) and yes we can work around this dependency when creating a rpm package, but that's not the point of this pull request. The point is that you do not need backports.functools_lru_cache as lru_cache is part of functools since Python 3.2. So why require it then?

@jaraco
Copy link
Member

jaraco commented Sep 10, 2019

That reversion commit mentions #85.

CHANGES.rst Outdated
v6.6.0
======

- Revisit :pr:`85` under :pr:`221`. Now ``backports.unittest_mock``
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Revisit :pr:`85` under :pr:`221`. Now ``backports.unittest_mock``
- Revisit :pr:`85` under :pr:`221`. Now ``backports.functools_lru_cache``

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 2f6199a

CHANGES.rst Outdated
v6.6.0
======

- Revisit :pr:`85` under :pr:`221`. Now ``backports.unittest_mock``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 2f6199a

@jaraco
Copy link
Member

jaraco commented Sep 10, 2019

So why require it then?

Reading up on #85, which goes into detail, the tl;dr is that it's simpler for this project (and all other projects) to unconditionally declare these compatibility libraries and in fact, the backports guidance specifically advises projects to unconditionally use the backport (see "What if the feature is present"). Cheroot doesn't do that, which is what gives the impression that it's an optional dependency. It's the declared dependencies that are authoritative, so if downstream packagers aren't honoring the declared dependencies, that's a bug in their system.

So let's say we accept this proposal. Does that make it easier for downstream packagers? It arguably makes it more difficult, as now packagers have different dependency trees depending on the target Python version.

Still, since cheroot is conditionally importing this dependency, I guess it's fine to accept to make it easier on the packagers.

@jaraco jaraco merged commit 8b5cffe into cherrypy:master Sep 10, 2019
@dcermak
Copy link
Contributor Author

dcermak commented Sep 10, 2019

Does that make it easier for downstream packagers?

Thank you for merging this, it actually makes our job easier in opensuse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This is not smth we're going to fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants