Bug 237403 - Tests in sys/opencrypto should be converted to Python3
Summary: Tests in sys/opencrypto should be converted to Python3
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: tests (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Enji Cooper
URL: https://bugs.freebsd.org/bugzilla/sho...
Keywords:
Depends on:
Blocks:
 
Reported: 2019-04-20 07:39 UTC by Li-Wen Hsu
Modified: 2019-08-14 23:04 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Li-Wen Hsu freebsd_committer 2019-04-20 07:39:10 UTC
Tests in sys/opencrypto should be converted to Python3, the default version of Python in the ports tree has been switched to 3.
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2019-04-20 07:58:01 UTC
While the default version has changed, python27 is still available

Can the test systems be modified to use Python 2.x?
Do they use ports or packages? 
If the latter, do they use a custom package repo or the default ones?
Comment 2 Enji Cooper freebsd_committer 2019-04-20 11:40:03 UTC
I did this work before, but never committed the end-result. I’ll take care of this.
Comment 3 Li-Wen Hsu freebsd_committer 2019-04-20 15:57:17 UTC
Yes I have explicitly mark these tests need Python 2:

https://svnweb.freebsd.org/changeset/base/346431

This is more like a reminder as we need to convert these after all.
Comment 4 Enji Cooper freebsd_committer 2019-04-29 21:02:06 UTC
Opened a PR to require the net/py-dpkt package in order to run tests/sys/opencrypto/runtests , so I can make forward progress eliminating one of the reasons why tests/sys/opencrypto/runtests requires python 2.x.
Comment 5 commit-hook freebsd_committer 2019-05-10 00:04:11 UTC
A commit references this bug:

Author: ngie
Date: Fri May 10 00:03:33 UTC 2019
New revision: 347417
URL: https://svnweb.freebsd.org/changeset/base/347417

Log:
  Refactor tests/sys/opencrypto/runtests

  * Convert from plain to TAP for slightly improved introspection when skipping
    the tests due to requirements not being met.
  * Test for the net/py-dpkt (origin) package being required when running the
    tests, instead of relying on a copy of the dpkt.py module from 2014. This
    enables the tests to work with py3. Subsequently, remove
    `tests/sys/opencrypto/dpkt.py(c)?` via `make delete-old`.
  * Parameterize out `python2` as `$PYTHON`.

  PR:		237403
  MFC after:	1 week

Changes:
  head/ObsoleteFiles.inc
  head/tests/sys/opencrypto/Makefile
  head/tests/sys/opencrypto/dpkt.py
  head/tests/sys/opencrypto/runtests.sh
Comment 6 commit-hook freebsd_committer 2019-05-20 16:38:20 UTC
A commit references this bug:

Author: ngie
Date: Mon May 20 16:38:12 UTC 2019
New revision: 347996
URL: https://svnweb.freebsd.org/changeset/base/347996

Log:
  Replace uses of `foo.(de|en)code('hex')` with `binascii.(un)?hexlify(foo)`

  Python 3 no longer doesn't support encoding/decoding hexadecimal numbers using
  the `str.format` method. The backwards compatible new method (using the
  binascii module/methods) is a comparable means of converting to/from
  hexadecimal format.

  In short, the functional change is the following:
  * `foo.decode('hex')` -> `binascii.unhexlify(foo)`
  * `foo.encode('hex')` -> `binascii.hexlify(foo)`

  While here, move the dpkt import in `cryptodev.py` down per PEP8, so it comes
  after the standard library provided imports.

  PR:		237403
  MFC after:	1 week

Changes:
  head/tests/sys/opencrypto/cryptodev.py
  head/tests/sys/opencrypto/cryptotest.py
Comment 7 commit-hook freebsd_committer 2019-05-21 00:30:43 UTC
A commit references this bug:

Author: ngie
Date: Tue May 21 00:30:29 UTC 2019
New revision: 348024
URL: https://svnweb.freebsd.org/changeset/base/348024

Log:
  Followup to r347996

  Replace uses of `foo.encode("hex")` with `binascii.hexlify(foo)` for forwards
  compatibility between python 2.x and python 3.

  PR:		237403
  MFC after:	1 week

Changes:
  head/tests/sys/opencrypto/cryptotest.py
Comment 8 commit-hook freebsd_committer 2019-05-21 02:14:06 UTC
A commit references this bug:

Author: ngie
Date: Tue May 21 02:13:46 UTC 2019
New revision: 348031
URL: https://svnweb.freebsd.org/changeset/base/348031

Log:
  Squash deprecation warning related to array.array(..).tostring()

  In version 3.2+, `array.array(..).tostring()` was renamed to
  `array.array(..).tobytes()`. Conditionally call `array.array(..).tobytes()` if
  the python version is 3.2+.

  PR:		237403
  MFC after:	1 week

Changes:
  head/tests/sys/opencrypto/cryptodev.py
Comment 9 commit-hook freebsd_committer 2019-05-21 02:31:21 UTC
A commit references this bug:

Author: ngie
Date: Tue May 21 02:30:43 UTC 2019
New revision: 348032
URL: https://svnweb.freebsd.org/changeset/base/348032

Log:
  Fix `KAT(CCM)?Parser` file descriptor leaks

  Make `KAT(CCM)?Parser` into a context suite-capable object by implementing
  `__enter__` and `__exit__` methods which manage opening up the file descriptors
  and closing them on context exit. This implementation was decided over adding
  destructor logic to a `__del__` method, as there are a number of issues around
  object lifetimes when dealing with threading cleanup, atexit handlers, and a
  number of other less obvious edgecases. Plus, the architected solution is more
  pythonic and clean.

  Complete the iterator implementation by implementing a `__next__` method for
  both classes which handles iterating over the data using a generator pattern,
  and by changing `__iter__` to return the object instead of the data which it
  would iterate over. Alias the `__next__` method to `next` when working with
  python 2.x in order to maintain functional compatibility between the two major
  versions.

  As part of this work and to ensure readability, push the initialization of the
  parser objects up one layer and pass it down to a helper function. This could
  have been done via a decorator, but I was trying to keep it simple for other
  developers to make it easier to modify in the future.

  This fixes ResourceWarnings with python 3.

  PR:		237403
  MFC after:	1 week
  Tested with:	python 2.7.16 (amd64), python 3.6.8 (amd64)

Changes:
  head/tests/sys/opencrypto/cryptodev.py
  head/tests/sys/opencrypto/cryptotest.py
Comment 10 commit-hook freebsd_committer 2019-05-21 03:53:27 UTC
A commit references this bug:

Author: ngie
Date: Tue May 21 03:52:48 UTC 2019
New revision: 348042
URL: https://svnweb.freebsd.org/changeset/base/348042

Log:
  Fix encoding issues with python 3

  In python 3, the default encoding was switched from ascii character sets to
  unicode character sets in order to support internationalization by default.
  Some interfaces, like ioctls and packets, however, specify data in terms of
  non-unicode encodings formats, either in host endian (`fcntl.ioctl`) or
  network endian (`dpkt`) byte order/format.

  This change alters assumptions made by previous code where it was all
  data objects were assumed to be basestrings, when they should have been
  treated as byte arrays. In order to achieve this the following are done:
  * str objects with encodings needing to be encoded as ascii byte arrays are
    done so via `.encode("ascii")`. In order for this to work on python 3 in a
    type agnostic way (as it anecdotally varied depending on the caller), call
    `.encode("ascii")` only on str objects with python 3 to cast them to ascii
    byte arrays in a helper function name `str_to_ascii(..)`.
  * `dpkt.Packet` objects needing to be passed in to `fcntl.ioctl(..)` are done
    so by casting them to byte arrays via `bytes()`, which calls
    `dpkt.Packet__str__` under the covers and does the necessary str to byte array
    conversion needed for the `dpkt` APIs and `struct` module.

  In order to accomodate this change, apply the necessary typecasting for the
  byte array literal in order to search `fop.name` for nul bytes.

  This resolves all remaining python 2.x and python 3.x compatibility issues on
  amd64. More work needs to be done for the tests to function with i386, in
  general (this is a legacy issue).

  PR:		237403
  MFC after:	1 week
  Tested with:	python 2.7.16 (amd64), python 3.6.8 (amd64)

Changes:
  head/tests/sys/opencrypto/cryptodev.py
Comment 11 commit-hook freebsd_committer 2019-05-21 04:03:37 UTC
A commit references this bug:

Author: ngie
Date: Tue May 21 04:03:22 UTC 2019
New revision: 348045
URL: https://svnweb.freebsd.org/changeset/base/348045

Log:
  Follow up to r348042: cast `aad` to a byte array

  This is not completely necessary today, but this change is being made in a
  conservative manner to avoid accidental breakage in the future, if this ever
  was a unicode string.

  PR:		237403
  MFC after:	1 week

Changes:
  head/tests/sys/opencrypto/cryptodev.py
Comment 12 Enji Cooper freebsd_committer 2019-05-21 04:08:14 UTC
I've attached the only required changes needed to make this test use python 3 in svn.

In order for this to work however, the changes need to be MFCed to ^/stable/11 and ^/stable/12 first, and the default python version needs to be changed in the upstream freebsd-ci GitHub repo.

Given the current code freeze, I'm going to merge the changes back to ^/stable/12 and will handle the MFCs to ^/stable/11 once the freeze is over (I would need to back port other changes with respect to opencrypto, potentially).

Index: tests/sys/opencrypto/Makefile
===================================================================
--- tests/sys/opencrypto/Makefile       (.../head/tests/sys/opencrypto) (revision 348046)
+++ tests/sys/opencrypto/Makefile       (.../user/ngie/bug-237403/tests/sys/opencrypto) (working copy)
@@ -14,7 +14,7 @@
 
 TAP_TESTS_SH+= runtests
 
-TEST_METADATA.runtests+= required_programs="python2"
+TEST_METADATA.runtests+= required_programs="python"
 TEST_METADATA.runtests+= required_user="root"
 
 PYMODULES=     cryptodev.py cryptodevh.py cryptotest.py
Index: tests/sys/opencrypto/cryptodev.py
===================================================================
--- tests/sys/opencrypto/cryptodev.py   (.../head/tests/sys/opencrypto) (revision 348046)
+++ tests/sys/opencrypto/cryptodev.py   (.../user/ngie/bug-237403/tests/sys/opencrypto) (working copy)
@@ -1,8 +1,9 @@
-#!/usr/local/bin/python2
+#!/usr/bin/env python
 #
 # Copyright (c) 2014 The FreeBSD Foundation
 # Copyright 2014 John-Mark Gurney
 # All rights reserved.
+# Copyright 2019 Enji Cooper
 #
 # This software was developed by John-Mark Gurney under
 # the sponsorship from the FreeBSD Foundation.
Index: tests/sys/opencrypto/runtests.sh
===================================================================
--- tests/sys/opencrypto/runtests.sh    (.../head/tests/sys/opencrypto) (revision 348046)
+++ tests/sys/opencrypto/runtests.sh    (.../user/ngie/bug-237403/tests/sys/opencrypto) (working copy)
@@ -29,7 +29,7 @@
 # $FreeBSD$
 #
 
-: ${PYTHON=python2}
+: ${PYTHON=python}
 
 if [ ! -d /usr/local/share/nist-kat ]; then
        echo "1..0 # SKIP: nist-kat package not installed for test vectors"
Comment 13 commit-hook freebsd_committer 2019-05-23 01:26:12 UTC
A commit references this bug:

Author: ngie
Date: Thu May 23 01:25:35 UTC 2019
New revision: 348140
URL: https://svnweb.freebsd.org/changeset/base/348140

Log:
  MFC r346431,r347417,r348019:

  r346431 (by lwhsu):

  Specify using Python2, these .py files have not been converted to use Python3
  yet, but the default Python version in ports has been switched to 3.

  r347417:

  Refactor tests/sys/opencrypto/runtests

  * Convert from plain to TAP for slightly improved introspection when skipping
    the tests due to requirements not being met.
  * Test for the net/py-dpkt (origin) package being required when running the
    tests, instead of relying on a copy of the dpkt.py module from 2014. This
    enables the tests to work with py3. Subsequently, remove
    `tests/sys/opencrypto/dpkt.py(c)?` via `make delete-old`.
  * Parameterize out `python2` as `$PYTHON`.

  PR:		237403

  r348019:

  Allow the end-user to pass along arguments to cryptotest.py via `$CRYPTOTEST_ARGS`

  This allows someone to use `-v` to dump out standard output.

Changes:
_U  stable/12/
  stable/12/ObsoleteFiles.inc
  stable/12/tests/sys/opencrypto/Makefile
  stable/12/tests/sys/opencrypto/cryptodev.py
  stable/12/tests/sys/opencrypto/cryptotest.py
  stable/12/tests/sys/opencrypto/dpkt.py
  stable/12/tests/sys/opencrypto/runtests.sh
Comment 14 Li-Wen Hsu freebsd_committer 2019-05-27 18:30:16 UTC
(In reply to Enji Cooper from comment #12)
> In order for this to work however, the changes need to be MFCed to ^/stable/11 and ^/stable/12 first, and the default python version needs to be changed in the upstream freebsd-ci GitHub repo.

freebsd-ci uses the packages from official pkg.freebsd.org so it's 3.x already.
Comment 15 Ed Maste freebsd_committer 2019-08-12 19:32:31 UTC
What still needs to be done here?
Comment 16 Li-Wen Hsu freebsd_committer 2019-08-14 23:04:44 UTC
tests/sys/opencrypto/*.py and Makefile are still specifying using python2.

Also note that Python 2 is going to EoL on 2020-01-01.