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

Python 3 support for ShinySDR #145

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
fa08731
Update setup.py for Python 3
quentinmit Jul 19, 2020
5d8c982
Update testutil.py for Python 3
quentinmit Jul 19, 2020
f16517b
More Python 3 support
quentinmit Jul 19, 2020
c7a0606
Work around Twisted Python 3.8 compatibility issue
quentinmit Jul 21, 2020
1267477
Call putChild with byte strings for Python 3 compatibility
quentinmit Jul 21, 2020
7b882d4
Fix make_websocket_url to use strings on Python 3
quentinmit Jul 21, 2020
9d77173
Filenames and MIME types are strings in Python 3
quentinmit Jul 21, 2020
97e2ef0
Return JSON as utf-8
quentinmit Jul 21, 2020
c9dd568
Parse WebSocket path correctly on Python 3
quentinmit Jul 21, 2020
480d0cf
Python blocks must have a reference held on the Python side as long a…
quentinmit Jul 21, 2020
55f2ece
Monkey-patch txWS to fix it on Python 3
quentinmit Jul 21, 2020
48bcccc
Fix BlockResource for Python 3
quentinmit Jul 21, 2020
9089b68
sort can fail on Python 3 if objects are not comparable
quentinmit Jul 21, 2020
1529d7f
Iterators no longer have a next() method in Python 3
quentinmit Jul 21, 2020
291496f
Filenames are not bytes in Py3
quentinmit Jul 21, 2020
b5b609c
Handle application/json headers with and without charset specified
quentinmit Jul 22, 2020
3a7529b
Address lint errors
quentinmit Jul 22, 2020
11b6d2f
More bytes/str changes that fix tests on Python 3.8
quentinmit Jul 22, 2020
9835805
Fix lint
quentinmit Jul 22, 2020
d4d6684
Address review comments
quentinmit Sep 17, 2020
0f0992b
Test removing charset from application/json headers
quentinmit Sep 17, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@

import os.path
import subprocess
import urllib
try:
from urllib.request import urlretrieve
except ImportError:
from urllib import urlretrieve
Comment on lines +24 to +27
Copy link
Owner

Choose a reason for hiding this comment

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

Add a comment indicating this is for version compatibility so we can strip it out later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


from setuptools import find_packages, setup, Command
from setuptools.command.build_py import build_py
Expand All @@ -47,7 +50,7 @@ def run(self):
print('skipping downloading {}, already exists'.format(destination_path))
else:
print('downloading {} to {}'.format(source_url, destination_path))
urllib.urlretrieve(source_url, destination_path)
urlretrieve(source_url, destination_path)


class InitGitSubModules(Command):
Expand Down
10 changes: 5 additions & 5 deletions shinysdr/i/blocks.py
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,8 @@ def __do_connect(self):
# It would make slightly more sense to use unsigned chars, but blocks.float_to_uchar does not support vlen.
self.__fft_converter = blocks.float_to_char(vlen=self.__freq_resolution, scale=1.0)

fft_sink = self.__fft_cell.create_sink_internal(numpy.dtype((numpy.int8, output_length)))
scope_sink = self.__scope_cell.create_sink_internal(numpy.dtype(('c8', self.__time_length)))
self.__fft_sink = self.__fft_cell.create_sink_internal(numpy.dtype((numpy.int8, output_length)))
self.__scope_sink = self.__scope_cell.create_sink_internal(numpy.dtype(('c8', self.__time_length)))
Comment on lines +305 to +306
Copy link
Owner

Choose a reason for hiding this comment

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

Why are these being made attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Letting Python garbage collect them while they're still in the flowgraph causes crashes now.

