update mail-alarm and usb_scan to python3

[acefei]

As @bernhardkaindl mentioned, those file already support python3, so this PR only changed:

• update shebang
• sort out imports

[psafont]

It's not obvious why this transformation is made, nor why it's correct

[bernhardkaindl]

It's also not obvious why any of the transformations in this PR are made.

Both, mail-alarm and usb_scan already support Python3, since this commit in 2019:
3179d66

This commit exactly describes how the changes were made, which is not described here.

The changes are described as:

    remove redundant else after return
    remove redundant parameters in super(), ref https://docs.python.org/3/library/functions.html#super
    use with open() statement
    remove redundant/unused variables

Clearly, many changes were made by automated tools, others seem to be manual changes.

Don't mix automated changes with manual changes in a single PR and even a single commit, as this makes them basically needlessly hard to review in a sensible manner.

This raise the chances to overlook such unexplained changes.
Kudos to Pau that he found this change buried in all the automatic reformatting.

While the use "with open()" would have theoretically some merit, if this these scripts were long-running daemons, but they aren't, so any unclosed open files are closed by the OS when the program is terminated.

That is not to say that with open should not be used, but just that it is not needed.

Without further explanations, this PR is a code cleanup PR, not a Python3 PR.

From my point of view, this PR is a hard NACK (will not approve), because it:

1. Mixes automated with manual changes, making them hard to review.
2. It is not obviously needed, these tools already work with Python3.

If you like to contribute the with open change, open a new PR with only it.

Likewise for other 'else after return', but it is harmless, except that your IDE may warn you about it.

The other changes are even far less of a fixed "problem" that would belong to a Python3 PR.

[acefei]

It's not obvious why this transformation is made, nor why it's correct

The * iterable unpacking operator was introduced in Python 3.5, which is useful as a more readable way of summing iterables into a list, ref https://peps.python.org/pep-0448/

[robhoes]

The CI tests are failing with a syntax error on this line.

[acefei]

The CI tests are failing with a syntax error on this line.

Since all of existed python scripts covered by unittests are upgraded to python3, I'm going to remove python2 CI from github action.

[bernhardkaindl]

See my comment:
#5296 (comment)

I'll be on away until 2nd January, and as I wrote in the comment following Pau's finding, for me, this PR is not review and approvable for me due to the mixed amount of changes (see my longer comment for details).

The with open is a nice modernisation and file descriptor leak fix, but as these are scripts which run to completion, not long-running daemons, it's also not a real problem.

If you have actual fixes where the code does not work with Python3 in practice, open a new PR for each problem, with a description of the issue and how to test it.

[acefei]

See my comment: #5296 (comment)

I'll be on away until 2nd January, and as I wrote in the comment following Pau's finding, for me, this PR is not review and approvable for me due to the mixed amount of changes (see my longer comment for details).

The with open is a nice modernisation and file descriptor leak fix, but as these are scripts which run to completion, not long-running daemons, it's also not a real problem.

If you have actual fixes where the code does not work with Python3 in practice, open a new PR for each problem, with a description of the issue and how to test it.

1. About Pau's comments, I've replied in update mail-alarm and usb_scan to python3 #5296 (comment)
2. The other changes are even far less of a fixed "problem" that would belong to a Python3 PR. What I did is writting a modernize python3 code instead of python2 and python3 compatible code, in other word, most of changes are introduced after python3.
3. It's not good practice that harmless code that is bad code smell should be left unmodified, I think cognitive inertia may lead to future mistakes.
4. There is a github action to run unittest to check if the changes in script run as expect.

[bernhardkaindl]

2. The other changes are even far less of a fixed "problem" that would belong to a Python3 PR. What I did is writting a modernize python3 code instead of python2 and python3 compatible code, in other word, most of changes are introduced after python3.

Thanks for admitting that your commits are not updating to Python3, but instead modernize Python3 code instead and remove Python2 compatibility.

As you admitted that, correct your commit messages and the PR title from "update ... to Python3" to:

"Modernize ... and remove Python2 compatibility"

