Fix the Python RPC tests on Big Endian architectures

[peternewman]

• ~~New placeholder for NEWS~~
• ~~Fix a typo in NEWS~~
• ~~Fix the OS X builds on Travis~~
• ~~Add a missing semicolon to the build for if we don't have CLOCK_MONOTONIC~~

The above has all been moved into #1703

This is now mostly:

• Fix the Python RPC tests on Big Endian architectures

[peternewman]

~~We probably want to cherry-pick this commit into here too:~~ ~~https://github.com/OpenLightingProject/ola/commit/5286a6925f5f9dc1a045d13632d08cef7f818536~~

~~Given the issues in https://github.com/Homebrew/homebrew-core/pull/65415~~

Now also in #1703

[peternewman]

@yoe 91cb697912664d58317d001f1e7998ac556c9de6 should fix those test on Big Endian architecture, would you mind testing it somehow as I've only dummied it up by reversing stuff on an LE machine. @brucelowekamp care to take a look at it for any Python howlers I've introduced?

[brucelowekamp]

@yoe 91cb697 should fix those test on Big Endian architecture, would you mind testing it somehow as I've only dummied it up by reversing stuff on an LE machine. @brucelowekamp care to take a look at it for any Python howlers I've introduced?

I don't know of any python concerns, but I do have to ask if this isn't a protocol error? It's sent out on the socket, so if you were to use the API across two endian machines, it wouldn't work? Looks like both the Python and C++ do the same thing, but still....

Bruce

[peternewman]

I don't know of any python concerns, but I do have to ask if this isn't a protocol error? It's sent out on the socket, so if you were to use the API across two endian machines, it wouldn't work? Looks like both the Python and C++ do the same thing, but still....

I'd be tempted to agree with you @brucelowekamp and I did consider fixing it. However due to the limitations mentioned here: http://docs.openlighting.org/ola/doc/latest/rpc_system.html#rpc_OLA_RPC

There won't/shouldn't be any inter-machine comms and hence any multi-endian issues. Whereas if we do fix it, then if someone runs an unfixed client against a fixed server, it won't work. Although given this is only on BE machines, I guess it's a fairly low risk. It will definitely be a breaking change though, so I'm not sure I'm personally convinced of the benefit apart from hypothetically. I guess we should at least flag the lines of code with a TODO and possibly consider making the change for 0.11.0 or similar.

Unfortunately @yoe found it didn't entirely work:

(10:52:23) wouter: PeterNewman4: your patches don't seem to resolve the issue
(10:52:31) wouter: that is, it kind of works, but things seem to hang
(10:52:58) wouter: and I also get a line saying "WARNING:root:Protocol mismatch: 3 != 1" when I run python/ola/RDMTest.py

Having had a better look, that code is in our codebase: https://github.com/OpenLightingProject/ola/blob/d1010976c47de23dc6031749179f8d92596b7e8d/python/ola/rpc/StreamRpcChannel.py#L246-L250

Given it's the RDM ones, presumably it's of course going to be because they have responses too and I haven't mangled those responses on the way back out, so it can't read them on BE architectures/thinks they're corrupt!

[peternewman]

Okay @yoe this definitely cracks it. Here's a broken build on s390x (thanks to Travis): https://travis-ci.org/github/OpenLightingProject/ola/jobs/748167069#L4381-L4475

And here's one with that fix applied on top (still on the same architecture): https://travis-ci.org/github/peternewman/ola/builds/748170563

[yoe]

Awesome. Didn't know Travis supports s390x too these days. Very cool!

Am I correct in stating that I just need to cherry pick ead457c, eb1f26a, and c402fcb? If not, what else would I need?

Peter Newman [email protected] schreef op 7 december 2020 22:29:08 GMT+02:00:

Okay @yoe this definitely cracks it. Here's a broken build on s390x (thanks to Travis): https://travis-ci.org/github/OpenLightingProject/ola/jobs/748167069#L4381-L4475

And here's one with that fix applied on top (still on the same architecture): https://travis-ci.org/github/peternewman/ola/builds/748170563

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/OpenLightingProject/ola/pull/1687#issuecomment-740161776

-- Verstuurd vanaf mijn Android apparaat met K-9 Mail. Excuseer mijn beknoptheid.

[peternewman]

Awesome. Didn't know Travis supports s390x too these days. Very cool!

Yeah I was rather pleased too, and how easily it worked, although there are some other potential Travis issues which may mean we have to move away from them soon, but hopefully not. I guess I should still do the paperwork to fix the default route on Hurd and other issues.

Am I correct in stating that I just need to cherry pick ead457c,

That's a Travis fix so I don't think so.

You do need this one though: 91cb697912664d58317d001f1e7998ac556c9de6

eb1f26a, and

This fixes the existing test.

c402fcb?

This just adds some more tests which are a bit rough around the edges, so you probably don't need it.

If not, what else would I need?

You may want this too in case it impacts any Debian setups: f907b4c992e85bfb30559b5855c41194d2c52050

You probably also want the stuff in #1690 which fixes Python 3.9 behaviour of the DMX functions.

I was contemplating releasing a 0.10.9 given there are a few little issues and everyone has had to apply lots of random patches, would that be easier or harder for you to integrate?

[yoe]

Awesome. Didn't know Travis supports s390x too these days. Very cool!

Yeah I was rather pleased too, and how easily it worked, although there are some other potential Travis issues which may mean we have to move away from them soon, but hopefully not. I guess I should still do the paperwork to fix the default route on Hurd and other issues.

Am I correct in stating that I just need to cherry pick ead457c,

That's a Travis fix so I don't think so.

You do need this one though: 91cb697

I'd already pulled that one :)

eb1f26a, and

This fixes the existing test.

okay, so adding that

c402fcb?

This just adds some more tests which are a bit rough around the edges, so you probably don't need it.

If not, what else would I need?

You may want this too in case it impacts any Debian setups: f907b4c

don't think it does, but can't hurt, so yeah, pulled.

You probably also want the stuff in #1690 which fixes Python 3.9 behaviour of the DMX functions.

There are no plans to get Python 3.9 into Debian 11 that I can see, so I'm going to skip those for the time being.

I was contemplating releasing a 0.10.9 given there are a few little issues and everyone has had to apply lots of random patches, would that be easier or harder for you to integrate?

Probably about the same. Can't hurt to clarify that we have 0.10.9 though.

I'll just upload 0.10.8-with-fixes for now so I get at least something into Debian testing, we can look at 0.10.9 later if there's still time.

[brucelowekamp]

On Thu, Dec 10, 2020 at 4:52 AM Wouter Verhelst [email protected] wrote:

You probably also want the stuff in #1690 https://github.com/OpenLightingProject/ola/pull/1690 which fixes Python 3.9 behaviour of the DMX functions.

There are no plans to get Python 3.9 into Debian 11 that I can see, so I'm going to skip those for the time being.

1690 is needed for any python 3, not just 3.9. I just didn't see it/don't use that code path.

Bruce

[peternewman]

Sounds good @yoe

1690 is needed for any python 3, not just 3.9. I just didn't see it/don't use that code path. Bruce

Not true @brucelowekamp . They were deprecated in 3.2 but only removed in 3.9, hence why my test didn't break on Travis but did for @mikacousin in #1690 which was what confused me at first: https://docs.python.org/3/whatsnew/3.9.html#removed