Pr15 split part3 #18

qinzhang22 · 2023-11-02T05:32:24Z

This is removing the use 'file' and updating 'xmlrpc, socketserver, urllib' syntax for python3. For this part, we didn't consider python2 compatibility considering it will make code complicated.

andyhhp · 2023-11-02T10:38:21Z

The first patch (file) is fine, and could have been part of the previous misc if I'd noticed it.

However, the second patch is definitely not Py2-safe, so we can't take it in that form. See:

https://python-future.org/compatible_idioms.html#urllib-module
https://python-future.org/compatible_idioms.html#xmlrpc-module
https://python-future.org/compatible_idioms.html#socketserver

Judging by the three of those, I'd suggest using the python-future option for compatibility, as that's available in xs8 & xs9, and just a small change (again) in terms of imports.

bernhardkaindl · 2023-11-02T23:11:46Z

XSConsoleRemoteTest.py

@@ -13,27 +13,27 @@
 # with this program; if not, write to the Free Software Foundation, Inc.,
 # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

-import os, socket, xmlrpclib
+import os, socket, xmlrpc.client


Suggested change

import os, socket, xmlrpc.client

import os

import socket

See my 2nd requested change below, Andrew's comment and my change request for context.

bernhardkaindl

With reference to Andrew's comment and with consideration that future is not maintained anymore and is broken in Python3.12, please use the two hunks I suggested instead of the 2nd commit, which also make the changes considerably smaller.

bernhardkaindl · 2023-11-02T23:13:16Z

XSConsoleRemoteTest.py

+import socketserver
+import xmlrpc.server


Suggested change

import socketserver

import xmlrpc.server

try:

import xmlrpclib

import SocketServer

import SimpleXMLRPCServer

except:

import xmlrpc.client as xmlrpclib

import socketserver as SocketServer

import xmlrpc.server as SimpleXMLRPCServer

I advocate for this simple solution to replace this entire commit with these two hunks.

The main reason is that the the python-future module became unmaintained and projects are already forced to drop their use of python-future because of this when they want to support Python 3.12. One Example:

https://github.com/gkovacs/lz-string-python/pull/6/files

A fixup for this particular issue of future using import imp which has been dropped has been submitted 3 weeks ago, but the maintainers of future have gone missing in January, which is now 9 months without any commit or merge.

The deprecation warning for the removal of imp was added starting from release of 3.7.
3.10 announced will be removed by 3.12:

# python3.10 -Wdefault -c 'from future import standard_library;standard_library.install_aliases()' /usr/lib/python3/dist-packages/future/standard_library/__init__.py:65: DeprecationWarning: the imp module is deprecated in favour of importlib and slated for removal in Python 3.12; see the module's documentation for alternative uses import imp

Python 3.12 did follow thru and removed imp:

# python3.12 -c 'from future import standard_library;standard_library.install_aliases()' Traceback (most recent call last): File "<string>", line 1, in <module> File "/usr/lib/python3/dist-packages/future/standard_library/__init__.py", line 65, in <module> import imp ModuleNotFoundError: No module named 'imp'

Therefore, just use the simple try: import method and no change to the actual code.

That's fully in line with Andrew's demand for Python2 compat and it avoids having dependency on no longer maintained packages (there is also six.moves, but that also is a not needed dependency which can become unmaintained). Besides, when pylint would used (may be good when developing new code), two pylint warnings would have to be disabled, because installing the aliases would have to be put on top, violating the import order:

# pylint: disable=wrong-import-position,wrong-import-order from future import standard_library standard_library.install_aliases()

andyhhp · 2023-11-03T00:31:36Z

and with consideration that future is not maintained anymore and is broken in Python3.12

Lovely... I'll defer to your suggestion for how best to do this.

liulinC · 2023-11-03T02:01:00Z

Again, I want to highlight,
XS8 already has python3 (3.6), so pure python3 code is compatible for XS9 and XS8 stream. (That is also the goal of the Epic to remove python2 from dom0), Note: XSConsole is not a lib like xen, or scp-python-libs, It does not need to provides py2 version for other callers.

The only usage of py2 of XSConsole is for Yangtze, which will be EoL in 2025.
Considering this repo only updates several times during years, I have good reason to believe we does not need much p2 updates.
If we do need to update Yangtze, then branch out for Yangtze support.

On the other side, to make master compatible for py2, we make the code unnecessary complicated.
@andyhhp @rosslagerwall @bernhardkaindl

