Internalize jaraco.classes code to replace dependency

[benjaoming]

• What kind of change does this PR introduce?

Resolves a bug when environment doesn't support namespaced packages

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

jaraco/jaraco.classes#2
jaraco/jaraco.classes#3

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

Will happen in an environment that doesn't understand namespaced packages. For instance just do:

$ mkdir test
$ cd test
$ pip install cherrypy -t .
$ ls
ls
cheroot                  CherryPy-13.0.0.dist-info       jaraco.classes-1.4.3-py3.6-nspkg.pth  portend-2.2.dist-info  pytz                   six.py   tempora-1.10.dist-info
cheroot-6.0.0.dist-info  jaraco                          more_itertools                        portend.py             pytz-2017.3.dist-info  six.pyc
cherrypy                 jaraco.classes-1.4.3.dist-info  more_itertools-4.0.1.dist-info        portend.pyc            six-1.11.0.dist-info   tempora
$ python
Python 2.7.12 (default, Nov 20 2017, 18:23:56) 
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import cherrypy
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "cherrypy/__init__.py", line 66, in <module>
    from ._cperror import (
  File "cherrypy/_cperror.py", line 128, in <module>
    from jaraco.classes.properties import classproperty
ImportError: No module named jaraco.classes.properties
>>> 

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

The above will not happen. CherryPy will be more portable.

• Other information:

AFAIK this is a problem with Pex and Celery.

[codecov]

Codecov Report

Merging #1671 into master will decrease coverage by <.01%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #1671      +/-   ##
==========================================
- Coverage    78.1%   78.09%   -0.01%     
==========================================
  Files         106      106              
  Lines       13790    13802      +12     
==========================================
+ Hits        10771    10779       +8     
- Misses       3019     3023       +4

[benjaoming]

In order to retain coverage, I decided only to copy over the essential bits. The classproperty decorator is used only once, and I'm assuming that it's used strictly as read-only.

There was a fair bit of linting to adhere to :) So I also had added in some docstrings that weren't in the original code.

[benjaoming]

Hoping this can be merged, because we had to revert to CherryPy 11. We can patch up code by doing something that amounts to this:

pip install -t cherrypy .
touch jaraco/__init__.py

I think since there's errors coming in from py2exe and Celery, you should really consider this carefully as it can take many hours of debugging for this error to be understood.

[jaraco]

As mentioned in the parent ticket, I don't want to accept this copy/paste until there is a ticket (or tickets) upstream (pex, celery, py2exe, pip) capturing the inability to support this use-case. This change also implies that CherryPy cannot depend on any package that contains namespace packages, so the comment linking to the offending ticket(s) should probably also be referenced in the setup.py to prevent other developers from adding dependencies on other namespace packages.

[benjaoming]

Please please please @jaraco skip to standard reviewing practice and take the fight for namespaced packages somewhere else. I haven't said anything bad about namespaced packages as a concept, and this dependency could be removed even if it didn't have the namespacing issues attached to it. You already have 3 people who searched out a repo with 0 stars complaining about it in 3 days, do you need more evidence?

Also, this is a security issue. You could inject any sort of malicious code into this dependency and release it to PyPi without anyone noticing. Not that you're a bad guy, but do you see the problem in this practice?

[benjaoming]

As mentioned in the parent ticket

Just to be clear here: This was mentioned after the PR was opened, and this PR didn't happen in spite of something mentioned.

[jaraco]

If we accept this change as-is, it could be readily optimized away, replaced by another developer (or my future self) with the implementation from jaraco.classes again. And there's nothing preventing CherryPy from accepting dependencies in other namespaced packages. What happens when the bit of functionality needed is supplied in another namespace package? Then CherryPy breaks again for an entirely different reason. It's possible, though I haven't looked, that there are already pending changes with additional namespace package dependencies.

But what this PR does is ask to make what seems like a minor change while implying a much bigger design decision - that CherryPy should not accept dependencies on namespace packages unless they provide a sufficiently high value (and where that threshold is subjective). I'm willing to accept the PR as long as the tradeoffs are acknowledged, the risks mitigated, and the underlying cause captured so the restriction can be lifted when appropriate.

If we only treat the symptom, then the underlying cause will continue to affect CherryPy and likely other projects. If on the other hand we capture the upstream failure and link that in comments proximate to the dependencies and to this inlined code, that protects against other packages bringing this same condition, documents the technical debt, presents a pattern for similar situations, and provides a mechanism to remove the duplication after a fix becomes available.

[jaraco]

In 52682a7 and 8b0e8a0, I illustrate two changes that I might consider to re-use constructs from packages in the jaraco namespace to simplify the CherryPy code.

Another library that I commonly use is backports.functools_lru_cache, which provides a backport of a popular standard library function lru_cache. Should CherryPy find that function helpful, it also would conflict (implicitly) with this proposed change.

Or looking at cherrypy.lib.lockfile, it appears to be copied from zc.lockfile, so it may make sense now that CherryPy allows for dependencies to depend on zc.lockfile rather than maintaining its own implementation.

[jaraco]

It's possible, though I haven't looked, that there are already pending changes with additional namespace package dependencies.

I just noticed #1654 also conflicts with this change.

My plan before I noticed that ticket was to go ahead and accept this PR, restoring support for environments that reject or fail on namespace packages, but to set a window (probably 6 months) for when CherryPy would once again allow dependencies on namespace packages.

And now I'm conflicted.

Still, I think it's best to acknowledge the regression and address it... and we can debate over what is the appropriate window for adding this constraint.

[webknjaz]

@jaraco I think, better place for such code vendoring would be some lib/vendor folder, which is a git submodule (or a subtree, but I don't have real experience with latter), which is easier to keep in sync. Also it would be easy to embed them into distribution.

[jaraco]

Well, I don't want to vendor the whole package. I also see this as a very temporary pattern until #1673 lands, so probably not worth too much extra effort. But if you'd like to take a stab at it, you're welcome to do so.

[webknjaz]

it's not that hard to implement, I'll probably find time for this

[jaraco]

This is released as v13.0.1. (well, releasing...)

[benjaoming]

Oh wow that's fantastic, and thanks so much for a fruitful discussions, too!

I will make sure we upgrade back to CherryPy 13.0.1 ASAP and report back here.