python3 rdm compat work from sdbbs 1615

[brucelowekamp]

This takes the patches from @sdbbs required for the python rdm support to be py3 compatible. Just a minor formatting change, remove the comparison fix, and fixing the rdm_get example for py3.

This has been manually tested using the python/examples code and the dummy plugin with olad. Looking at code coverage, it still didn't exercise some of the RDM Frame code that I found concerning (discussed in #1615) but I don't actually know what paths would trigger that code, and don't know that I have the right devices to exercise it anyway.

Ideally this would have tests included. I looked a bit at mocking, and I suspect it's possible by supplying a socket and sending packets manual, but pretty ugly and brittle. My preferred way to automate it would be to stand up olad with the dummy plugin and use the python examples/python tests against the running olad. AFAIK there aren't any automated tests that do that at present (except the ola client tests that use the olad library internally), and I decided against doing that without discussion.

[brucelowekamp]

@sdbbs hadn't heard from you for awhile and finally got time to play with it and couldn't generate any additional test cases. This is just your code changes with the formatting change and the example chang. More than happy to abandon this is you want to rebase your PR on 0.10 with the other two changes.

[brucelowekamp]

@peternewman mac build is broken at present? I don't think the failure had anything to do with this PR

[sdbbs]

Hi @brucelowekamp ,

hadn't heard from you for awhile

Sorry about that - I've been assigned to slightly different work these days (weeks, rather), so haven't had much time to respond - I only noticed this notification because it had my pseudonym there :)

More than happy to abandon this is you want to rebase your PR on 0.10 with the other two changes.

I would have loved to do that, unfortunately what I'm assigned to at work now, does not allow me the time to take a look into it deeper - I might have some more time in a month or so, but it's just a wild guess. So I'm more than happy this python3 compatibility is looked into regardless.

Looking at code coverage, it still didn't exercise some of the RDM Frame code that I found concerning (discussed in #1615)

Oh, that thread has advanced - I'd love to take a look at it in more detail, but as I mentioned, I don't really have the time for it these days ...

Just wanted to mention something - I think since I posted the #1615, I found that the last hunk (in function Pack, I guess), also needs a if sys.version >= '3.2': - I am attaching the patch where I recorded that here.

Note the attached patch has an additional string in the warning in the Unpack function, that is probably just some leftover from my debugging, so you can ignore it. Also, the patch was done in respect to OLA commit f69e4a6 :

ola0.10.7-1.py3_06_f69e4a61.patch.txt

So, yeah - sorry about not being able to contribute more at this time, but my work at this time does not allow me the time for it; hope things clear up a bit soon, so I can go back to contributing a bit more ... And thanks again for looking a bit more into this!

[peternewman]

A few comments

[peternewman]

fixing the rdm_get example for py3.

We should probably start with compilation checks for all the python example code. I assume there is a way to easily check it's valid at the CLI?

It doesn't look like we currently have Pack/Unpack tests for Python. See equivalents in common/rdm/MessageSerializerTest.cpp and common/rdm/RDMCommandSerializerTest.cpp for example.

Ideally this would have tests included. I looked a bit at mocking, and I suspect it's possible by supplying a socket and sending packets manual, but pretty ugly and brittle.

Can we not go half way and generate the packet it would send to the socket and validate that the generated packet matches the expected manually generated data?

My preferred way to automate it would be to stand up olad with the dummy plugin and use the python examples/python tests against the running olad. AFAIK there aren't any automated tests that do that at present (except the ola client tests that use the olad library internally), and I decided against doing that without discussion.

I think this would be great, although perhaps one for another PR. If we also ran the RDM responder tests (or perhaps run them as a first thing, as they're easy to automate), it would also mean we'd never end up with our responders breaking the tests and vice versa, which is a great place to be. I think I've half looked in the past but never quite got round to it.

@peternewman mac build is broken at present? I don't think the failure had anything to do with this PR

I think it might just be a flaky test?

[brucelowekamp]

Hi @brucelowekamp ,

hadn't heard from you for awhile

Sorry about that - I've been assigned to slightly different work these days (weeks, rather), so haven't had much time to respond - I only noticed this notification because it had my pseudonym there :)

No worries, I don't use ola routinely either, so have spent a few months before getting back to it. But would also like it to work well with py3.

Just wanted to mention something - I think since I posted the #1615, I found that the last hunk (in function Pack, I guess), also needs a if sys.version >= '3.2': - I am attaching the patch where I recorded that here.

Thanks, grabbed that as well.

fixing the rdm_get example for py3.

We should probably start with compilation checks for all the python example code. I assume there is a way to easily check it's valid at the CLI?

That part was easy. I actually had it written from my local branch that has (almost all) the python3 changes done for the rest of the tree. Those changes aren't in (I think we discussed doing that after 0.10.8) so I commented out the parts of the tree that aren't updated and just run the compilation test on the sdk (and the test in data that does compile).

It doesn't look like we currently have Pack/Unpack tests for Python. See equivalents in common/rdm/MessageSerializerTest.cpp and common/rdm/RDMCommandSerializerTest.cpp for example.

agree, there aren't tests for Pack/Unpack. I could possibly look at it at some point but didn't have time now.

Ideally this would have tests included. I looked a bit at mocking, and I suspect it's possible by supplying a socket and sending packets manual, but pretty ugly and brittle.

Can we not go half way and generate the packet it would send to the socket and validate that the generated packet matches the expected manually generated data?

Yeah, that can be done. In fact in ClientWrapperTest I used a socketpair to confirm data is going out, so the framework is even there, it's a matter of working out the test cases.

In the meantime I've pushed the comment fix, additional fix, and the compile test.

[brucelowekamp]

@peternewman I used the socketpair to add a regression test for rdm_get that I think effectively tests the functionality. Assuming that approach looks reasonable, I can pretty easily abstract it a bit and add some more test cases in there. Obviously it's possible to do unit tests for pack/unpack, though I think this tests a bit more of the codepath.

The mac build is currently failing on the python compile test. I'd like to see what that error is but I don't find test-suite.log in the build output. What am I missing?

On linux, at least, distcheck passes on my machine using PYTHON=python3. The parts of the tree that aren't python compatible aren't exercised by distcheck. Any objection if I try to switch just one of the builds to py3 just to catch regressions?

Other than the mac error, just simple misses on license and flake8 (that I broke fixing the vpath) that I can fix.

[peternewman]

A few comments

[peternewman]

@peternewman I used the socketpair to add a regression test for rdm_get that I think effectively tests the functionality.

Looks good.

Assuming that approach looks reasonable, I can pretty easily abstract it a bit and add some more test cases in there.

Personally I'd be tempted to try and do a lot of the other tests at a higher level, that hex data is basically garbage (I think it's actually two levels of protobuf wrapped). So diagnosing would be a pain.

Obviously it's possible to do unit tests for pack/unpack, though I think this tests a bit more of the codepath.

I think one test like this is useful for the codepath, but testing the other pack/unpack separately like we do in C++ is easier to see what's going on and add new tests for new things.

The mac build is currently failing on the python compile test. I'd like to see what that error is but I don't find test-suite.log in the build output. What am I missing?

It should be one of these (unless it has another path, in which case just add another line):
https://github.com/OpenLightingProject/ola/blob/0.10/.travis.yml#L358-L359

Any objection if I try to switch just one of the builds to py3 just to catch regressions?

I'd be tempted to add another Travis check, then we can clearly see if it's a Python 2 or three issue. Adding one to Linux would be pretty easy and quick I'd think. Or I guess on Linux the Clang and G++ tests could do one Python 2 and one 3, on the assumption there shouldn't be interactions between them.

[peternewman]

The mac build is currently failing on the python compile test. I'd like to see what that error is but I don't find test-suite.log in the build output. What am I missing?

It should be one of these (unless it has another path, in which case just add another line):
https://github.com/OpenLightingProject/ola/blob/0.10/.travis.yml#L358-L359

Edit: try commenting out the config.log logging, I think all the logging is too long and so we can't see the test suite log.

[brucelowekamp]

@peternewman agree, I'll try to add maybe one or two more regression tests, but then a few at a higher level. The regression tests are useful for their purpose, but as you say, not for debugging.

For some reason one of the mac builds had plugins/usbpro/WidgetDetectorThreadTester fail.

I tried to switch the clang build to python 3.8, but for some reason the obvious travis.yml change was ignored a build time. I'll have to look at that more later. (and I could only see what happened a run time, as a clean run doesn't save the files or enough output.

[peternewman]

@peternewman agree, I'll try to add maybe one or two more regression tests, but then a few at a higher level. The regression tests are useful for their purpose, but as you say, not for debugging.

Great, sounds good.

For some reason one of the mac builds had plugins/usbpro/WidgetDetectorThreadTester fail.

Yeah that and HealthCheckedConnectionTester have always been a bit flaky on Travis Mac. I think it's a race condition of some sort, but as they only show on the VM it's a bit hard to track down.