andyhhp · 2023-11-03T03:13:46Z

Hmm, on second thoughts, you're right. There's no dependency of xsconsole that's going to need it to stay py2 compatible, and we're not going to be backporting anything but critical fixes to ytz. (Need to double check the way branches are handled in the spec repo, but I expect that will be fine.)

So, new plan, move for Py3.6 baseline. However, master needs to remain atomically py2 or py3 compatible, and not a mix of fixes which leaves it broken in both.

So, please can we collect together all commits which are py2-safe fixes, which can get reviewed and merged freely, and please keep all commits which break compatibility with py2 in a separate single PR so it can be merged all in one go. The final commit on the py3 branch should bump the major version number of xsconsole seeing as its a bit step change.

Sound ok?

liulinC · 2023-11-03T04:56:49Z

Hmm, on second thoughts, you're right. There's no dependency of xsconsole that's going to need it to stay py2 compatible, and we're not going to be backporting anything but critical fixes to ytz. (Need to double check the way branches are handled in the spec repo, but I expect that will be fine.)

So, new plan, move for Py3.6 baseline. However, master needs to remain atomically py2 or py3 compatible, and not a mix of fixes which leaves it broken in both.

So, please can we collect together all commits which are py2-safe fixes, which can get reviewed and merged freely, and please keep all commits which break compatibility with py2 in a separate single PR so it can be merged all in one go. The final commit on the py3 branch should bump the major version number of xsconsole seeing as its a bit step change.

Sound ok?

Sounds a good and reasonable plan for me @qinzhang22 your thoughts?

By default, file objectes are opened in binary mode in python3. With universal_newlines=True, the file objects stdin, stdout and stderr will be opened in text mode. And it is provided for backwards compatibility. Signed-off-by: Qin Zhang （张琴） <qin.zhang@citrix.com>

qinzhang22 · 2023-11-03T07:00:31Z

Hmm, on second thoughts, you're right. There's no dependency of xsconsole that's going to need it to stay py2 compatible, and we're not going to be backporting anything but critical fixes to ytz. (Need to double check the way branches are handled in the spec repo, but I expect that will be fine.)
So, new plan, move for Py3.6 baseline. However, master needs to remain atomically py2 or py3 compatible, and not a mix of fixes which leaves it broken in both.
So, please can we collect together all commits which are py2-safe fixes, which can get reviewed and merged freely, and please keep all commits which break compatibility with py2 in a separate single PR so it can be merged all in one go. The final commit on the py3 branch should bump the major version number of xsconsole seeing as its a bit step change.
Sound ok?

Sounds a good and reasonable plan for me @qinzhang22 your thoughts?

qinzhang22 · 2023-11-03T07:09:59Z

I updated the PR per our agreement that collecting together all commits which are py2-safe fixes. Hopefully it can be reviewed and merged freely.

Signed-off-by: Qin Zhang （张琴） <qin.zhang@citrix.com>

andyhhp

Everything else LGTM.

andyhhp · 2023-11-03T13:27:56Z

plugins-base/XSFeatureSRInfo.py

@@ -112,7 +112,7 @@ def MenuRegenerator(cls, inList, inMenu):
        srList = [ sr for sr in HotAccessor().visible_sr if sr.other_config({}).get('xensource_internal', '') != 'true' ]

        # Sort list by SR shared flag then name
-        srList.sort(lambda x, y: cmp(y.shared(False), x.shared(False)) or cmp (x.name_label(''), y.name_label()))
+        srList.sort(key=lambda sr: (-sr.shared(False), sr.name_label('')))


is -sr right here?

Personally, I think the logic would be easier to follow as

# Sort list by SR shared flag then name srList.sort(key=lambda sr: sr.name_label('')) srList.sort(key=lambda sr: sr.shared(False))

maybe with a reverse=True in the second line if I've interpreted the intent of the negation properly ?

Alternatively, srList.sort(key=lambda sr: (not sr.shared(False), sr.name_label(''))) might be better ? At least it makes clear that the first part of the tuple is supposed to be a boolean.

As a tangent, passing the default-if-missing value in makes this code horrible to read and follow. Do you happen to know if this is a local artifact, or a property of the API?

is -sr right here?

Yes, it is correct. The 1st half of the original cmp(y.shared(False), x.shared(False)) had y and x reversed, so the primary sort order is reversed.