scope_chunker = blocks.stream_to_vector_decimator(
item_size=gr.sizeof_gr_complex,
sample_rate=sample_rate,
Expand All @@ -324,15 +324,15 @@ def __do_connect(self):
logarithmizer)
if self.__after_fft is not None:
self.connect(logarithmizer, self.__after_fft)
self.connect(self.__after_fft, self.__fft_converter, fft_sink)
self.connect(self.__after_fft, self.__fft_converter, self.__fft_sink)
self.connect((self.__after_fft, 1), blocks.null_sink(gr.sizeof_float * self.__freq_resolution))
else:
self.connect(logarithmizer, self.__fft_converter, fft_sink)
self.connect(logarithmizer, self.__fft_converter, self.__fft_sink)
if self.__enable_scope:
self.connect(
self.__gate,
scope_chunker,
scope_sink)
self.__scope_sink)
finally:
self.__context.unlock()

Expand Down
10 changes: 5 additions & 5 deletions shinysdr/i/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
# Note that gnuradio-dependent modules are loaded lazily, to avoid the startup time if all we're going to do is give a usage message
from shinysdr.i.db import DatabaseModel, database_from_csv, databases_from_directory
from shinysdr.i.network.base import UNIQUE_PUBLIC_CAP
from shinysdr.i.pycompat import bytes_or_ascii, repr_no_string_tag
from shinysdr.i.pycompat import repr_no_string_tag
from shinysdr.i.roots import CapTable, generate_cap


