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

Internalize jaraco.classes code to replace dependency #1671

Merged
merged 2 commits into from Dec 17, 2017

Conversation

Projects
None yet
4 participants
@benjaoming
Contributor

benjaoming commented Dec 16, 2017

  • 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.

@benjaoming benjaoming force-pushed the benjaoming:bug/internalize-jaraco-classes branch from 68cb569 to eb59db7 Dec 16, 2017

@codecov

This comment has been minimized.

codecov bot commented Dec 16, 2017

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 benjaoming force-pushed the benjaoming:bug/internalize-jaraco-classes branch from eb59db7 to 1ab2155 Dec 16, 2017

Internalize jaraco.classes code to replace dependency - resolves issu…
…es when environment doesnt support namespaced packages - see jaraco/jaraco.classes#2

@benjaoming benjaoming force-pushed the benjaoming:bug/internalize-jaraco-classes branch from 1ab2155 to 7920822 Dec 17, 2017

@benjaoming

This comment has been minimized.

Contributor

benjaoming commented Dec 17, 2017

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

This comment has been minimized.

Contributor

benjaoming commented Dec 17, 2017

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

This comment has been minimized.

Contributor

benjaoming commented Dec 17, 2017

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

This comment has been minimized.

Contributor

benjaoming commented Dec 17, 2017

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

This comment has been minimized.

Member

jaraco commented Dec 17, 2017

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.

@codacy-bot

This comment has been minimized.

codacy-bot commented Dec 17, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 1
           

See the complete overview on Codacy

# environments (Pex, Celery, pip install -t)
#
# Code from http://stackoverflow.com/a/5191224
class _ClassPropertyDescriptor(object):

This comment has been minimized.

@codacy-bot
@jaraco

This comment has been minimized.

Member

jaraco commented Dec 17, 2017

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

This comment has been minimized.

Member

jaraco commented Dec 17, 2017

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.

@jaraco

jaraco approved these changes Dec 17, 2017

@jaraco jaraco merged commit 951489b into cherrypy:master Dec 17, 2017

1 of 4 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
lgtm analysis: Python Running analyses for revisions
Details
WIP ready for review
Details
####
# Inlined from jaraco.classes 1.4.3

This comment has been minimized.

@webknjaz

webknjaz Dec 17, 2017

Member

@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.

This comment has been minimized.

@jaraco

jaraco Dec 17, 2017

Member

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.

This comment has been minimized.

@webknjaz

webknjaz Dec 17, 2017

Member

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

@jaraco

This comment has been minimized.

Member

jaraco commented Dec 17, 2017

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

@benjaoming

This comment has been minimized.

Contributor

benjaoming commented Dec 17, 2017

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.

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