Splitting the nested sort into two consecutive sorts would make the result reliant of the sort algorithm chosen by the Python interpreter for the given data and I'd like the result to be kept clearly defined as a nested sort with two sort keys. Therefore, I see the change is correct!

Because the sort is already commented using a meaningful comment, the sort does not need further explanation:

# Sort list by SR shared flag then name srList.sort(key=lambda sr: (-sr.shared(False), sr.name_label('')))

Even though the comment says that sr.shared(False) returns a flag, nothing here ensures that it will always return a boolean. In principle, it could return anything (numbers, floats or a mix of all that). Therefore I think keeping -sr.shared(False) is the safe thing to do (does not change the behavior at all) and it's clear that - means negation.

bernhardkaindl

Reviewed the changes and checked the sort lambdas.

bernhardkaindl · 2023-11-03T21:40:14Z

plugins-base/XSFeatureSRInfo.py

@@ -112,7 +112,7 @@ def MenuRegenerator(cls, inList, inMenu):
        srList = [ sr for sr in HotAccessor().visible_sr if sr.other_config({}).get('xensource_internal', '') != 'true' ]

        # Sort list by SR shared flag then name
-        srList.sort(lambda x, y: cmp(y.shared(False), x.shared(False)) or cmp (x.name_label(''), y.name_label()))
+        srList.sort(key=lambda sr: (-sr.shared(False), sr.name_label('')))


is -sr right here?

Yes, it is correct. The 1st half of the original cmp(y.shared(False), x.shared(False)) had y and x reversed, so the primary sort order is reversed.

Splitting the nested sort into two consecutive sorts would make the result reliant of the sort algorithm chosen by the Python interpreter for the given data and I'd like the result to be kept clearly defined as a nested sort with two sort keys. Therefore, I see the change is correct!

Because the sort is already commented using a meaningful comment, the sort does not need further explanation:

# Sort list by SR shared flag then name srList.sort(key=lambda sr: (-sr.shared(False), sr.name_label('')))

Even though the comment says that sr.shared(False) returns a flag, nothing here ensures that it will always return a boolean. In principle, it could return anything (numbers, floats or a mix of all that). Therefore I think keeping -sr.shared(False) is the safe thing to do (does not change the behavior at all) and it's clear that - means negation.

liulinC

Approve basing on my review, and the discussion.

Note by Bernhard: As per the agreement with Andrew in xapi-project#18, we shall keep master py2-compatible and py3-only changes go to a py3 branch: Andrew wrote: > However, master needs to remain atomically py2 or py3 compatible, > and not a mix of fixes which leaves it broken in both. [...] > please keep all commits which break compatibility with py2 in a > separate single PR so it can be merged all in one go. > The final commit on the py3 branch should [...] Reference: xapi-project#18 (comment) Therefore, I'll squash this commit with the next commit for fixing it up keep master py2-compatible, at least until all py3 checks and a complete manual test was done by QA. Original commit message by Qin Zhang （张琴）: Remove the use of 'encode' as it's used to turn a Unicode string into a regular string in Python2 Final Remarks by Bernhard, for completeleness: - This commit also adds conversion from curses input bytes to str. - Rebased to apply on the current master branch. Co-authored-by: Bernhard Kaindl <bernhard.kaindl@cloud.com> Signed-off-by: Qin Zhang （张琴） <qin.zhang@citrix.com>

Note by Bernhard: As per the agreement with Andrew in #18, we shall keep master py2-compatible and py3-only changes go to a py3 branch: Andrew wrote: > However, master needs to remain atomically py2 or py3 compatible, > and not a mix of fixes which leaves it broken in both. [...] > please keep all commits which break compatibility with py2 in a > separate single PR so it can be merged all in one go. > The final commit on the py3 branch should [...] Reference: #18 (comment) Therefore, I'll squash this commit with the next commit for fixing it up keep master py2-compatible, at least until all py3 checks and a complete manual test was done by QA. Original commit message by Qin Zhang （张琴）: Remove the use of 'encode' as it's used to turn a Unicode string into a regular string in Python2 Final Remarks by Bernhard, for completeleness: - This commit also adds conversion from curses input bytes to str. - Rebased to apply on the current master branch. Co-authored-by: Bernhard Kaindl <bernhard.kaindl@cloud.com> Signed-off-by: Qin Zhang （张琴） <qin.zhang@citrix.com>

bernhardkaindl reviewed Nov 2, 2023

View reviewed changes

bernhardkaindl requested changes Nov 3, 2023

View reviewed changes

qinzhang22 closed this Nov 3, 2023

