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

Add stand-alone command for running server #87

Merged
merged 24 commits into from May 18, 2018

Conversation

Projects
None yet
3 participants
@scottp-dpaw
Contributor

scottp-dpaw commented Apr 19, 2018

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    New feature. This pull request adds the ability to run a new Cheroot instance from the command line, similar to the standalone tools provided by Gunicorn and uWSGI.

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

  • What is the current behavior? (You can also link to an open issue here)
    Applications wishing to support Cheroot have to provide a loader stub to interface with the generic WSGI application.

  • What is the new behavior (if this is a feature change)?
    Applications can start a Cheroot instance for a WSGI application from the command line without any additional Python code.


This change is Reviewable

@scottp-dpaw scottp-dpaw force-pushed the scottp-dpaw:executable branch from 43fb9e7 to 1a93c1d Apr 19, 2018

@codecov

This comment has been minimized.

codecov bot commented Apr 19, 2018

Codecov Report

Merging #87 into master will increase coverage by 1.39%.
The diff coverage is 41.09%.

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   66.37%   67.76%   +1.39%     
==========================================
  Files          17       19       +2     
  Lines        2974     3909     +935     
==========================================
+ Hits         1974     2649     +675     
- Misses       1000     1260     +260
@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 19, 2018

Hello, run pre-commit run --all-files locally.

@scottp-dpaw scottp-dpaw force-pushed the scottp-dpaw:executable branch from 1a93c1d to d78a2ca Apr 19, 2018

@webknjaz

This comment has been minimized.

@scottp-dpaw

This comment has been minimized.

Contributor

scottp-dpaw commented Apr 19, 2018

Thanks for the heads up. I saw contributor notes did mention "ensure you set up pre-commit utility correctly", but I didn't find any detail beyond that. Didn't occur to me there was a tool literally called "pre-commit".

def wsgi_bind_addr(string):
if ':' in string:

This comment has been minimized.

@webknjaz

webknjaz Apr 19, 2018

Member

Mind adding ability to bypassing UNIX sockets via argument?

Here's how curl, httpie and requests do it:

curl --unix-socket /tmp/cp.sock http://localhost/
http http+unix://%2Fvar%2Frun%2Fdocker.sock/info
r = requests.get('http+unix://%2Fvar%2Frun%2Fdocker.sock/info')

Also, software tends to prefix abstract sockets with @:
https://unix.stackexchange.com/questions/206386/what-does-the-symbol-denote-in-the-beginning-of-a-unix-domain-socket-path-in-l?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa

So I'd like it to accept unix socket starting with an @ (or extra argument if you think it's better)

This comment has been minimized.

@scottp-dpaw

scottp-dpaw Apr 20, 2018

Contributor

I had a long think about this... the two supported inputs for bind_addr are either the tuple (host_or_ip, port), or a string as a path for a UNIX socket. As the string equivalent for the former ("host_or_ip:port") is pretty easy to match with a regex, it's possible to get away with treating any non-matches as a UNIX socket path. Main difference would be that passing just an IP address would no longer give you port 8000 for free, but chances are if you want to change the IP address you'll be wanting to change the port too.

Alternatively we could put in a case to deal with just a bare IP address so it doesn't make a socket, or require UNIX socket paths begin with '@' and throw an exception as the overall fallback. Also if the plan is to make a new struct type for bind_addr, any string deserializing logic should go in there.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 19, 2018

Also maybe it makes sense to make use of click?

@webknjaz webknjaz requested review from jaraco and cherrypy/contributors Apr 19, 2018

@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 19, 2018

Didn't occur to me there was a tool literally called "pre-commit".

Thanks, I've improved that doc a bit now.