Please apply this correction to:

• The commit message of commit 1
• The commit message of commit 2
• The caption title of this PR

3. It's not good practice that harmless code that is bad code smell should be left unmodified, I think cognitive inertia may lead to future mistakes.

Side note: I think else after return does not always have to be an indication of bad code. It may have just been a pattern preferred by the initial XenServer Python developers. I don't like it as well, and it should be refactored, of course.

But one's mileage of calling it a code smell like you did may vary. It's a pylint and sourcery warning that can be disabled and be fixed separately.

On your main comment:

I don't see the claim ("I think cognitive inertia may lead to future mistakes") as having been proven. If you make such claims, please provide some evidence for it.

The opposite of it is the famous, seemingly universal rule in programming, "if it works, don't touch it". It reflects the caution often associated with making unnecessary changes to a working system.

Making changes to functioning code can introduce new bugs or unforeseen issues. This does not mean that software should not be continuously improved and maintained.

But there is one thing that's worse than both, in my humble opinion:

Misrepresenting changes and work as doing X while doing Y.

Now, you admitted yourself that this PR is modernizing the code and removing Python2 compatibility, which is not the same as updating to support Python3. It already supports Python3. So correct your commit messages and your PR's title please to tell what they actually do.

I ask for these corrections for false claims because these changes may not be integrated into currently released XS8.2 CU1 hotfixes because it does not have Python 3.5+.

4. There is a github action to run unittest to check if the changes in script run as expect.

Some evidence that is easy to verify that the unit tests are sufficiently verifying all your modernisations would be good.

For example, is there a code coverage report web page, e.g. at cover from CI or diff-coverage check against the master branch that shows that your modernisations are covered by the unit tests?

Example Codecov report: https://app.codecov.io/gh/xenserver/python-libs/pull/40?src=pr&el=h1

In general, the clean-up of the bad code smells (like fixing pylint's else after return warnings) is good, but they have nothing to do with:

• modernisations
• using newer, less-known Python 3.5+ features
• removing Python2 compatibility.

You could have submitted the clean-up of bad code smells completely independent of the above as separate PRs. This would make each of these PRs more clear, with the correct commit messages and PR titles, of course.

[bernhardkaindl]

I like the cleanup of the not needed enumerate(),
but what is the coding rule for renaming match to _match here?

I did not see other examples for single pre underscored variables in this code base here, so it looks inconsistent. Furthermore, I also found no explanation that would match its use in a local loop variable on a quick search.

From what I found, the _variable naming scheme is for private class variables like self._PATTERN used here, not for local variables. Based on this, it looks like a naming scheme violation.

[acefei]

The change here is the avoidance of using the same name as a builtin variable, match is a function in builtin module re.match.
if you don't like using underscore naming as local variable, we can consider alternatives.

[bernhardkaindl]

As you improve the import ordering, please also fix the import order of from xml.dom.minidom import parseString to be the last entry of the 3rd party libraries, not a local/application specific import, conforming to the PEP8 import rules, like isort applies them.

You can apply this change by clicking the button "Commit suggestion" in my review comment.

[acefei]

As you admitted that, correct your commit messages and the PR title from "update ... to Python3" to:
"Modernize ... and remove Python2 compatibility"

Fair enough, I will update this.

I don't see the claim ("I think cognitive inertia may lead to future mistakes") as having been proven. If you make such claims, please provide some evidence for it.

When I said that, I intended to respond to this segment as follows.

While the use "with open()" would have theoretically some merit, if this these scripts were long-running daemons, but they aren't, so any unclosed open files are closed by the OS when the program is terminated.

That is not to say that with open should not be used, but just that it is not needed.

I insist on making with open() changes because I feel that it dont make sense to use inconsistent code style between daemon and non-daemon programs.

You could have submitted the clean-up of bad code smells completely independent of the above as separate PRs. This would make each of these PRs more clear, with the correct commit messages and PR titles, of course.

You're right. I'll elaborate on the final squashed commit messages.

[psafont]

I'm blocking the merge because the PR breaks xs8 builds of xapi.spec.

Current xs8 builds use python2. As long as this happens this PR cannot be merged. Is there a PR missing to xapi.spec that removes the python2 dependency?

Bytecompiling .py files below /builddir/build/BUILDROOT/xapi-23.32.0.40.g5683e9378-1.2.g838bda5.xs8.x86_64/usr/lib/python2.7 using /usr/bin/python2.7
Bytecompiling .py files below /builddir/build/BUILDROOT/xapi-23.32.0.40.g5683e9378-1.2.g838bda5.xs8.x86_64/usr/lib/python3.6 using /usr/bin/python3.6
Compiling /builddir/build/BUILDROOT/xapi-23.32.0.40.g5683e9378-1.2.g838bda5.xs8.x86_64/opt/xensource/libexec/usb_scan.py ...
  File "/opt/xensource/libexec/usb_scan.py", line 161
    _PROPS_NONABLE = [*_PRODUCT_DESC, _SERIAL]
                      ^
SyntaxError: invalid syntax



RPM build errors:
error: Bad exit status from /var/tmp/rpm-tmp.RjFDNt (%install)
    Bad exit status from /var/tmp/rpm-tmp.RjFDNt (%install)

I recommend reevaluating the approach, I think what's valuable right now is making sure all used code is python-3 compatible, not removing python2 compatibility. Once the former is done the package can be build with using only python 3, and only then these cleanups / improvements can be integrated

[acefei]

We also found this as the spec rule https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#py_byte_compile.

In current xapi.spec, py_byte_compile will compile python code with both python2 and python3 interpreter, even the Python code can only be executed by the Python 3 interpreter on xs8. Given refactoring the spec file is convoluted, we can't do it shortly, I will re-work this to do minium changes to keep compatible, finally, I will provide UT and karma Test result.

[acefei]

Karma Testing 192605 (Dev Run) passed.

[liulinC]

Merge the commit considering:

• The PR LGTM,
• XenRT is happy.

[psafont]

This is being reverted in #5361 because it has errors. There are internal tests catching the issue, so more tests were needed to ensure it didn't break anything.

This is a big part why unit-tests are preferred: they catch errors before merging.

[robhoes]

Karma Testing 192605 (Dev Run) passed.

And unfortunately, that test suite did not actually include tests that exercise the change code.

[liulinC]

@psafont @robhoes @acefei I do agree we need more test to cover the updates.
For unittest,

1. We have test_mail-alarm for the unttest, and the unittest passed. (Pull request CI run the tests)
2. The unittest can not detect the issue in this case.

Just for this case, it seems we should run more XenRT (like ring3 BST & BVT which detect the failure)

From the calltrace, the issue happens in os.write , this builtin function is often mocked during the unittest., thus can not trigger the issue (as we do not want to really write the configuration files)

Edit: If we do want to cover the read/write in UT, we can do by refine the code to be more Ut-able, and to some temporary path. But the refine itself involve some potential risk.

From python2, this function can write bytes and string,

os.write(1,b"abc\n")
abc
4
os.write(1,"abc\n")                                                                                                                          
abc 
4

From python3, this function can only write bytes.

>>> os.write(1,b"a\n")
a
2
>>> os.write(1,"a\n")                                                                                                                            
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: a bytes-like object is required, not 'str'

Backtrace

Jan 16 12:44:53 xrtmia-12-03 xapi: [error||2522 ||xapi_message] Unexpected exception in message hook /opt/xensource/libexec/mail-alarm: INTERNAL_ERROR: [ Subprocess exited with unexpected code 1; stdout = [ ]; stderr = [ Traceback (most recent call last):\x0A\x0A File "/opt/xensource/libexec/mail-alarm", line 1055, in <module>\x0A rc = main()\x0A\x0A File "/opt/xensource/libexec/mail-alarm", line 1027, in main\x0A os.write(fd, config)\x0A\x0ATypeError: a bytes-like object is required, not 'str'\x0A\x0A ] ]