You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2018/05/04 20:44:43 UTC
Review Request 66964: Ported (some) Python 2 support scripts to
Python 3.
-----------------------------------------------------------
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
Re: Review Request 66964: Ported (some) Python 2 support scripts to
Python 3.
Posted by Eric Chung <ci...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66964/#review202728
-----------------------------------------------------------
Mostly lgtm, but it would be even better if we could also put in some unit tests, e.g. using tox.
- Eric Chung
On May 4, 2018, 8: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, 8: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
>
>
Re: Review Request 66964: Ported (some) Python 2 support scripts to
Python 3.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66964/#review202513
-----------------------------------------------------------
Patch looks great!
Reviews applied: [66964]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
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
>
>
Re: Review Request 66964: Ported (some) Python 2 support scripts to
Python 3.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> 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
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...
- 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
>
>
Re: Review Request 66964: Ported (some) Python 2 support scripts to
Python 3.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> 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
>
>
Re: Review Request 66964: Ported (some) Python 2 support scripts to
Python 3.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66964/#review202497
-----------------------------------------------------------
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
- Mesos Reviewbot Windows
On May 4, 2018, 8: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, 8: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
>
>