I tried to switch the clang build to python 3.8, but for some reason the obvious travis.yml change was ignored a build time. I'll have to look at that more later. (and I could only see what happened a run time, as a clean run doesn't save the files or enough output.

If I was to guess, I think you'd need to possibly do brew link to make python->python3, but I've no idea if that upsets other stuff. Or somehow otherwise tell it to use the python3 binary.

What did config.log say about what it was using/had found?

[peternewman]

Hi @brucelowekamp ,

@yoe would like us to release 0.10.8 by Nov 14th at the latest, to give time to get it into Debian before their deadline. How's this going, anything I can do to help? It would be great to get this in by then if possible, but no worries if not.

[brucelowekamp]

Hi @brucelowekamp ,

@yoe would like us to release 0.10.8 by Nov 14th at the latest, to give time to get it into Debian before their deadline. How's this going, anything I can do to help? It would be great to get this in by then if possible, but no worries if not.

I'll see if I can get some time in the next week to get a couple tests in here.

[brucelowekamp]

@peternewman it looks like the failures I see are not related to any changes in here, so I consider this ready. Please let me know if you would like any other changes.

Just to state clearly, this changes the linux clang build to use python3. The direct "python" setting in travis apparently only impacts code run directly by travis, but xenial is using pyenv to manage python installations so it turned out to be a simple matter of setting the pyenv version and then all the pip commands worked fine. I left in a PYTHON=python3 b/c it seemed better, though I think it was unnecessary.

I dislike that it requires pinning pyenv to a specific release (3.7.1). There is a feature request to support wildcards (3.7*) but the pyenv developers seem unmoved. Though travis.yml already pins PATH to that for codespell, so it isn't the only thing that needs to be changed.

[peternewman]

Just a few minor comments

[peternewman]

@peternewman it looks like the failures I see are not related to any changes in here, so I consider this ready.

Agreed.

Please let me know if you would like any other changes.

Review with a few minor bits on the latest changes. I clearly need to fix all that Codespell stuff!

Just to state clearly, this changes the linux clang build to use python3. The direct "python" setting in travis apparently only impacts code run directly by travis, but xenial is using pyenv to manage python installations so it turned out to be a simple matter of setting the pyenv version and then all the pip commands worked fine. I left in a PYTHON=python3 b/c it seemed better, though I think it was unnecessary.

I dislike that it requires pinning pyenv to a specific release (3.7.1). There is a feature request to support wildcards (3.7*) but the pyenv developers seem unmoved. Though travis.yml already pins PATH to that for codespell, so it isn't the only thing that needs to be changed.

Do you think pyenv is a better/more pythonic way to do it than I'm doing with the path variable in Codespell? I think it would probably make sense to be consistent between the two as they both have to run as Python 3 of some version.

[brucelowekamp]

I dislike that it requires pinning pyenv to a specific release (3.7.1). There is a feature request to support wildcards (3.7*) but the pyenv developers seem unmoved. Though travis.yml already pins PATH to that for codespell, so it isn't the only thing that needs to be changed.

Do you think pyenv is a better/more pythonic way to do it than I'm doing with the path variable in Codespell? I think it would probably make sense to be consistent between the two as they both have to run as Python 3 of some version.

I'm not sure if it's more pythonic, but given that travis/xenial seems to have adopted pyenv to manage their python versions it's probably the better way to go. I glanced at it and I think it's a trivial change, so pushing it now.

[brucelowekamp]

I'm not sure if it's more pythonic, but given that travis/xenial seems to have adopted pyenv to manage their python versions it's probably the better way to go. I glanced at it and I think it's a trivial change, so pushing it now.

Looks like it worked. Looking again, I'm a bit unclear if the travis variable PYTHON=python3 exports all the way down to the build commands. I think it might, in which case that second command I added to set it based on travis env was unnecessary. But I don't think it hurts anything, either.

[peternewman]

Sorry a few minor nits if you've got time?

[peternewman]

Just a minor improvement of the tests and this comment:
#1655 (comment)

[brucelowekamp]

@peternewman anything else pending? A bit hard to follow but I think all the comments are addressed

[peternewman]

@peternewman anything else pending? A bit hard to follow but I think all the comments are addressed

Yeah I think so too. I'm going to do a final sanity check of the code and comments later and merge. Sorry got distracted earlier so didn't get through it all.

[peternewman]

Thanks again for this @brucelowekamp and @sddbs and everyone else who's worked on it!