Py3 cmp #1605
Py3 cmp #1605
Conversation
|
Firstly thanks for this!
So taking Ubuntu as a good basemark, if we ignore the paid support option then 16.04 is the oldest supported OS, it's Python is still only 2.7: Although even 14.04 had 2.7... However it would be a shame to break 2.6 in the 0.10.8 release, and not really in the spirit of bug-fixes only, but feel free to remove that requirement in the master/0.11 branch. 2.7 needs to keep working until at least April 2023 though! I think 0.10.8 will be the final 0.10 branch release as there are some RDM changes to handle the E1.33/E1.37-7 PIDs which become too much of a new feature for a patch release, so that will help things too. If this PR becomes too awkward we can just push it into 0.11 anyway. |
.travis.yml
Outdated
| @@ -287,6 +287,7 @@ before_cache: | |||
| install: | |||
| # Match the version of protobuf being installed via apt | |||
| - pip install --user protobuf==3.1.0 | |||
| - pip install --user timeout-decorator | |||
There was a problem hiding this comment.
Is this a requirement we should add to the various build systems?
There was a problem hiding this comment.
That's used by the Event and OlaClient tests in ClientWrapperTest. Not used outside the tests. Honestly it could be dropped if it were preferable to let the tests hang if something breaks and let the build system kill them, but I hate tests that do that :)
Actually, I realized I forgot to add something to disable those tests in Windows in Py2. It relies on socketpair that wasn't supported natively in Win until 3.5, and IMHO it wouldn't be worth adding one of the various implementations at this point. Will add that.
There was a problem hiding this comment.
Checked and the python code is already not windows compatible so not changing anything in that test.
There was a problem hiding this comment.
If it's required, we should add it to the Debian, configure script etc. Can I suggest taking out the decorators for the moment until we've got this in, and then looking at adding them back in later to simplify the merge, given if the tests are passing it will work fine without the decorators.
There was a problem hiding this comment.
For posterity, the decorators are now commented out
There was a problem hiding this comment.
Firstly thanks for this!
Note that Nils had added comments that functools requires dropping 2.7. As far as I can tell the features in 3.x were backported to 2.7, so dropping 2.6 would let a lot of the boilerplate here be ignored.
So taking Ubuntu as a good basemark, if we ignore the paid support option then 16.04 is the oldest supported OS, it's Python is still only 2.7:
https://packages.ubuntu.com/search?keywords=python&searchon=names&exact=1&suite=xenial§ion=allAlthough even 14.04 had 2.7...
However it would be a shame to break 2.6 in the 0.10.8 release, and not really in the spirit of bug-fixes only, but feel free to remove that requirement in the master/0.11 branch. 2.7 needs to keep working until at least April 2023 though!
I think 0.10.8 will be the final 0.10 branch release as there are some RDM changes to handle the E1.33/E1.37-7 PIDs which become too much of a new feature for a patch release, so that will help things too. If this PR becomes too awkward we can just push it into 0.11 anyway.
I think leaving 2.6 supported until the next major release makes sense. Having put the boilerplate code in, it doesn't really cost anything to leave it. If someone needs to refactor again they could remove the extra comparisons and do some things like shift the protocol classes to use the attrs package rather than tons of boilerplate, but it's fine now.
I don't believe 2.7 is going anywhere :)
IMHO this change ought to be in 10.8 for those who want to use the OLA client API with a py3 project. It would require them to do a manual install, but it would unblock them (and those who try to use the mac brew build that forced py3).
I have more changes that are mostly finished to make the whole thing build and install with python3. There are quite a few changes but they're mostly mechanical and not in the client sdk (Nils got most of them there). I don't know that there's a reason not to put the additional changes into 10.8 either, but as there's one non-trivial change (short but I will need help validating it), need for a new builder, and as there's no test-suite I don't really know if it runs, I felt like I would put it into a separate PR.
Haven't worked with debuild before but will try to get it added in the next day or two. |
|
@peternewman I got debuild working to the point toward the end that it noticed that my protobuf install wasn't from a deb and quit (I have a hand-built protobuf since I couldn't find a package for 3.1.0). There doesn't seem to be a way to bypass that check. Do you know the location of a package I could use instead or another way to get around that? |
Sounds very sensible
Indeed I suspect you're right.
Yeah we always seem to be playing catchup now with new releases. My thinking is release 0.10.8 probably as soon as this is in, possibly after one or two other minor bugs. Then I suspect release 0.11.0 almost immediately as it's had a queue of shiny new features appearing since 2016 (gulp), but no-one seems to offer to test it thoroughly when I ask, branch that off for 0.11.1 if there are any bugfixes, then master becomes what will be 0.12.0 and people can start culling or breaking things if they like, such as C++11 and Python 2, rather than throw away a lot of known good compatibility for no good reason. Then people have a choice of a bug fix on what they're using (0.10.8), latest stable (0.11.0) or bleeding edge (master).
Thanks @brucelowekamp sounds sensible, we can make a judgement on if it goes into 0.10.8 or gets pushed back. |
That's great news. So the official Debian release gets round this by cherry-picking our hack that's only in our 0.10 branch to make Protobuf work with a newer version, I'm still on an older release which hence has an old protobuf and doesn't have these issues. I suspect your best bet might be to test the debuild stuff against @yoe 's branch which is where the Debian package, and hence our debuild usage comes from: Then just cherry-pick your debuild changes back from a fork of that into this PR or something? He can probably advise either way or possibly even give you some useful CI or something, he's certainly our guru when it comes to Debian! |
|
To clarify here, do you think this PR is ready to go aside from debuild? I think while tests are nice, if it looks like it will be a long haul or you don't get too far with what it and you've tested what you've committed (which I'm guessing you have), we could get it merged and leave debuild as a nice to have and leave the issue open to track that as well as general Python code python3 compatibility. |
I've used it with my code in both 2.7 and 3.7. Now my code only sends and doesn't receive or do any sort of management, but as far as I can tell the changes are consistent. As I said in the other thread I'm not sure the new tests will pass in 2.6, but I believe that would be it. Having said that, I went to look again and found that @nils-van-zuijlen original change altered the semantics of the comparison in PidStore.py. I'm not sure why (I tried to avoid that and made sure the hashes and comparisons were consistent, but I missed this one). Will ask if he remembers why. |
|
I added checks for the other fields in 3d9d8d8 from @peternewman 's question in 3d9d8d8#r257505257 |
There was a problem hiding this comment.
I added checks for the other fields in 3d9d8d8 from @peternewman 's question in 3d9d8d8#r257505257
Thanks. I agree they should be there, but I realized I missed PidStore.py entirely, including testing. Hope to get some tests in later this week.
@peternewman @nomis52 In doing a PidStore test (pulling tests from PidStoreTest.cpp), I discovered there is no override support in the python PidStore (loading common/rdm/testdata/pids fails). Doesn't appear hard to implement, but a comment in PidStore.h says higher version is a newer pid, but the PidStoreTest.cpp appears to assume the lower version will be used. It looks relatively easy to add version to the store and pids, but wanted to check that the "lower version wins" behavior is correct before I implement it. |
Oops, good spot! Where's the comment and test about versions, the only stuff I can find is here: I think the version comment is prior to overrides existing and I don't believe we actually check versions anywhere. The idea though is the override file gets loaded regardless of how versions might compare. One thing, if we add this to the Python code, which as you say we should, it should probably be disabled by default in the RDM tests/model collector because I think by default it should probably use the "official" data. |
@nils-van-zuijlen what's the link to my question, it just seems to point to the commit? |
|
It's #1538 (comment) Sorry, I didn't check if the previous link was working as intended. |
|
Ah, I found a description of what the version number was, but not the overrides.proto comment (or the code corresponding to it). Should be pretty easy to implement. Hope to get it in over the weekend and rebase the code on top of current 0.10. Bruce |
See https://portingguide.readthedocs.io/en/latest/comparisons.html#rich-comparisons for an explaination of why it was broken.
functools.total_ordering is only available from python 3.2 fix flake8 formatting
When you call RegisterUniverse with action set to UNREGISTER, you don't want to be forced to provide a data_callback
As stated in the doc (https://docs.python.org/2.7/reference/datamodel.html?highlight=cmp#object.__cmp__) __cmp__ is only called if rich comparison is not defined and we define it as it is the only way to support python 3
Every time I've pushed without running flake8, I've regretted it... |
The other challenge is that this is only (I think the most important) part of the python3 work. I have another set of changes that got most of the tools/rdm working (or at least passing sanity checks), and there was another patch submitted a couple weeks ago for part of the API. Given how long it's taken getting these patches through, I thought it best to wait to put in a PR for more. The new protobuf code should help as well, though. But ultimately there's more work, and it will require testing from some folks who use that part of the code. (I only use the sending API) Bruce |
|
Another option (certainly not my favourite) if we are not able to get this done in time for the Ubuntu 20.10 release (which would require that this is all resolved by July-August if I'm not mistaken) is to upload a version of ola with all python support disabled at compile time. That would definitely fit the bill as far as Debian is concerned, but is obviously not a very good long-term strategy, and disabling the python-based tools would have some downside. Still, it would get something in for Ubuntu 20.10, which is better than what's going to happen to 20.04. |
Ah great thanks @yoe , so we won't have dropped out of Debian, that's good. Your fix for GCC9 #1583 means it compiles with GCC at least (if not Clang currently).
Amazing, congratulations to both of you! 🎊 👰♀️ 🤵♂️
Damn, I hoped they might have an exception for packages which had previously been in Ubuntu and it's a particular shame it's an LTS release. @VoltVisionFrenchy had previously made an OLA PPA, perhaps if we ask really nicely he'll be able to update that (or possibly automate it so it just pulls from Git and builds in future.
Yeah hopefully the other stuff is simpler when we've got the core changes in and should need less checking for example as it won't interact with other bits so much or be as complicated hopefully.
Yeah sorry I've been rather slow with some of these PR reviews @brucelowekamp . Various bits of other life going on. Hopefully given the current situation things will quieten down a bit! 😷 I'm sure we can cover off the testing too.
Technically we could just release the ola deb couldn't we (without ola-python or ola-rdm-tests)?
Yeah definitely. I guess we missed a trick there and should have done just ola (without Python) for 20.04. |
Yes, we could (and we'd have to, otherwise those packages would be empty, and that makes no sense)
Yup, but it's too late now, unfortunately. |
There was a problem hiding this comment.
Just a few minor bits @brucelowekamp , mostly around that change of behaviour you flagged up, and then some minor tidying of the Makefiles, but this is basically ready to merge now.
Continued thanks for all your work on getting this over the line!
| @@ -65,7 +69,7 @@ class UnpackException(Error): | |||
|
|
|||
|
|
|||
| class MissingPLASAPIDs(Error): | |||
There was a problem hiding this comment.
I wonder if we should really have renamed this (or kept the old one for backwards compatibility and subclass or re-raise or something)?
There was a problem hiding this comment.
The name hasn't changed since it was introduced, AFAICT. The comment change was one of the changes brought in from master.
There was a problem hiding this comment.
Ah okay, one for after this mammoth PR is in then!
|
So, I think there is a deletion, an updated of a comment and a minor fix or deletion listed above. Then we're good to go! 🎉 Can you confirm, have you re-run a few basic real world tests since all the changes? Fixes for later (in a future PR, by one of us):
|
I put (part of) my lightshow back up when quarantine began and checked today that my code still works with the python lib (in both 2.7 and 3.7). Groups didn't look like it would be hard to fix the indenting on given an example pid and deciding what it should actually produce :) |
Yeah agreed, I think we'd just need to have a ponder exactly what we wanted to do, and to generate some test examples. |
There was a problem hiding this comment.
Massive thanks to @brucelowekamp for getting this across the line and also for @nils-van-zuijlen for your initial work on it and @yoe for some advice along the way and anyone else I've missed.
Now to fix all the other bits of Python 3 compatibility 😆
|
I gave advice on python stuff? Oh dear. I do have a reputation to consider ;-P |
This picks up on @nils-van-zuijlen work on fixing cmp for python3 compatibility in PR #1538 My client code works in python3 after these changes, so might be enough for #1506 though would like to get it built with python3 as well.
Note that Nils had added comments that functools requires dropping 2.7. As far as I can tell the features in 3.x were backported to 2.7, so dropping 2.6 would let a lot of the boilerplate here be ignored. Also, sorting None elements in with UID and MACAddress appear to work (judging by the tests) with the current implementation when functools.total_ordering is used. I updated comments to reflect that.
Changes in here on top of #1538
minor random changes: