mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Review Request 66964: Ported (some) Python 2 support scripts to Python 3.
Date Fri, 04 May 2018 20:44:43 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66964/
-----------------------------------------------------------

Review request for mesos, Armand Grillet, Benjamin Mahler, Eric Chung, Clement Michaud, Gaston
Kleiman, Joseph Wu, and Kevin Klues.


Bugs: MESOS-7658
    https://issues.apache.org/jira/browse/MESOS-7658


Repository: mesos


Description
-------

According to https://docs.python.org/3/howto/pyporting.html, we can
make our support scripts compatible with both Python 2 and 3 provided
we only worry about supporting Python 2.7.

Using the Python-Future quick start guide
http://python-future.org/quickstart.html, I installed a Python 3.6.5
environment, ran `pip install future`, and then ran `futurize -w` on
the modified scripts.

In my unchanged system environment, attempting to use the scripts
resulted in a traceback with the error `ImportError: No module named
future`, since the `future` package is sadly not installed by default.
On Ubuntu 17.10, I was able to `apt install python-future` to add it
to my system's Python 2 installation, so would be a new requirement
despite the Python 2 support.

The ported scripts were chosen in order to support using Python 3 on
the Windows build bot, so that author names with non-ASCII characters
can be used. The rest of the scripts should follow, but are not
necessary, and require more extensive testing and fixes.

A manual change was made to `post-reviews` to deal with the stdout of
child processes. In `execute` we explicitly decode the stdout to a
string to match the existing expectation (in Python 2, it was already
a string, but in Python 3 bytes and strings are different types). When
printing the stdout of a child process, we use `sys.stdout.buffer` to
print bytes instead of a string in order to preserve ANSI color
escapes in the output. This also fixes an existing bug on Windows
where the script would not correctly display colors.


Diffs
-----

  support/apply-reviews.py b6801b926d9fc32172f102586d49e0464da71359 
  support/mesos-split.py 7ef5c86d55e61b80b5a7c8f137323aa056f272ed 
  support/post-reviews.py a6646f2eca45f911aad23333403b8b0fef3a9323 
  support/push-commits.py cd751cbf3c7e64005f1da6c96eb33c6fc62c7ae3 
  support/verify-reviews.py fbc2460051e96761c9669db001c2fa13906f0cca 


Diff: https://reviews.apache.org/r/66964/diff/1/


Testing
-------

This is a work-in-progress. I used the automatic porting script `futurize` on some of our
support scripts (I tried on all of them, but some (like `cpplint.py`) are going to take more
work). So I'd prefer to start with the set of scripts necessary to change the Windows ReviewBot
over to Python 3. Also, `mesos-style.py` _does not_ like these changes; it throws a bunch
of errors and warnings when linting these files:

```
W: 39, 0: Redefining built-in 'str' (redefined-builtin)
E: 37, 0: Unable to import 'future' (import-error)
E: 39, 0: Unable to import 'builtins' (import-error)
C: 39, 0: Import "from builtins import str" should be placed at the top of the module (wrong-import-position)
C: 40, 0: Import "import atexit" should be placed at the top of the module (wrong-import-position)
C: 41, 0: Import "import json" should be placed at the top of the module (wrong-import-position)
C: 42, 0: Import "import os" should be placed at the top of the module (wrong-import-position)
C: 43, 0: Import "import platform" should be placed at the top of the module (wrong-import-position)
C: 44, 0: Import "import subprocess" should be placed at the top of the module (wrong-import-position)
C: 45, 0: Import "import sys" should be placed at the top of the module (wrong-import-position)
E: 46, 0: No name 'request' in module 'urllib' (no-name-in-module)
E: 46, 0: Unable to import 'urllib.request' (import-error)
C: 46, 0: Import "import urllib.request" should be placed at the top of the module (wrong-import-position)
E: 47, 0: No name 'parse' in module 'urllib' (no-name-in-module)
E: 47, 0: Unable to import 'urllib.parse' (import-error)
C: 47, 0: Import "import urllib.parse" should be placed at the top of the module (wrong-import-position)
E: 48, 0: No name 'error' in module 'urllib' (no-name-in-module)
E: 48, 0: Unable to import 'urllib.error' (import-error)
C: 48, 0: Import "import urllib.error" should be placed at the top of the module (wrong-import-position)
C: 50, 0: Import "from datetime import datetime" should be placed at the top of the module
(wrong-import-position)
```

It doesn't seem to handle the Python 2/3 compatibility layer.

Please help test these scripts, I'm testing by hand but we don't have any unit test coverage.

For Windows, installing `future` is easiest with `python -m pip install future`, and for Linux,
the `python-future` package.

So far I tested:

`post-reviews.py` on Linux (for this patch) with Python 2 and 3
`apply-reviews.py` on Windows with Python 2 and 3
`mesos-split.py` on Linux with Python 2 and 3
`push-commits.py` (only with -n) on Linux with Python 2 (fails with 3, fix pending)

I'll have to manually test verify-reviews with Python 3; hoping the bots test it with Python
2 using this review.


Thanks,

Andrew Schwartzmeyer


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message