Expand Down Expand Up @@ -127,8 +127,8 @@ def serve_web(self,
self._not_finished()
# TODO: See if we're reinventing bits of Twisted service stuff here

http_base_url = _coerce_and_validate_base_url(http_base_url, 'http_base_url', ('http', 'https'))
ws_base_url = _coerce_and_validate_base_url(ws_base_url, 'ws_base_url', ('ws', 'wss'), allow_path=True)
http_base_url = _coerce_and_validate_base_url(http_base_url, 'http_base_url', (b'http', b'https'))
ws_base_url = _coerce_and_validate_base_url(ws_base_url, 'ws_base_url', (b'ws', b'wss'), allow_path=True)

if root_cap is not None:
root_cap = six.text_type(root_cap)
Expand Down Expand Up @@ -198,11 +198,11 @@ def _coerce_and_validate_base_url(url_value, label, allowed_schemes, allow_path=
if url_value is not None:
url_value = str(url_value)

scheme, _netloc, path_bytes, _params, _query_bytes, _fragment = urlparse(bytes_or_ascii(url_value))
scheme, _netloc, path_bytes, _params, _query_bytes, _fragment = urlparse(six.ensure_binary(url_value))

# Ensure that the protocol is compatible.
if scheme.lower() not in allowed_schemes:
raise ConfigException('config.serve_web: {} must be a {} URL but was {}'.format(label, ' or '.join(repr_no_string_tag(s + ':') for s in allowed_schemes), repr_no_string_tag(url_value)))
raise ConfigException('config.serve_web: {} must be a {} URL but was {}'.format(label, ' or '.join(repr_no_string_tag(six.ensure_str(s) + ':') for s in allowed_schemes), repr_no_string_tag(url_value)))

# Ensure that there are no path components. There are two reasons for this:
# 1. The client makes use of host-relative URLs.
Expand Down
20 changes: 10 additions & 10 deletions shinysdr/i/db.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,10 @@ class DatabasesResource(resource.Resource):

def __init__(self, databases):
resource.Resource.__init__(self)
self.putChild('', ElementRenderingResource(_DbsIndexListElement(self)))
self.putChild(b'', ElementRenderingResource(_DbsIndexListElement(self)))
self.names = []
for (name, database) in six.iteritems(databases):
self.putChild(name, DatabaseResource(database))
self.putChild(name.encode(), DatabaseResource(database))
self.names.append(name)
self.names.sort() # TODO reconsider case/locale

Expand All @@ -170,9 +170,9 @@ def __init__(self, database):
resource.Resource.__init__(self)

def instantiate(rkey):
self.putChild(str(rkey), _RecordResource(database, database.records[rkey]))
self.putChild(str(rkey).encode(), _RecordResource(database, database.records[rkey]))

self.putChild('', _DbIndexResource(database, instantiate))
self.putChild(b'', _DbIndexResource(database, instantiate))
for rkey in database.records:
instantiate(rkey)

Expand All @@ -186,11 +186,11 @@ def __init__(self, db, instantiate):
self.__instantiate = instantiate

def render_GET(self, request):
request.setHeader(b'Content-Type', b'application/json')
request.setHeader(b'Content-Type', b'application/json; charset=utf-8')
Copy link
Owner

Choose a reason for hiding this comment

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

application/json does not have a charset parameter; this change and its other instances below should be reverted. https://tools.ietf.org/html/rfc7159

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get what the RFC says, but I believe this was in fact necessary to make it work because something else started setting/requiring it. I'll try to reproduce the error so I can post it here.

(I certainly didn't spontaneously decide to add this all over the place!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't reproduce whatever error I saw (and this was months ago so I've long since forgotten). I reverted these changes for now as a separate commit so I can easily re-add them if/when I figure out why they were necessary in the first place.

return json.dumps({
u'records': self.__database.records,
u'writable': self.__database.writable
})
}).encode('utf-8')

def render_POST(self, request):
desc = json.load(request.content)
Expand All @@ -207,7 +207,7 @@ def render_POST(self, request):
dbdict[rkey] = record
self.__database.dirty() # TODO: There is no test that this is done.
self.__instantiate(rkey)
url = request.prePathURL() + str(rkey)
url = request.prePathURL() + six.ensure_binary(str(rkey))
request.setResponseCode(http.CREATED)
request.setHeader(b'Content-Type', b'text/plain')
request.setHeader(b'Location', url)
Expand All @@ -223,11 +223,11 @@ def __init__(self, database, record):
self.__record = record

def render_GET(self, request):
request.setHeader(b'Content-Type', b'application/json')
return json.dumps(self.__record)
request.setHeader(b'Content-Type', b'application/json; charset=utf-8')
return json.dumps(self.__record).encode('utf-8')

def render_POST(self, request):
assert request.getHeader(b'Content-Type') == b'application/json'
assert request.getHeader(b'Content-Type') in (b'application/json', b'application/json; charset=utf-8')
if not self.__database.writable:
request.setResponseCode(http.FORBIDDEN)
request.setHeader(b'Content-Type', b'text/plain')
Expand Down
6 changes: 4 additions & 2 deletions shinysdr/i/depgraph.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@

from __future__ import absolute_import, division, print_function, unicode_literals

import six

from twisted.internet import defer
from twisted.internet.task import deferLater
from twisted.logger import Logger
Expand Down Expand Up @@ -115,7 +117,7 @@ def get_fitting(self, other_ff, requester_ff=None):

def _schedule_change(self, reason):
"""Internal for _ActiveFittingHolder -- asynchronously trigger a reevaluation of the graph"""
self.__log.debug('scheduled change ({reason})', reason=unicode(reason))
self.__log.debug('scheduled change ({reason})', reason=six.ensure_text(reason))
self.__scheduled_change.start()

def _mark_for_rebuild(self, fitting_factory):
Expand Down Expand Up @@ -153,7 +155,7 @@ def add_factory(ff, reason):

self.__log.debug('CHANGE: ...completed analysis')

newly_inactive_ffs = (set(self.__active_holders.iterkeys())
newly_inactive_ffs = (set(self.__active_holders.keys())
.difference(active_ffs)
.union(self.__do_not_reuse))
needs_configuration = newly_active_ffs or newly_inactive_ffs
Expand Down
4 changes: 2 additions & 2 deletions shinysdr/i/ephemeris.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ def render_GET(self, request):
y = math.cos(sun.az) * math.cos(sun.alt)
z = -math.sin(sun.alt)

request.setHeader(b'Content-Type', b'application/json')
return json.dumps([x, y, z])
request.setHeader(b'Content-Type', b'application/json; charset=utf-8')
return json.dumps([x, y, z]).encode('utf-8')


__all__.append('EphemerisResource')
4 changes: 2 additions & 2 deletions shinysdr/i/network/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,8 @@ def make_websocket_url(self, request, path):
else:
return endpoint_string_to_url(
self.__ws_endpoint_string,
hostname=request.getRequestHostname(),
scheme=b'ws',
hostname=six.ensure_str(request.getRequestHostname()),
scheme='ws',
path=path)


Expand Down
20 changes: 10 additions & 10 deletions shinysdr/i/network/export_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@

from shinysdr.i.json import serialize
from shinysdr.i.network.base import prepath_escaped, template_filepath
from shinysdr.i.pycompat import defaultstr
from shinysdr.values import IWritableCollection


Expand Down Expand Up @@ -71,19 +70,20 @@ def __init__(self, block, wcommon, deleteSelf):
if cell.type().is_reference():
self._blockCells[key] = cell
else:
self.putChild(key, ValueCellResource(cell, self.__wcommon))
self.putChild(key.encode(), ValueCellResource(cell, self.__wcommon))
quentinmit marked this conversation as resolved.
Show resolved Hide resolved
self.__element = _BlockHtmlElement(wcommon)

def getChild(self, path, request):
name = path.decode()
quentinmit marked this conversation as resolved.
Show resolved Hide resolved
if self._dynamic:
curstate = self._block.state()
if path in curstate:
cell = curstate[path]
if name in curstate:
cell = curstate[name]
if cell.type().is_reference():
return self.__getBlockChild(path, cell.get())
return self.__getBlockChild(name, cell.get())
else:
if path in self._blockCells:
return self.__getBlockChild(path, self._blockCells[path].get())
if name in self._blockCells:
return self.__getBlockChild(name, self._blockCells[name].get())
# old-style-class super call
return Resource.getChild(self, path, request)

Expand All @@ -102,7 +102,7 @@ def deleter():
return BlockResource(block, self.__wcommon, deleter)

def render_GET(self, request):
accept = request.getHeader('Accept')
accept = request.getHeader(b'Accept')
if accept is not None and b'application/json' in accept: # TODO: Implement or obtain correct Accept interpretation
request.setHeader(b'Content-Type', b'application/json')
return serialize(self.__describe_block()).encode('utf-8')
Expand All @@ -118,7 +118,7 @@ def render_POST(self, request):
assert request.getHeader(b'Content-Type') == b'application/json'
reqjson = json.load(request.content)
key = block.create_child(reqjson) # note may fail
url = request.prePathURL() + defaultstr('/receivers/') + urllib.parse.quote(key, safe='')
url = six.ensure_text(request.prePathURL()) + '/receivers/' + urllib.parse.quote(key, safe='')
request.setResponseCode(201) # Created
request.setHeader(b'Location', url)
# TODO consider a more useful response
Expand Down Expand Up @@ -204,7 +204,7 @@ def render_GET(self, request):
1: 'r',
2: 2
})
process.pipes[0].write(self.__block.dot_graph())
process.pipes[0].write(self.__block.dot_graph().encode('utf-8'))
process.pipes[0].loseConnection()
return NOT_DONE_YET

Expand Down
6 changes: 3 additions & 3 deletions shinysdr/i/network/export_ws.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ def __send_references_and_update_refcount(self, objs, is_single):

# Decrement refcounts of old (or existing) references.
refs = self.__previous_references
refs.sort() # ensure determinism
refs.sort(key=id) # ensure determinism
for obj in refs:
if obj not in self.__ssi._registered_objs:
raise Exception("Shouldn't happen: previous value not registered", obj)
Expand Down Expand Up @@ -194,7 +194,7 @@ def dec_refcount_and_maybe_notify(self):

# capture refs to decrement
refs = self.__previous_references
refs.sort() # ensure determinism
refs.sort(key=id) # ensure determinism

# drop previous value
self.__set_previous_references({})
Expand Down Expand Up @@ -376,7 +376,7 @@ def __dispatch_url(self):
self.__log.info('Stream connection to {url}', url=self.transport.location)
_scheme, _netloc, path_bytes, _params, query_bytes, _fragment = urlparse(bytes_or_ascii(self.transport.location))
# py2/3: unquote returns str in either version but we want Unicode
path = [six.text_type(urllib.parse.unquote(x)) for x in path_bytes.split(b'/')]
path = [six.ensure_text(urllib.parse.unquote(six.ensure_str(x))) for x in path_bytes.split(b'/')]

# parse fixed elements of path
# TODO: generally better error reporting, maybe use twisted's Resource dispatch???
Expand Down
14 changes: 7 additions & 7 deletions shinysdr/i/network/session_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,23 +38,23 @@ def __init__(self, session, wcommon, read_only_dbs, writable_db):
SlashedResource.__init__(self)

# UI entry point
self.putChild('', ElementRenderingResource(_RadioIndexHtmlElement(wcommon)))
self.putChild(b'', ElementRenderingResource(_RadioIndexHtmlElement(wcommon)))

# Exported radio control objects
self.putChild(CAP_OBJECT_PATH_ELEMENT, BlockResource(session, wcommon, _not_deletable))
self.putChild(CAP_OBJECT_PATH_ELEMENT.encode(), BlockResource(session, wcommon, _not_deletable))
Copy link
Owner

Choose a reason for hiding this comment

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

How about using b'' for the constant value since it's always used in this context? If that doesn't work, I'd rather see .encode('utf-8') here because it's the right thing for all URL components, rather than depending on the default encoding under Python 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I actually buy that utf-8 is correct; arguably it should be utf-8 + urlencode, but since it's a constant that's pure ASCII it's kind of moot. Done.


# Frequency DB
self.putChild('dbs', shinysdr.i.db.DatabasesResource(read_only_dbs))
self.putChild('wdb', shinysdr.i.db.DatabaseResource(writable_db))
self.putChild(b'dbs', shinysdr.i.db.DatabasesResource(read_only_dbs))
self.putChild(b'wdb', shinysdr.i.db.DatabaseResource(writable_db))

# Debug graph
self.putChild('flow-graph', FlowgraphVizResource(wcommon.reactor, session.flowgraph_for_debug()))
self.putChild(b'flow-graph', FlowgraphVizResource(wcommon.reactor, session.flowgraph_for_debug()))

# Ephemeris
self.putChild('ephemeris', EphemerisResource())
self.putChild(b'ephemeris', EphemerisResource())

# Standard audio-file-over-HTTP audio stream (the ShinySDR web client uses WebSockets instead, but both have the same path modulo protocol)
self.putChild(AUDIO_STREAM_PATH_ELEMENT, AudioStreamResource(session))
self.putChild(AUDIO_STREAM_PATH_ELEMENT.encode(), AudioStreamResource(session))
quentinmit marked this conversation as resolved.
Show resolved Hide resolved


class _RadioIndexHtmlElement(EntryPointIndexElement):
Expand Down
4 changes: 2 additions & 2 deletions shinysdr/i/network/test_audio_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ class TestAudioStreamResource(unittest.TestCase):

def setUp(self):
tree = Resource()
tree.putChild('mono', AudioStreamResource(_FakeSession(1)))
tree.putChild('stereo', AudioStreamResource(_FakeSession(2)))
tree.putChild(b'mono', AudioStreamResource(_FakeSession(1)))
tree.putChild(b'stereo', AudioStreamResource(_FakeSession(2)))
self.port = the_reactor.listenTCP(0, SiteWithDefaultHeaders(tree), interface="127.0.0.1") # pylint: disable=no-member

def tearDown(self):
Expand Down
Loading