search

opencomputeproject / oom

MIT License
66 stars 29 forks source link

Oom python3 #23

Closed bacharya closed 3 years ago

bacharya commented 3 years ago

Hi,

Please review the changes with python3 support along with python2 compatibility. Please find the attached test log.

Thanks, Bhishma

OOM_test_log.txt

donboll commented 3 years ago

Hi,

Thanks for the comprehensive pull request. I see lots of good test data, both python and python3. I note also ‘opticsdump.py’, which looks like a useful addition.

I want to take some time to review the actual changes as well as test some myself. Unfortunately I will be offline for the next two weeks. So, I am not ignoring you, I just won’t be able to work on this until I’m back online.

Don

From: bacharya @. Sent: Tuesday, July 06, 2021 8:18 AM To: opencomputeproject/oom @.> Cc: Subscribed @.***> Subject: [opencomputeproject/oom] Oom python3 (#23)

Hi,

Please review the changes with python3 support along with python2 compatibility. Please find the attached test log.

Thanks, Bhishma

OOM_test_log.txt https://github.com/opencomputeproject/oom/files/6771044/OOM_test_log.txt

You can view, comment on, or merge this pull request online at:

https://github.com/opencomputeproject/oom/pull/23

Commit Summary

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/opencomputeproject/oom/pull/23 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AD6VCJ6CGFFWMDKYPSFQVTTTWMNDLANCNFSM474ZLNTQ . https://github.com/notifications/beacon/AD6VCJ6JFKFFQAR4MRHXDG3TWMNDLA5CNFSM474ZLNT2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4N7I3SKQ.gif

donboll commented 3 years ago

A quick status report on the OOM python3 submittal…

I am back from vacation :)

I read all of the code changes, they appeared to be good.

I notice some new code, including support for a couple of accton platforms in oomsysfsshim.py and a new test case test/opticsdump.py. I like them both, and will include them when I have a candidate ready to commit.

I ran a bunch of tests, and came up with a bunch of issues. Some are silly, like my python 2.7 interpreter sees ‘print()’ and it actually prints ‘()’. I’m changing all of the print() to print(“”). Some of the issues would have been difficult to find without all the test scaffolding I have built. The CFP code doesn’t work, the JSON server/shim code doesn’t work. I have developed fixes for them, at least for python 2.7. (I don’t have a JSON app that works on python3. When someone does, they/we can debug it.)

The encode/decode ‘utf-8’ code doesn’t work quite right. I’ve fought between ctypes create_string_buffer and every other Python representation of strings and bytes since the origin of OOM. It continues to plague me. I found a remarkably simple solution to some of the cases:

c = create_string_buffer(4)

s = c[:]

This works in both P2.7 and P3! It copies c to s. In P2.7 s is of type ‘str’, in P3 s is of type ‘class bytes’. This reduces get_string(x) to just one line: return(x[:])

I pulled the pull request into my development environment and find that it has all DOS newlines, rather than Linux newlines in the changed files (and even in some that didn’t change). I will switch those back so they look like native Linux (even though I actually develop on Cygwin on Windows :)).

I seem to have a working set of code that I’m satisfied with, but it is sort of a mess right now. Having solved all the issues, I’m going to start over, with a clean branch, and methodically apply all of the changes in a way I can review, test and commit. That will probably take at least a week.

I’ll post a candidate for y’all to test when I have it ready.

Don

From: bacharya @. Sent: Tuesday, July 06, 2021 8:18 AM To: opencomputeproject/oom @.> Cc: Subscribed @.***> Subject: [opencomputeproject/oom] Oom python3 (#23)

Hi,

Please review the changes with python3 support along with python2 compatibility. Please find the attached test log.

Thanks, Bhishma

OOM_test_log.txt https://github.com/opencomputeproject/oom/files/6771044/OOM_test_log.txt

You can view, comment on, or merge this pull request online at:

https://github.com/opencomputeproject/oom/pull/23

Commit Summary

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/opencomputeproject/oom/pull/23 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AD6VCJ6CGFFWMDKYPSFQVTTTWMNDLANCNFSM474ZLNTQ .

donboll commented 3 years ago

Re: OOM Python3 support... I have completed, tested and committed my changes to Bacharya's pull request. Everything that I tried now works, I'm prepared to commit this to the OOM mainline.

Oddly, I seem to have committed and pushed the changes into bacharya's oom_python3 branch. Not sure how I did that, but I did, which means that interested parties can access the changes from the pull request #23 page. Look for 'donboll added 2 commits...', the final one is 256731e.

I'd like a confirmation from bacharya and Mikhail that this version works before merging into master. If I don't get a response, I'll probably go ahead and commit late next week (~August 11).

Don

bacharya commented 3 years ago

Hi Don,

I have validated your changes in my setup and the test results look perfectly fine. Hereby I acknowledge my confirmation.

In addition, I have made minor enhancements in keytest.py and qtest.py. Please consider the changes too(commit-id 701fff3).

Thanks, Bhishma

donboll commented 3 years ago

Excellent enhancements, I almost did the same thing. Your solution is simple and effective, I like it.

Don

From: bacharya @. Sent: Friday, August 06, 2021 10:28 PM To: opencomputeproject/oom @.> Cc: donboll @.>; Assign @.> Subject: Re: [opencomputeproject/oom] Oom python3 (#23)

Hi Don,

I have validated your changes in my setup and the test results look perfectly fine. Hereby I acknowledge my confirmation.

In addition, I have made minor enhancements in keytest.py and qtest.py. Please consider the changes too(commit-id https://github.com/opencomputeproject/oom/commit/701fff34e35deb6074e0e91fc1fcda781a17a550 701fff3).

Thanks, Bhishma

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/opencomputeproject/oom/pull/23#issuecomment-894607687 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AD6VCJ5DMSOTOFJTUCW46UDT3S75HANCNFSM474ZLNTQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

donboll commented 3 years ago

Done. OOM now supports Python 3! Thank you bacharya. Latest code is available in the OOM github repo. FYI, optoe.c, the linux driver, has also been substantially updated and supports several kernel subsystems including device tree configuration and nvmem access. See all the info in oom/optoe/optoe.rst.

Don

From: bacharya @. Sent: Friday, August 06, 2021 10:28 PM To: opencomputeproject/oom @.> Cc: donboll @.>; Assign @.> Subject: Re: [opencomputeproject/oom] Oom python3 (#23)

Hi Don,

I have validated your changes in my setup and the test results look perfectly fine. Hereby I acknowledge my confirmation.

In addition, I have made minor enhancements in keytest.py and qtest.py. Please consider the changes too(commit-id https://github.com/opencomputeproject/oom/commit/701fff34e35deb6074e0e91fc1fcda781a17a550 701fff3).

Thanks, Bhishma

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/opencomputeproject/oom/pull/23#issuecomment-894607687 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AD6VCJ5DMSOTOFJTUCW46UDT3S75HANCNFSM474ZLNTQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .