-
Notifications
You must be signed in to change notification settings - Fork 202
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
Fix the Python RPC tests on Big Endian architectures #1687
Fix the Python RPC tests on Big Endian architectures #1687
Conversation
Hopefully we don't ever have to use this unless I've made some huge mea culpa and we'll go straight to 0.11.0
Now also in #1703 |
@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? |
# The RPC header (version and size) is encoded in native format, so flip that | ||
# part of the expected data where necessary (as our expected is from a | ||
# little endian source | ||
if sys.byteorder == 'big': |
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.
Technically we could just run it all of the time, as it wouldn't change the order on an LE machine, but that seemed a bit unnecessary
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 |
I'd be tempted to agree with you @brucelowekamp and I did consider fixing it. However due to the limitations mentioned here: 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:
Having had a better look, that code is in our codebase: ola/python/ola/rpc/StreamRpcChannel.py Lines 246 to 250 in d101097
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! |
…into 0.10-clang-latest
Okay @yoe this definitely cracks it. Here's a broken build on s390x (thanks to Travis): And here's one with that fix applied on top (still on the same architecture): |
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 <notifications@github.com> 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:
#1687 (comment)
--
Verstuurd vanaf mijn Android apparaat met K-9 Mail. Excuseer mijn beknoptheid.
|
…into 0.10-clang-latest
While this breaks the theoretical Python 2.6 support, other tests have already used the newer asserts. There's also a separate PR in to sort this backwards compatibility anyway.
…into 0.10-clang-latest
…osed to be two calls, testFetchDmx covers what we want to test anyway
…o 0.10-clang-latest
…into 0.10-clang-latest
…into 0.10-clang-latest
New placeholder for NEWSFix a typo in NEWSFix the OS X builds on TravisAdd a missing semicolon to the build for if we don't have CLOCK_MONOTONICThe above has all been moved into #1703
This is now mostly: