Skip to content
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.

Sign up for GitHub

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

CP-41972: update xcp to support ack py3 #21

Closed
wants to merge 1 commit into from
+3 −3
Closed

CP-41972: update xcp to support ack py3 #21

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion xcp/net/biosdevname.py
View file
Original file line number Diff line number Diff line change
@@ -65,7 +65,7 @@ def all_devices_all_names():
if retcode:
continue

for device in (x.strip() for x in stdout.split("\n\n") if len(x)):
for device in (x.strip() for x in stdout.decode().split("\n\n") if len(x)):
dinfo = {}

for l in device.split("\n"):
4 changes: 2 additions & 2 deletions xcp/pci.py
View file
Original file line number Diff line number Diff line change
@@ -186,7 +186,7 @@ def __init__(self, fn):
vendor = None
cls = None

fh = open(fn)
fh = open(fn, 'r', encoding='UTF-8')
for l in fh:
line = l.rstrip()
Comment on lines +189 to 191
Copy link
Collaborator

@bernhardkaindl bernhardkaindl Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this change is not clear, the reason is:

open() does not change it's return value between Python2 and Python3:
See: https://docs.python.org/3.6/library/functions.html#open -> See "text mode (default)"

All of these work just fine and produce the same output:

python2 -c 'file=open("/etc/passwd"); lines = [line.decode() for line in file]; print(lines[0])'
python3 -c 'file=open("/etc/passwd"); lines = [line.decode() for line in file]; print(lines[0])'
python3 -c 'file=open("/etc/passwd", "r", encoding="UTF-8"); lines = [line for line in file]; print(lines[0])'

But encoding= is not supported by Python2:

python2.7 -c "open('/etc/passwd', 'r', encoding='UTF-8')"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: 'encoding' is an invalid keyword argument for this function

Also, it does not change anything because UTF-8 is already the default encoding, at least on XenServer 8.3.0 with Python3.6 installed:

[root@lcy2-dt76 ~]# python3 -c "import locale;print(locale.getpreferredencoding())"
UTF-8
[root@lcy2-dt76 ~]# python2 -c "import locale;print(locale.getpreferredencoding())"
UTF-8

If you use a different Python enviromnent for testing, and get different results, switch it to UTF-8 by default as well.

Therefore, revert this change:

Suggested change
fh = open(fn, 'r', encoding='UTF-8')
for l in fh:
line = l.rstrip()
fh = open(fn)
for l in fh:
line = l.rstrip()

Copy link
Author

@alexhimmel alexhimmel Apr 19, 2023 •
edited by bernhardkaindl
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bernhardkaindl
Got error when try the nwe branch with fh = open(fn)

About to call plugin 'get_network_devices' on host 'OpaqueRef:482bcea2-6914-9d49-ecc9-6708b236fa9e' with args '{}'
Traceback (most recent call last):
  File "./ack_cli.py", line 536, in <module>
    main(config, test_run_file)
  File "./ack_cli.py", line 508, in main
    generate_test_config(session, config, test_run_file)
  File "./ack_cli.py", line 404, in generate_test_config
    ifs = network_interfaces_to_test(session, config)
  File "./ack_cli.py", line 335, in network_interfaces_to_test
    devices = utils.get_master_network_devices(session)
  File "/opt/xensource/packages/files/auto-cert-kit/utils.py", line 2427, in get_master_network_devices
    nics = call_ack_plugin(session, 'get_network_devices')
  File "/opt/xensource/packages/files/auto-cert-kit/utils.py", line 2226, in call_ack_plugin
    args)
  File "/usr/lib/python3.6/site-packages/XenAPI.py", line 260, in __call__
    return self.__send(self.__name, args)
  File "/usr/lib/python3.6/site-packages/XenAPI.py", line 154, in xenapi_request
    result = _parse_result(getattr(self, methodname)(*full_params))
  File "/usr/lib/python3.6/site-packages/XenAPI.py", line 234, in _parse_result
    raise Failure(result['ErrorDescription'])
XenAPI.Failure: ['XENAPI_PLUGIN_FAILURE', 'get_network_devices', 'UnicodeDecodeError', "'ascii' codec can't decode byte 0xc2 in position 265: ordinal not in range(128)"]

Can fix this issue when specify the correct codec when opening the file or when calling the decode method e.g.
fh = open(fn, 'r', encoding='UTF-8') or somewhere decode the stdout with stdout.decode('UTF-8')

if line == '' or line.startswith('#'):
@@ -260,7 +260,7 @@ def __init__(self):
stdout = subprocess.PIPE)
for l in cmd.stdout:
line = l.rstrip()
el = [x for x in line.replace('"', '').split() if not x.startswith('-')]
Copy link
Collaborator

@bernhardkaindl bernhardkaindl Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alexhimmel because Popen() changes it's return value to bytes, this is a needed fix
(BTW: In general, it's always a very good idea to explain why a change is needed).

But, the current testsuite also needs to be update to reflect this behavior, otherwise it would fail to pass for Python3. I've done the needed testsuite updates in my own fork, see below.

Thus, this change must be applied with the testsuite fix to have working CI/PR checks.

Likewise, the tests for biosdevname have to be fixed and cmd.py is also missing the same fixes. I've done all this as well.

You can review, checkout and test the needed fixes here: xenserver-next#2

Please check if the lated branch works for you:
https://github.com/xenserver-next/python-libs/tree/testsuite-driven-py3+Popen

PS: We have to fix the Python3 testsuite for #17 before we can merge these fixes, but it's next on my TODO, and you can test the new branch https://github.com/xenserver-next/python-libs/tree/testsuite-driven-py3+Popen fixes all Python3 issues you detec in python-lib.

Best Regards, Bernhard

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bernhardkaindl
Thank you for helping me to review this PR, i will test with new branch "testsuite-driven-py3+Popen".
Regards,
Tian

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixes issues which the changes in this PR adressed are now merged to master.

Hi @alexhimmel, you could schedule re-testing the ACK XAPI plugin with the new master branch in an upcoming sprint!

PS: The Python3 migration is not done yet, there are still many issues (or at least untested cases) which need testing and likely fixing.

For example the github action summay page is currently still showing pytype errors in xcp/accessor.py and xcp/repository.py.

If the ACK plugin uses such modules or you run into any other error, please open an issue here to report the problem, thanks!

el = [x for x in line.decode().replace('"', '').split() if not x.startswith('-')]
self.devs[el[0]] = {'id': el[0],
'class': el[1][:2],
'subclass': el[1][2:],