setup.py Outdated
@@ -103,6 +103,9 @@
'Topic :: Internet :: WWW/HTTP :: WSGI :: Server',
],
entry_points={
'console_scripts': [
'cheroot = cheroot.__main__:main'

This comment has been minimized.

@webknjaz

webknjaz Apr 19, 2018

Member

@jaraco what do you think about this script naming? Do we want it to have trailing d to be consistent with CherryPy? Does it need a daemon mode?

port = int(port)
else:
addr, port = string, 8000
return addr, port

This comment has been minimized.

@webknjaz

webknjaz Apr 19, 2018

Member

@jaraco I've been thinking about having a public struct for the interface in our codebase. What do you think?

It could be:

  1. reused across the project
  2. support several different concrete socket implementations (and maybe hold some list of features they have):
  • TCP sockets
  • UNIX file sockets (most unix-like OSs)
  • UNIX abstract namespace sockets (GNU/Linux starting v2.2.0 of Linux kernel)
@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 19, 2018

@scottp-dpaw thanks for your PR 🎉

ping me once linter errors are fixed and you react to comments and I'll get back to review ;)

@scottp-dpaw scottp-dpaw force-pushed the scottp-dpaw:executable branch 2 times, most recently from 2729dc2 to 3c9d1b7 Apr 20, 2018

@scottp-dpaw scottp-dpaw force-pushed the scottp-dpaw:executable branch from 3c9d1b7 to 5606633 Apr 20, 2018

@scottp-dpaw

This comment has been minimized.

Contributor

scottp-dpaw commented Apr 20, 2018

@webknjaz: should be ready for review again. Is it okay if I ignore the remaining CodeClimate test? 25 lines is a difficult limit for argument parsing, it counts each arg as 5 lines!

I considered third party arg parsing modules; in the end I went with argparse because it didn't add a dependency and worked alright for printing/variable naming/type validation. (Also I once worked on a project which used docopt and almost went mad from trying to debug it)

@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 20, 2018

// this got folded, so copy-pasting from diff comments:

@jaraco I've been thinking about having a public struct for the interface in our codebase. What do you think?

It could be:

  1. reused across the project
  2. support several different concrete socket implementations (and maybe hold some list of features they have):
    • TCP sockets
    • UNIX file sockets (most unix-like OSs)
    • UNIX abstract namespace sockets (GNU/Linux starting v2.2.0 of Linux kernel)
setup.py Outdated
@@ -103,6 +103,9 @@
'Topic :: Internet :: WWW/HTTP :: WSGI :: Server',
],
entry_points={
'console_scripts': [
'cheroot = cheroot.__main__:main'
]

This comment has been minimized.

@webknjaz

webknjaz Apr 20, 2018

Member

Could you add a trailing comma here?

setup.py Outdated
@@ -103,6 +103,9 @@
'Topic :: Internet :: WWW/HTTP :: WSGI :: Server',
],
entry_points={
'console_scripts': [
'cheroot = cheroot.__main__:main'

This comment has been minimized.

@webknjaz

webknjaz Apr 20, 2018

Member

and comma here

server_args['wsgi_app'] = wsgi_app_path(raw_args._wsgi_app)
server = Server(**server_args)
try:

This comment has been minimized.

@@ -0,0 +1,134 @@
"""Command line tool for starting a Cheroot instance.

This comment has been minimized.

@webknjaz

webknjaz Apr 20, 2018

Member

Let's clarify that it's WSGI.

Also, I think it might be good to also support starting plain HTTP thing accepting cheroot.server.Gateway instance or class descendant.

# Start a server on 0.0.0.0:9000 with 8 threads
# for the WSGI app myapp/wsgi.py:main_app()
cheroot myapp.wsgi:main_app --bind 0.0.0.0:9000 --threads 8

This comment has been minimized.

@webknjaz

webknjaz Apr 20, 2018

Member

I'd like to see file socket and abstract socket examples

def wsgi_app_path(string):
"""Convert WSGI app string to a tuple."""
if ':' in string:

This comment has been minimized.

@webknjaz

webknjaz Apr 20, 2018

Member

Refactor:

mod_path, app_path = string.split(':', 1)
if ':' in app_path:
  raise ValueError('Application object name is invalid: it should not contain colon (:) character.')
elif not app_path:
    app_path = 'application'
@jaraco

Just a couple nitpick comments on the bind addr, but otherwise, this looks good to me.

parser.add_argument(
'--bind', metavar='ADDRESS',
dest='_bind_addr', type=str,
default='127.0.0.1:8000',

This comment has been minimized.

@jaraco

jaraco May 17, 2018

Member

Default should be IPv6-ready, i.e. '::1:8000'

This comment has been minimized.

@jaraco

jaraco May 17, 2018

Member

Also, it seems odd to supply two parameters as one (and thus have to parse it).

@jaraco

This comment has been minimized.

Member

jaraco commented May 17, 2018

I'll work on the codeclimate failures - bow to our robot overlords.

@jaraco jaraco force-pushed the scottp-dpaw:executable branch from 4dc9bf1 to fcdcb75 May 17, 2018

@webknjaz

This comment has been minimized.

Member

webknjaz commented May 18, 2018

This pull request introduces 2 alerts when merging fc4eb35 into ad4cd1c - view on lgtm.com

new alerts:

  • 1 for Missing call to __init__ during object initialization
  • 1 for Wrong number of arguments in a call

Comment posted by lgtm.com

jaraco added some commits May 18, 2018

@webknjaz

This comment has been minimized.

Member

webknjaz commented May 18, 2018

This pull request introduces 1 alert when merging d1c5841 into ad4cd1c - view on lgtm.com

new alerts:

  • 1 for Wrong number of arguments in a call

Comment posted by lgtm.com

@jaraco

jaraco approved these changes May 18, 2018

@jaraco jaraco merged commit 54e748d into cherrypy:master May 18, 2018

6 of 8 checks passed

ci/circleci: macos-build CircleCI is running your tests
Details
code-review/reviewable 3 files, 31 discussions left (jaraco, webknjaz)
Details
WIP ready for review
Details
ci/circleci: linux-build Your tests passed on CircleCI!
Details
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details

jaraco added a commit that referenced this pull request May 18, 2018

@@ -0,0 +1,6 @@
"""Stub for accessing the Cheroot CLI tool."""

This comment has been minimized.

@webknjaz

webknjaz May 18, 2018

Member

It's not a stub, but an entry point for running CLI

mod_path, _, app_path = full_path.partition(':')
app = getattr(import_module(mod_path), app_path or 'application')
with contextlib.suppress(TypeError):

This comment has been minimized.

@webknjaz

webknjaz May 18, 2018

Member

@jaraco this thing appeared in 3.4, did I miss adding a compat lib for 2.7?

This comment has been minimized.

@webknjaz

webknjaz May 18, 2018

Member

Also, what exactly can cause TypeError here?

This comment has been minimized.

@webknjaz

webknjaz May 18, 2018

Member

From what I see, it can simply be removed.

)
self.wsgi_app = wsgi_app
def server_args(self, parsed_args):

This comment has been minimized.

@webknjaz

webknjaz May 18, 2018

Member

This looks like a @staticmethod to me

dest='bind_addr',
type=parse_wsgi_bind_addr,
default='[::1]:8000',
help='Network interface to listen on (default: [::1]:8000)',

This comment has been minimized.

@webknjaz

webknjaz May 18, 2018

Member

Description should tell how to pass UNIX file and abstract sockets.

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