mesos-reviews mailing list archives

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


> On May 4, 2018, 3:44 p.m., Mesos Reviewbot Windows wrote:
> > PASS: Mesos patch 66964 was successfully built and tested.
> > 
> > Reviews applied: `['66964']`
> > 
> > All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66964
> 
> Andrew Schwartzmeyer wrote:
>     Grr. It looks like the review bot uses an out-of-tree script instead of `verify-reviews.py`:
>     
>     ```
>     Successfully executed: python.exe C:\Jenkins\workspace\mesos-reviewbot-testing\Mesos\utils\get-review-ids.py
-r 66964 -o C:\Users\mesos\AppData\Local\Temp\mesos_dependent_review_ids
>     ```
>     
>     They live [here](https://github.com/Microsoft/mesos-jenkins/tree/master/Mesos/utils).
I'll ask Ionut to start upstreaming these...

[Issue #66](https://github.com/Microsoft/mesos-jenkins/issues/66).


- Andrew


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


On May 4, 2018, 1:44 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66964/
> -----------------------------------------------------------
> 
> (Updated May 4, 2018, 1:44 p.m.)
> 
> 
> 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