qinzhang22 reopened this Nov 3, 2023

qinzhang22 force-pushed the pr15-split-part3 branch from 64153bc to 7514fde Compare November 3, 2023 07:04

qinz0822 added 3 commits November 3, 2023 15:12

Use '//' as integer division

c0c47ac

Signed-off-by: Qin Zhang （张琴） <qin.zhang@citrix.com>

Use key parameter in sort() for sorting purposes

eb7eccc

Signed-off-by: Qin Zhang （张琴） <qin.zhang@citrix.com>

Misc changes

0cb382b

Signed-off-by: Qin Zhang （张琴） <qin.zhang@citrix.com>

qinzhang22 force-pushed the pr15-split-part3 branch from 7514fde to 0cb382b Compare November 3, 2023 07:13

andyhhp reviewed Nov 3, 2023

View reviewed changes

bernhardkaindl approved these changes Nov 3, 2023

View reviewed changes

liulinC approved these changes Nov 6, 2023

View reviewed changes

liulinC merged commit 922f41d into xapi-project:master Nov 6, 2023

bernhardkaindl mentioned this pull request Nov 24, 2023

Python2->Python3 update, Python2 unsafe #20

Closed

This was referenced Dec 3, 2023

Fix PR#17 commit 'misc changes', it broke listing and editing Storage Repositories #23

Merged

Split PR20 for better review: Make URLopener and xmlrpc compatible #24

Merged

Remove the use of __cmp__ and use __eq__ instead #25

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pr15 split part3 #18

Pr15 split part3 #18

qinzhang22 commented Nov 2, 2023 •

edited

Loading

andyhhp commented Nov 2, 2023 •

edited

Loading

bernhardkaindl Nov 2, 2023 •

edited

Loading

bernhardkaindl left a comment

bernhardkaindl Nov 2, 2023

bernhardkaindl Nov 3, 2023 •

edited

Loading

andyhhp commented Nov 3, 2023

liulinC commented Nov 3, 2023 •

edited

Loading

andyhhp commented Nov 3, 2023 •

edited

Loading

liulinC commented Nov 3, 2023

qinzhang22 commented Nov 3, 2023

qinzhang22 commented Nov 3, 2023

andyhhp left a comment

andyhhp Nov 3, 2023

andyhhp Nov 3, 2023 •

edited

Loading

bernhardkaindl Nov 3, 2023 •

edited

Loading

bernhardkaindl left a comment

bernhardkaindl Nov 3, 2023 •

edited

Loading

liulinC left a comment

-import socketserver
-import xmlrpc.server
+try:
+    import xmlrpclib
+    import SocketServer
+    import SimpleXMLRPCServer
+except:
+    import xmlrpc.client as xmlrpclib
+    import socketserver as SocketServer
+    import xmlrpc.server as SimpleXMLRPCServer

Pr15 split part3 #18

Pr15 split part3 #18

Conversation

qinzhang22 commented Nov 2, 2023 • edited Loading

andyhhp commented Nov 2, 2023 • edited Loading

bernhardkaindl Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

bernhardkaindl left a comment

Choose a reason for hiding this comment

bernhardkaindl Nov 2, 2023

Choose a reason for hiding this comment

bernhardkaindl Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

andyhhp commented Nov 3, 2023

liulinC commented Nov 3, 2023 • edited Loading

andyhhp commented Nov 3, 2023 • edited Loading

liulinC commented Nov 3, 2023

qinzhang22 commented Nov 3, 2023

qinzhang22 commented Nov 3, 2023

andyhhp left a comment

Choose a reason for hiding this comment

andyhhp Nov 3, 2023

Choose a reason for hiding this comment

andyhhp Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

bernhardkaindl Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

bernhardkaindl left a comment

Choose a reason for hiding this comment

bernhardkaindl Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

liulinC left a comment

Choose a reason for hiding this comment

qinzhang22 commented Nov 2, 2023 •

edited

Loading

andyhhp commented Nov 2, 2023 •

edited

Loading

bernhardkaindl Nov 2, 2023 •

edited

Loading

bernhardkaindl Nov 3, 2023 •

edited

Loading

liulinC commented Nov 3, 2023 •

edited

Loading

andyhhp commented Nov 3, 2023 •

edited

Loading

andyhhp Nov 3, 2023 •

edited

Loading

bernhardkaindl Nov 3, 2023 •

edited

Loading

bernhardkaindl Nov 3, 2023 •

edited

Loading