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

Adjusting cherrypy WSGI Server import path according to new release #3235

Merged
merged 8 commits into from Mar 9, 2018
Merged

Adjusting cherrypy WSGI Server import path according to new release #3235

merged 8 commits into from Mar 9, 2018

Conversation

heronrs
Copy link
Contributor

@heronrs heronrs commented Mar 8, 2018

Fixing cherrypy WSGI import, ref #3195

A couple of questions:

  • cherrypy_server_runner has a explicit # pragma: no cover comment. Should I remove it and add a specif test for it ?

  • CHANGES.txt doesn`t look like is beeing mainted lately. Should I add an entry to it ?

@heronrs
Copy link
Contributor Author

heronrs commented Mar 8, 2018

Fixing formatting and tox failed test.

Copy link
Member

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

We don't depend on cherrypy / cheroot at all, which means we cannot test against it, without adding some extra machinery.

Please do update CHANGES.txt: I'll check to see what PRs have been merged since the 1.9 branch forked off, and update it manually.

ssl_pem, ssl_pem)
from cherrypy.wsgiserver import get_ssl_adapter_class, ssl_builtin
get_ssl_adapter_class()
server.ssl_adapter = ssl_builtin.BuiltinSSLAdapter(ssl_pem, ssl_pem)
Copy link
Member

Choose a reason for hiding this comment

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

This part needs to use cheroot, too. E.g.:

try:
    from cheroot.server import get_ssl_adapter_class
except ImportError:
    from cherrypy.wsgiserver import get_ssl_adapter_class

try:
    from cheroot.ssl.builtin import BuiltinSSLAdapter
except ImportError:
    from cherrypy.wsgiserver.ssl_builtin import BuiltinSSLAdapter

@heronrs
Copy link
Contributor Author

heronrs commented Mar 8, 2018

Thanks @tseaver , I updated what you pointed out 👍

As for CHANGES.txt, I'ḿ not sure which would be the best section to add it. As its not really related to Pyramid itself I don't if it should be considered a Bug Fix. Maybe create a new Backward Compatibilities section ?
What do you think ?

@mmerickel
Copy link
Member

A note belongs in features and another in backward incompatibilities mentioning the change in import paths for the cherrypy server runner.

@heronrs
Copy link
Contributor Author

heronrs commented Mar 9, 2018

Guess I need update my fork and resolve the conflicts. Will do it shortly.

@tseaver
Copy link
Member

tseaver commented Mar 9, 2018

@mmerickel I don't think there is a backward-incompatibility here for Pyramid: we try the cheroot imports first, but fall back to the old cherrypy ones, which means that even users who have an old cherrypy installed will not have to adjust their code (although why they would be installing a new Pyramid and not updating to the lastest cherrypy is beyond me).

CHANGES.txt Outdated
@@ -35,6 +39,9 @@ Backward Incompatibilities
depending on it directly within your project.
See https://github.com/Pylons/pyramid/pull/3140

- On CherryPy 9+ all functionality from ``cherrypy.wsgiserver`` was moved to ``cheroot``.
See https://github.com/Pylons/pyramid/issues/3195
Copy link
Member

Choose a reason for hiding this comment

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

As I noted, I think we can drop this mention: users won't need to adjust their code, which means this is not a backward-incompatibility for Pyramid.

CHANGES.rst Outdated
@@ -21,6 +21,10 @@ Features
instead of ``pyramid.util.Request``.
See https://github.com/Pylons/pyramid/pull/3129

- Added a fallback ``try/except`` block on ``pyramid/scripts/pserve.cherrypy_server_runner``
due to module/import changes on CherryPy 9+
Copy link
Member

Choose a reason for hiding this comment

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

Can we phrase this more clearly as:

- In `cherrypy_server_runner`, prefer imports from the `cheroot` package over the legacy
  imports from `cherrypy.wsgiserver`.

@tseaver tseaver merged commit 396e533 into Pylons:master Mar 9, 2018
@mmerickel
Copy link
Member

@tseaver you're right of course, I missed that the old imports were preserved as a fallback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants