-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Python 2 and 3 compatibility #8096
Conversation
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.
Some initial feedback.
ffd88cc
to
1cc98c0
Compare
Refreshed based on @Conan-Kudo initial review feedback. @loli10K @aerusso I'm going to need a hand finishing the conversion of
There may also need to be some more updates to Can someone also please confirm the minimum versions I set are correct: Python 2.6+, or Python 3.4+: This means that by default
|
For Python 3, should we install arc_summary3 as arc_summary? |
Good idea, now would be the best time. The output is slightly different which may break some scripts, but not wildly so (whitespace, capitalization, extra rows). |
Refreshed. The latest version addressed all the packaging issues I'm aware of related to this change. That should only leave updating test-runner and pyzfs for Python 3. |
This is troubling, buildbot does not detect these as failures
The following patch seems to solve this kind of issue with the logging FileHandler: diff --git a/tests/test-runner/bin/test-runner.py b/tests/test-runner/bin/test-runner.py
index a0fcd1c61..7f7db2dc0 100755
--- a/tests/test-runner/bin/test-runner.py
+++ b/tests/test-runner/bin/test-runner.py
@@ -689,7 +689,7 @@ class TestRun(object):
fail('%s' % e)
filename = os.path.join(self.outputdir, 'log')
- logfile = logging.FileHandler(filename)
+ logfile = logging.FileHandler(filename, encoding='utf-8')
logfile.setLevel(logging.DEBUG)
logfilefmt = logging.Formatter('%(message)s')
logfile.setFormatter(logfilefmt) |
@loli10K thanks, I added your test-runner change to this patch stack. |
tests/test-runner/bin/test-runner.py
Outdated
@@ -106,7 +108,7 @@ def _read(self): | |||
for easy sorting/merging later. | |||
""" | |||
fd = self.fileno() | |||
buf = os.read(fd, 4096) | |||
buf = bytes.decode(os.read(fd, 4096)) |
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.
The Coverage builder (Python 3.5) is now dying here (http://build.zfsonlinux.org/builders/Ubuntu%2017.04%20x86_64%20Coverage%20%28TEST%29/builds/4335/steps/shell_9/logs/tests):
Traceback (most recent call last):
File "/var/lib/buildbot/slaves/zfs/Ubuntu_17_04_x86_64_Coverage__TEST_/build/zfs/tests/test-runner/bin/test-runner.py", line 922, in <module>
main()
File "/var/lib/buildbot/slaves/zfs/Ubuntu_17_04_x86_64_Coverage__TEST_/build/zfs/tests/test-runner/bin/test-runner.py", line 917, in main
testrun.run(options)
File "/var/lib/buildbot/slaves/zfs/Ubuntu_17_04_x86_64_Coverage__TEST_/build/zfs/tests/test-runner/bin/test-runner.py", line 728, in run
self.testgroups[testgroup].run(self.logger, options)
File "/var/lib/buildbot/slaves/zfs/Ubuntu_17_04_x86_64_Coverage__TEST_/build/zfs/tests/test-runner/bin/test-runner.py", line 479, in run
test.run(options)
File "/var/lib/buildbot/slaves/zfs/Ubuntu_17_04_x86_64_Coverage__TEST_/build/zfs/tests/test-runner/bin/test-runner.py", line 235, in run
self.result.stdout, self.result.stderr = self.collect_output(proc)
File "/var/lib/buildbot/slaves/zfs/Ubuntu_17_04_x86_64_Coverage__TEST_/build/zfs/tests/test-runner/bin/test-runner.py", line 201, in collect_output
fd.read()
File "/var/lib/buildbot/slaves/zfs/Ubuntu_17_04_x86_64_Coverage__TEST_/build/zfs/tests/test-runner/bin/test-runner.py", line 99, in read
while self._read() is not None:
File "/var/lib/buildbot/slaves/zfs/Ubuntu_17_04_x86_64_Coverage__TEST_/build/zfs/tests/test-runner/bin/test-runner.py", line 111, in _read
buf = bytes.decode(os.read(fd, 4096))
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe1 in position 687: invalid continuation byte
It still troubles me buildbot does not mark the " zfstests" step as "FAILED" in the build summary: does it make sense to abort the build (and report) when such unhandled errors happens?
diff --git a/scripts/zfs-tests.sh b/scripts/zfs-tests.sh
index 6fdd658f7..996dca699 100755
--- a/scripts/zfs-tests.sh
+++ b/scripts/zfs-tests.sh
@@ -583,8 +583,11 @@ REPORT_FILE=$(mktemp -u -t zts-report.XXXX -p "$FILEDIR")
#
msg "${TEST_RUNNER} ${QUIET} -c ${RUNFILE} -T ${TAGS} -i ${STF_SUITE}" \
"-I ${ITERATIONS}"
+set -o pipefail
${TEST_RUNNER} ${QUIET} -c "${RUNFILE}" -T "${TAGS}" -i "${STF_SUITE}" \
-I "${ITERATIONS}" 2>&1 | tee "$RESULTS_FILE"
+RESULT=$? # <- fail with this != 0
+set +o pipefail
#
# Analyze the results.
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.
Right, I definitely think this should fail. That last thing we want is to incorrectly reporting success here. Let me also pull in your pipefail
change.
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.
We should be able to fix this by explicitly forcing UTF-8 encoding here:
diff --git a/tests/test-runner/bin/test-runner.py b/tests/test-runner/bin/test-runner.py
index 7f7db2d..d3a356a 100755
--- a/tests/test-runner/bin/test-runner.py
+++ b/tests/test-runner/bin/test-runner.py
@@ -108,7 +108,7 @@ class Output(object):
for easy sorting/merging later.
"""
fd = self.fileno()
- buf = bytes.decode(os.read(fd, 4096))
+ buf = bytes.decode(os.read(fd, 4096), encoding='utf-8')
if not buf:
return None
if '\n' not in buf:
We also need to convert tests/zfs-tests/tests/functional/channel_program/lua_core/tst.lib_table.lua
from ISO-8859 to UTF-8 to fix the following issue:
Test: /usr/share/zfs/zfs-tests/tests/functional/channel_program/lua_core/tst.large_prog (run as root) [00:00] [PASS]
Traceback (most recent call last):
File "/usr/share/zfs/test-runner/bin/test-runner.py", line 922, in <module>
main()
File "/usr/share/zfs/test-runner/bin/test-runner.py", line 917, in main
testrun.run(options)
File "/usr/share/zfs/test-runner/bin/test-runner.py", line 728, in run
self.testgroups[testgroup].run(self.logger, options)
File "/usr/share/zfs/test-runner/bin/test-runner.py", line 479, in run
test.run(options)
File "/usr/share/zfs/test-runner/bin/test-runner.py", line 235, in run
self.result.stdout, self.result.stderr = self.collect_output(proc)
File "/usr/share/zfs/test-runner/bin/test-runner.py", line 201, in collect_output
fd.read()
File "/usr/share/zfs/test-runner/bin/test-runner.py", line 99, in read
while self._read() is not None:
File "/usr/share/zfs/test-runner/bin/test-runner.py", line 111, in _read
buf = bytes.decode(os.read(fd, 4096))
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe1 in position 687: invalid continuation byte
Test PASS from my Fedora27 builder:
[root@linux zfs]# sudo -u nobody -s ./scripts/zfs-tests.sh -d /var/tmp -t tests/functional/channel_program/lua_core/tst.libraries
Test: /usr/src/zfs/tests/zfs-tests/tests/functional/channel_program/lua_core/setup (run as root) [00:01] [PASS]
Test: /usr/src/zfs/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.libraries (run as root) [00:00] [PASS]
Test: /usr/src/zfs/tests/zfs-tests/tests/functional/channel_program/lua_core/cleanup (run as root) [00:00] [PASS]
Results Summary
PASS 3
Running Time: 00:00:03
Percent passed: 100.0%
Log directory: /var/tmp/test_results/20181113T212434
Tests with results other than PASS that are expected:
Tests with result of PASS that are unexpected:
Tests with results other than PASS that are unexpected:
[root@linux zfs]# file tests/zfs-tests/tests/functional/channel_program/lua_core/tst.lib_table.lua*
tests/zfs-tests/tests/functional/channel_program/lua_core/tst.lib_table.lua: UTF-8 Unicode text
tests/zfs-tests/tests/functional/channel_program/lua_core/tst.lib_table.lua.old: ISO-8859 text
[root@linux zfs]#
5f66414
to
0ba3d48
Compare
This needs to be marked to block the 0.8.0 release, and #7828 should probably be closed since this supersedes it. |
0ba3d48
to
7121392
Compare
9e95191
to
5b23cba
Compare
Codecov Report
@@ Coverage Diff @@
## master #8096 +/- ##
==========================================
- Coverage 78.57% 78.34% -0.23%
==========================================
Files 379 379
Lines 114924 114924
==========================================
- Hits 90299 90042 -257
- Misses 24625 24882 +257
Continue to review full report at Codecov.
|
These changes are efficient and valid in python 2 and 3. For the most part, they are also pythonic. * 2to3 conversion * add __future__ imports * iterator changes * integer division * relative import fixes Signed-off-by: Antonio Russo <antonio.e.russo@gmail.com>
* All pool, dataset, and nvlist keys must be of type bytes. Signed-off-by: John Kennedy <john.kennedy@delphix.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
* Updated unit tests to be compatbile with python 2 or 3. In most cases all that was required was to add the 'b' prefix to existing strings to convert them to type bytes for python 3 compatibility. * There were several places where the python version need to be checked to remain compatible with pythong 2 and 3. Some one more seasoned with Python may be able to find a way to rewrite these statements in a compatible fashion. Signed-off-by: John Wren Kennedy <john.kennedy@delphix.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Almost all of the Python code in the respository has been updated to be compatibile with Python 2.6, Python 3.4, or newer. The only exceptions are arc_summery3.py which requires Python 3, and pyzfs which requires at least Python 2.7. This allows us to maintain a single version of the code and support most default versions of python. This change does the following: * Sets the default shebang for all Python scripts to python3. If only Python 2 is available, then at install time scripts which are compatible with Python 2 will have their shebangs replaced with /usr/bin/python. This is done for compatibility until Python 2 goes end of life. Since only the installed versions are changed this means Python 3 must be installed on the system for test-runner when testing in-tree. * Added --with-python=<2|3|3.4,etc> configure option which sets the PYTHON environment variable to target a specific python version. By default the newest installed version of Python will be used or the preferred distribution version when creating pacakges. * Fixed --enable-pyzfs configure checks so they are run when --enable-pyzfs=check and --enable-pyzfs=yes. * Enabled pyzfs for Python 3.4 and newer, which is now supported. * Renamed pyzfs package to python<VERSION>-pyzfs and updated to install in the appropriate site location. For example, when building with --with-python=3.4 a python34-pyzfs will be created which installs in /usr/lib/python3.4/site-packages/. * Renamed the following python scripts according to the Fedora guidance for packaging utilities in /bin - dbufstat.py -> dbufstat - arcstat.py -> arcstat - arc_summary.py -> arc_summary - arc_summary3.py -> arc_summary3 * Updated python-cffi package name. On CentOS 6, CentOS 7, and Amazon Linux it's called python-cffi, not python2-cffi. For Python3 it's called python3-cffi or python3x-cffi. * Install one version of arc_summary. Depending on the version of Python available install either arc_summary2 or arc_summary3 as arc_summary. The user output is only slightly different. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Since we're only installing one version of arc_summary we only need one test case. Update the test to determine which version is available and then test its supported flags. Remove files for misc tests which should have been cleaned up. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Updated to be compatible with Python 2.6, 2.7, 3.5 or newer. Signed-off-by: John Wren Kennedy <john.kennedy@delphix.com>
5b23cba
to
6499f26
Compare
@loli10K @Conan-Kudo @aerusso @GregorKopka @johnramsden the updated patch stack is ready for review. The the updated pyzfs library and unit tests have been tested on Python 2.7, 3.4, 3.5, 3.6, and 3.7 and the build system was updated so a particular version can be requested. |
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.
Pending the final run of the tests, it looks good to me.
Fix exception caught by the buildbot. This should be squashed in the final merge. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
* All pool, dataset, and nvlist keys must be of type bytes. Reviewed-by: John Ramsden <johnramsden@riseup.net> Reviewed-by: Neal Gompa <ngompa@datto.com> Reviewed-by: loli10K <ezomori.nozomu@gmail.com> Signed-off-by: John Kennedy <john.kennedy@delphix.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #8096
* Updated unit tests to be compatbile with python 2 or 3. In most cases all that was required was to add the 'b' prefix to existing strings to convert them to type bytes for python 3 compatibility. * There were several places where the python version need to be checked to remain compatible with pythong 2 and 3. Some one more seasoned with Python may be able to find a way to rewrite these statements in a compatible fashion. Reviewed-by: John Ramsden <johnramsden@riseup.net> Reviewed-by: Neal Gompa <ngompa@datto.com> Reviewed-by: loli10K <ezomori.nozomu@gmail.com> Signed-off-by: John Wren Kennedy <john.kennedy@delphix.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #8096
Almost all of the Python code in the respository has been updated to be compatibile with Python 2.6, Python 3.4, or newer. The only exceptions are arc_summery3.py which requires Python 3, and pyzfs which requires at least Python 2.7. This allows us to maintain a single version of the code and support most default versions of python. This change does the following: * Sets the default shebang for all Python scripts to python3. If only Python 2 is available, then at install time scripts which are compatible with Python 2 will have their shebangs replaced with /usr/bin/python. This is done for compatibility until Python 2 goes end of life. Since only the installed versions are changed this means Python 3 must be installed on the system for test-runner when testing in-tree. * Added --with-python=<2|3|3.4,etc> configure option which sets the PYTHON environment variable to target a specific python version. By default the newest installed version of Python will be used or the preferred distribution version when creating pacakges. * Fixed --enable-pyzfs configure checks so they are run when --enable-pyzfs=check and --enable-pyzfs=yes. * Enabled pyzfs for Python 3.4 and newer, which is now supported. * Renamed pyzfs package to python<VERSION>-pyzfs and updated to install in the appropriate site location. For example, when building with --with-python=3.4 a python34-pyzfs will be created which installs in /usr/lib/python3.4/site-packages/. * Renamed the following python scripts according to the Fedora guidance for packaging utilities in /bin - dbufstat.py -> dbufstat - arcstat.py -> arcstat - arc_summary.py -> arc_summary - arc_summary3.py -> arc_summary3 * Updated python-cffi package name. On CentOS 6, CentOS 7, and Amazon Linux it's called python-cffi, not python2-cffi. For Python3 it's called python3-cffi or python3x-cffi. * Install one version of arc_summary. Depending on the version of Python available install either arc_summary2 or arc_summary3 as arc_summary. The user output is only slightly different. Reviewed-by: John Ramsden <johnramsden@riseup.net> Reviewed-by: Neal Gompa <ngompa@datto.com> Reviewed-by: loli10K <ezomori.nozomu@gmail.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #8096
Since we're only installing one version of arc_summary we only need one test case. Update the test to determine which version is available and then test its supported flags. Remove files for misc tests which should have been cleaned up. Reviewed-by: John Ramsden <johnramsden@riseup.net> Reviewed-by: Neal Gompa <ngompa@datto.com> Reviewed-by: loli10K <ezomori.nozomu@gmail.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #8096
Updated to be compatible with Python 2.6, 2.7, 3.5 or newer. Reviewed-by: John Ramsden <johnramsden@riseup.net> Reviewed-by: Neal Gompa <ngompa@datto.com> Reviewed-by: loli10K <ezomori.nozomu@gmail.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: John Wren Kennedy <john.kennedy@delphix.com> Closes #8096
With Python 2 (slowly) approaching EOL and its removal from distribitions already being planned (Fedora), the existing Python 2 code needs to be transitioned to Python 3. This patch stack updates the Python code to be compatible with Python 2.7, 3.4, 3.5, 3.6, and 3.7. Reviewed-by: John Ramsden <johnramsden@riseup.net> Reviewed-by: Neal Gompa <ngompa@datto.com> Reviewed-by: loli10K <ezomori.nozomu@gmail.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: John Wren Kennedy <john.kennedy@delphix.com> Reviewed-by: Antonio Russo <antonio.e.russo@gmail.com> Closes #8096
Updated to be compatible with Python 2.6, 2.7, 3.5 or newer. Reviewed-by: John Ramsden <johnramsden@riseup.net> Reviewed-by: Neal Gompa <ngompa@datto.com> Reviewed-by: loli10K <ezomori.nozomu@gmail.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: John Wren Kennedy <john.kennedy@delphix.com> Closes openzfs#8096
Updated to be compatible with Python 2.6, 2.7, 3.5 or newer. Reviewed-by: John Ramsden <johnramsden@riseup.net> Reviewed-by: Neal Gompa <ngompa@datto.com> Reviewed-by: loli10K <ezomori.nozomu@gmail.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: John Wren Kennedy <john.kennedy@delphix.com> Closes openzfs#8096
Updated to be compatible with Python 2.6, 2.7, 3.5 or newer. Reviewed-by: John Ramsden <johnramsden@riseup.net> Reviewed-by: Neal Gompa <ngompa@datto.com> Reviewed-by: loli10K <ezomori.nozomu@gmail.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: John Wren Kennedy <john.kennedy@delphix.com> Closes #8096
Motivation and Context
Update the build system to allow a specific version of Python to be targeted.
Since most of the Python code has been updated to be compatible with
Python 2.7 or Python 3.4 this allows us to install the correct version for the
distribution. This is necessary to support upcoming Fedora release which
will remove Python 2 entirely, and CentOS which will never provide Python 3
from the default repositories.
This change builds on pyzfs updates done by @aerusso in #7828.
Description
Almost all of the Python code in the respository has been updated
to be compatibile with Python 2.6, Python 3.4, or newer. The only
exceptions are arc_summery3.py which requires Python 3, and pyzfs
which requires at least Python 2.7. This allows us to maintain a
single version of the code and support most default versions of
python. This change does the following:
Sets the default shebang for all Python scripts to python3. If
only Python 2 is available, then at install time scripts which
are compatible with Python 2 will have their shebangs replaced
with /usr/bin/python. This is done for compatibility until
Python 2 goes end of life. Since only the installed versions
are changed this does mean Python 3 must be installed on the
system when testing in-tree.
Added --with-python=<2|3|3.4,etc> configure option which is used
to set the PYTHON environment variable to target a specific
python version. By default the newest installed version of
Python we be used.
Fixed --enable-pyzfs configure checks so they are run when
--enable-pyzfs=check and --enable-pyzfs=yes.
Enabled pyzfs for Python 3.4 and newer, which is now supported.
Renamed pyzfs package to python-pyzfs and updated to
install in the appropriate site location. For example, when
building with --with-python=3.4 a python34-pyzfs will be
created which installs in /usr/lib/python3.4/site-packages/.
How Has This Been Tested?
Packages build on CentOS 6 and CentOS 7 for Python 2.6, 2.7, and 3.4.
Submitted to the CI for additional build system testing.
Types of changes
Checklist:
Signed-off-by
.