-
Notifications
You must be signed in to change notification settings - Fork 115
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
base: master
Are you sure you want to change the base?
Conversation
My computer(s) are a bit in disarray right now, so I may be a while giving this a proper code review. So, thanks for doing this, and I'll give you some quick cheap observations. Unfortunately, the Travis CI integration seems to have not posted results here (I may have reauthorized it now, but I'm not sure) but here they are. There are two simple lints:
And there are several test failures in the Python 2.7 environment, all looking like they have to do with details of web serving (probably str/bytes/unicode confusion). It's okay to go from failing to failing on Python 3, but the tests must remain passing under Python 2 (until we drop Python 2 support, which will come no sooner than adding GNU Radio 3.8 support). |
I don't have a good Python 2 GR environment anymore. I guess I could see how many tests run without being able to import gnuradio. I'll take a stab at trying to fix the bytes confusion. I didn't mention it in the PR description but this also runs on GNU Radio 3.8. |
OK, when I have my GR development environment back, I'll see if I can contribute any tweaks to get it working on both — or perhaps merge whatever subset of these commits are straightforwardly compatible. |
…s they are part of a flowgraph
I just pushed a fix to the only test that I could find broken on 2.7 (
There are definitely still test failures left on Python 3.8, though. I don't know how disarrayed your computers are right now, but if you have a computer that can run Docker you can pull the |
I can't leave it alone, so I ran trial3 inside docker and got it down to:
trial2 is still passing as well, and running it on Py3.8 + GR3.8 seems to cursorily work. As part of the most recent commit I think I fixed the OsmoSDR source (we don't use it, though, so I can't be sure). The only remaining failing tests on Python 3.8 are:
AFAICT the last two (and maybe also the first?) appear to be testing the order of items in a dictionary, which was not defined behavior and changed in Python 3.7. |
@kpreid Hey, this has been sitting for a couple weeks... any chance to look at this yet? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review only; I haven't run these myself yet.
try: | ||
from urllib.request import urlretrieve | ||
except ImportError: | ||
from urllib import urlretrieve |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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))) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
shinysdr/i/db.py
Outdated
@@ -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') |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!)
There was a problem hiding this comment.
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.
@@ -50,13 +51,43 @@ | |||
from shinysdr.values import SubscriptionContext | |||
|
|||
|
|||
# txWS is unfortunately dead upstream, and has buggy Python 3 support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:(
I have a dead branch that tries to move to Autobahn, but I was having trouble getting it to even show me any error stacks from my callbacks not working.
shinysdr/i/network/webapp.py
Outdated
for resource_def in getPlugins(_IClientResourceDef, shinysdr.plugins): | ||
# Add the plugin's resource to static serving | ||
plugin_resources.putChild(resource_def.key, resource_def.resource) | ||
plugin_resources.putChild(resource_def.key.encode(), resource_def.resource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one definitely needs to be 'utf-8' as the plugin could put arbitrary characters in its names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean it also needs to be URL-encoded? Otherwise done.
It's been three weeks, have you had a chance to get your GR development environment working yet? |
Sorry, I've been very busy dealing with various further life complications, and also a bit of obsession with another project. This is still on my todo list. |
I’ve been checking back here from time to time. I’d like to add shinysdr back to DragonOS (project of mine). It’s already got python3/gnuradio 3.8 included. Just curious if there’s any movement planned on this or if I should just try to piece it together based on this PR and test it that way? Thanks |
Tested it real quick and found that it works rather well. Thanks for doing this PR. I went ahead and also installed gr-air-modes and gr-dad. I had pretty much everything else that was needed, except gr-radioteletype which looks to be for an older gnuradio. One thing I noticed, I can see mode S info in the terminal but am I supposed to be seeing the messages decoded somewhere in the shinysdr GUI? I have the map up and do not see anything there. |
I think I figured out that for whatever reason I’m not getting positions on Adsb, but I do see mode S messages. Another note, I thought I’d get lucky and this would work, mcapdeville/gr-radioteletype@69b6b18 But after installing the 3.8 version shiny SDR would not start till I uninstalled gr-radioteletype. Thought maybe I’d have all the features working, so close. |
Love this project. I hope this branch gets completed and this project upgraded to python 3! |
我现在正在开始的迁移工作,从gnuradio3.7+python2.7迁移到gnuradio3.9+python3.8 正在迁移的过程中,目前通过python setup.py build 和 sudo python setup.py install 返回的错误提示修改了两处位置。
try: 并且升级了 Twisted six txWS PyEphem pyserial等相关依赖,但是目前还是存在问题。 |
Hi, are there any plans to merge this PR? |
Be even better to see if support gr 3.9+ |
I can't promise everything works but all the basic functionality seems to be there and working in my tests. I kept each logical change as a separate commit so you can easily review them. I believe all of these changes are backwards-compatible to Python 2.7. Several of the tests are failing but it seems like that was true before these commits as well. I can start trying to knock off the tests in this PR if you'd like, or I can leave that for another PR.