You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Armand Grillet <ag...@mesosphere.io> on 2018/05/10 11:36:06 UTC
Review Request 67055: Added Python 3 version of cpplint.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
Bugs: MESOS-8770
https://issues.apache.org/jira/browse/MESOS-8770
Repository: mesos
Description
-------
The Python 3 supports scripts currently live in support/python3.
Once all support scripts will have been updated to Python 3 and
tested by the developers, we will replace the Python 2 scripts by
the new ones in an ongoing effort of modernizing our Python codebase.
Diffs
-----
support/python3/cpplint.patch PRE-CREATION
support/python3/cpplint.py PRE-CREATION
Diff: https://reviews.apache.org/r/67055/diff/1/
Testing
-------
Used `mesos-style.py` with the old `cpplint.py` (no change on `mesos-style.py`):
```
apache-mesos (cpplint) $ ./support/mesos-style.py
Checking 1289 C++ files
Total errors found: 0
Checking 4 JavaScript files
Total errors found: 0
Checking 47 Python files
py27-lint: commands succeeded
congratulations :)
py27-lint: commands succeeded
congratulations :)
Total errors found: 0
```
Updated `mesos-style.py` to use the new version of cpplint:
```
# We do not use a version of cpplint available through pip as
# we use a custom version (see cpplint.path) to lint C++ files.
process = subprocess.Popen(
['python3', 'support/python3/cpplint.py', rules_filter] + source_paths,
stderr=subprocess.PIPE,
close_fds=True)
```
Run `mesos-style.py` again:
```
apache-mesos (cpplint) $ ./support/mesos-style.py
Checking 1289 C++ files
Total errors found: 0
Checking 4 JavaScript files
Total errors found: 0
Checking 47 Python files
py27-lint: commands succeeded
congratulations :)
py27-lint: commands succeeded
congratulations :)
************* Module mesos-style
C:279, 0: Line too long (83/80) (line-too-long)
Total errors found: 1
```
As we can see, the number of errors found by cpplint is the same as before.
I have also used cpplint separately bu running `python3 support/python3/cpplint` on a few C++ files to see that it was running as expected.
Thanks,
Armand Grillet
Re: Review Request 67055: Updated cpplint to be compatible with
Python 3.
Posted by Eric Chung <ci...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/#review203357
-----------------------------------------------------------
Ship it!
Ship It!
- Eric Chung
On May 16, 2018, 2:29 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67055/
> -----------------------------------------------------------
>
> (Updated May 16, 2018, 2:29 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
>
>
> Bugs: MESOS-8770
> https://issues.apache.org/jira/browse/MESOS-8770
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch keeps the bundled cpplint to upstream '43d512b'.
> Cpplint now works with Python 2 and 3 by adding part of the
> content of https://github.com/google/styleguide/pull/349.
>
> After alias unicode to str and replace iteritems, itervalues, cpplint.py
> can work well using Python3 if the linted file hasn not any issues. If
> there is even only one violation of Google C++ Style Guide in the linted
> file, cpplint.py will need to output error message, then exceptions like
> "TypeError: write() argument must be str, not bytes" will appear.
>
> This is due to different supporting methods of Unicode between Python2
> and Python3. In Python2 `str` is a bytes sequence and `unicode` is used
> to handle Unicode strings but in Python3 `str` is a Unicode sequence
> and `unicode` does not exist. Type "bytes" is introduced to handle bytes
> sequences, thus Python3 `bytes` is equal to Python2 `str`.
>
> The type of argument of `sys.stderr.write()` is kept the same between
> Python2 and Python3 but the arguments of sys.stderr.write() are changed.
> It receives a bytes sequence in Python2 and a Unicode sequence in
> Python3. As a result, sys.stderr shouldn't be wrapped by a UTF-8 stream
> reader and writer of codecs when using Python3.
>
>
> Diffs
> -----
>
> support/cpplint.patch 69fbb5efd61962e8fef2c0188e181c789d16bae0
> support/cpplint.py 42a3dda2090a01950dcc2de7691cb3f247283e5e
>
>
> Diff: https://reviews.apache.org/r/67055/diff/4/
>
>
> Testing
> -------
>
> Used `mesos-style.py`:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> Updated `mesos-style.py` to use Python 3 with cpplint:
> ```
> process = subprocess.Popen(
> ['python3', 'support/cpplint.py', '--quiet',
> rules_filter] + source_paths,
> stderr=subprocess.PIPE,
> close_fds=True)
> ```
>
> Run `mesos-style.py` again:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> As we can see, the number of errors found by cpplint is the same as before.
>
> I have also used cpplint separately by running `python3 support/cpplint` on a few C++ files to see that it was running as expected.
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 67055: Updated cpplint to be compatible with
Python 3.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On May 16, 2018, 7:08 p.m., Eric Chung wrote:
> >
I don't believe that requiring additional dependencies for these very basic tools is a good idea, as it would add another hurdle to getting started with contributing to Mesos (the situation is completely different for tools configuring their own environment like e.g., the CLI where we have more freedom). The adaptions here are also pretty minimal, so they do not introduce a lot of additional maintenance.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/#review203248
-----------------------------------------------------------
On May 16, 2018, 4:29 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67055/
> -----------------------------------------------------------
>
> (Updated May 16, 2018, 4:29 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
>
>
> Bugs: MESOS-8770
> https://issues.apache.org/jira/browse/MESOS-8770
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch keeps the bundled cpplint to upstream '43d512b'.
> Cpplint now works with Python 2 and 3 by adding part of the
> content of https://github.com/google/styleguide/pull/349.
>
> After alias unicode to str and replace iteritems, itervalues, cpplint.py
> can work well using Python3 if the linted file hasn not any issues. If
> there is even only one violation of Google C++ Style Guide in the linted
> file, cpplint.py will need to output error message, then exceptions like
> "TypeError: write() argument must be str, not bytes" will appear.
>
> This is due to different supporting methods of Unicode between Python2
> and Python3. In Python2 `str` is a bytes sequence and `unicode` is used
> to handle Unicode strings but in Python3 `str` is a Unicode sequence
> and `unicode` does not exist. Type "bytes" is introduced to handle bytes
> sequences, thus Python3 `bytes` is equal to Python2 `str`.
>
> The type of argument of `sys.stderr.write()` is kept the same between
> Python2 and Python3 but the arguments of sys.stderr.write() are changed.
> It receives a bytes sequence in Python2 and a Unicode sequence in
> Python3. As a result, sys.stderr shouldn't be wrapped by a UTF-8 stream
> reader and writer of codecs when using Python3.
>
>
> Diffs
> -----
>
> support/cpplint.patch 69fbb5efd61962e8fef2c0188e181c789d16bae0
> support/cpplint.py 42a3dda2090a01950dcc2de7691cb3f247283e5e
>
>
> Diff: https://reviews.apache.org/r/67055/diff/4/
>
>
> Testing
> -------
>
> Used `mesos-style.py`:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> Updated `mesos-style.py` to use Python 3 with cpplint:
> ```
> process = subprocess.Popen(
> ['python3', 'support/cpplint.py', '--quiet',
> rules_filter] + source_paths,
> stderr=subprocess.PIPE,
> close_fds=True)
> ```
>
> Run `mesos-style.py` again:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> As we can see, the number of errors found by cpplint is the same as before.
>
> I have also used cpplint separately by running `python3 support/cpplint` on a few C++ files to see that it was running as expected.
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 67055: Updated cpplint to be compatible with
Python 3.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On May 16, 2018, 7:08 p.m., Eric Chung wrote:
> >
>
> Benjamin Bannier wrote:
> I don't believe that requiring additional dependencies for these very basic tools is a good idea, as it would add another hurdle to getting started with contributing to Mesos (the situation is completely different for tools configuring their own environment like e.g., the CLI where we have more freedom). The adaptions here are also pretty minimal, so they do not introduce a lot of additional maintenance.
>
> Eric Chung wrote:
> there won't be any new dependencies as six is already included in pylint:
> ```
> (linters) uber@echung3:~/mesos/support$ pip-compile pip-requirements.txt
> No handlers could be found for logger "pip._vendor.cachecontrol.controller"
> #
> # This file is autogenerated by pip-compile
> # To update, run:
> #
> # pip-compile --output-file pip-requirements.txt pip-requirements.txt
> #
> --index-url https://yoober7:ood5geishoothue8peT5ieCoCa9ieNef@pypi.uberinternal.com/index
>
> astroid==1.4.9 # via pylint
> backports.functools-lru-cache==1.5 # via pylint
> configparser==3.5.0 # via pylint
> futures==3.2.0 # via isort
> isort==4.3.4 # via pylint
> lazy-object-proxy==1.3.1 # via astroid
> mccabe==0.6.1 # via pylint
> nodeenv==1.1.2
> pylint==1.6.4
> six==1.11.0 # via astroid, pylint
> wrapt==1.10.11 # via astroid
> ```
`mesos-style.py` which calls `cpplint.py` is typically run from git post-commit and pre-rewrite hooks and is unrelated to anything some virtualenv might install.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/#review203248
-----------------------------------------------------------
On May 16, 2018, 4:29 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67055/
> -----------------------------------------------------------
>
> (Updated May 16, 2018, 4:29 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
>
>
> Bugs: MESOS-8770
> https://issues.apache.org/jira/browse/MESOS-8770
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch keeps the bundled cpplint to upstream '43d512b'.
> Cpplint now works with Python 2 and 3 by adding part of the
> content of https://github.com/google/styleguide/pull/349.
>
> After alias unicode to str and replace iteritems, itervalues, cpplint.py
> can work well using Python3 if the linted file hasn not any issues. If
> there is even only one violation of Google C++ Style Guide in the linted
> file, cpplint.py will need to output error message, then exceptions like
> "TypeError: write() argument must be str, not bytes" will appear.
>
> This is due to different supporting methods of Unicode between Python2
> and Python3. In Python2 `str` is a bytes sequence and `unicode` is used
> to handle Unicode strings but in Python3 `str` is a Unicode sequence
> and `unicode` does not exist. Type "bytes" is introduced to handle bytes
> sequences, thus Python3 `bytes` is equal to Python2 `str`.
>
> The type of argument of `sys.stderr.write()` is kept the same between
> Python2 and Python3 but the arguments of sys.stderr.write() are changed.
> It receives a bytes sequence in Python2 and a Unicode sequence in
> Python3. As a result, sys.stderr shouldn't be wrapped by a UTF-8 stream
> reader and writer of codecs when using Python3.
>
>
> Diffs
> -----
>
> support/cpplint.patch 69fbb5efd61962e8fef2c0188e181c789d16bae0
> support/cpplint.py 42a3dda2090a01950dcc2de7691cb3f247283e5e
>
>
> Diff: https://reviews.apache.org/r/67055/diff/4/
>
>
> Testing
> -------
>
> Used `mesos-style.py`:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> Updated `mesos-style.py` to use Python 3 with cpplint:
> ```
> process = subprocess.Popen(
> ['python3', 'support/cpplint.py', '--quiet',
> rules_filter] + source_paths,
> stderr=subprocess.PIPE,
> close_fds=True)
> ```
>
> Run `mesos-style.py` again:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> As we can see, the number of errors found by cpplint is the same as before.
>
> I have also used cpplint separately by running `python3 support/cpplint` on a few C++ files to see that it was running as expected.
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 67055: Updated cpplint to be compatible with
Python 3.
Posted by Eric Chung <ci...@gmail.com>.
> On May 16, 2018, 5:08 p.m., Eric Chung wrote:
> >
>
> Benjamin Bannier wrote:
> I don't believe that requiring additional dependencies for these very basic tools is a good idea, as it would add another hurdle to getting started with contributing to Mesos (the situation is completely different for tools configuring their own environment like e.g., the CLI where we have more freedom). The adaptions here are also pretty minimal, so they do not introduce a lot of additional maintenance.
there won't be any new dependencies as six is already included in pylint:
```
(linters) uber@echung3:~/mesos/support$ pip-compile pip-requirements.txt
No handlers could be found for logger "pip._vendor.cachecontrol.controller"
#
# This file is autogenerated by pip-compile
# To update, run:
#
# pip-compile --output-file pip-requirements.txt pip-requirements.txt
#
--index-url https://yoober7:ood5geishoothue8peT5ieCoCa9ieNef@pypi.uberinternal.com/index
astroid==1.4.9 # via pylint
backports.functools-lru-cache==1.5 # via pylint
configparser==3.5.0 # via pylint
futures==3.2.0 # via isort
isort==4.3.4 # via pylint
lazy-object-proxy==1.3.1 # via astroid
mccabe==0.6.1 # via pylint
nodeenv==1.1.2
pylint==1.6.4
six==1.11.0 # via astroid, pylint
wrapt==1.10.11 # via astroid
```
- Eric
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/#review203248
-----------------------------------------------------------
On May 16, 2018, 2:29 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67055/
> -----------------------------------------------------------
>
> (Updated May 16, 2018, 2:29 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
>
>
> Bugs: MESOS-8770
> https://issues.apache.org/jira/browse/MESOS-8770
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch keeps the bundled cpplint to upstream '43d512b'.
> Cpplint now works with Python 2 and 3 by adding part of the
> content of https://github.com/google/styleguide/pull/349.
>
> After alias unicode to str and replace iteritems, itervalues, cpplint.py
> can work well using Python3 if the linted file hasn not any issues. If
> there is even only one violation of Google C++ Style Guide in the linted
> file, cpplint.py will need to output error message, then exceptions like
> "TypeError: write() argument must be str, not bytes" will appear.
>
> This is due to different supporting methods of Unicode between Python2
> and Python3. In Python2 `str` is a bytes sequence and `unicode` is used
> to handle Unicode strings but in Python3 `str` is a Unicode sequence
> and `unicode` does not exist. Type "bytes" is introduced to handle bytes
> sequences, thus Python3 `bytes` is equal to Python2 `str`.
>
> The type of argument of `sys.stderr.write()` is kept the same between
> Python2 and Python3 but the arguments of sys.stderr.write() are changed.
> It receives a bytes sequence in Python2 and a Unicode sequence in
> Python3. As a result, sys.stderr shouldn't be wrapped by a UTF-8 stream
> reader and writer of codecs when using Python3.
>
>
> Diffs
> -----
>
> support/cpplint.patch 69fbb5efd61962e8fef2c0188e181c789d16bae0
> support/cpplint.py 42a3dda2090a01950dcc2de7691cb3f247283e5e
>
>
> Diff: https://reviews.apache.org/r/67055/diff/4/
>
>
> Testing
> -------
>
> Used `mesos-style.py`:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> Updated `mesos-style.py` to use Python 3 with cpplint:
> ```
> process = subprocess.Popen(
> ['python3', 'support/cpplint.py', '--quiet',
> rules_filter] + source_paths,
> stderr=subprocess.PIPE,
> close_fds=True)
> ```
>
> Run `mesos-style.py` again:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> As we can see, the number of errors found by cpplint is the same as before.
>
> I have also used cpplint separately by running `python3 support/cpplint` on a few C++ files to see that it was running as expected.
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 67055: Updated cpplint to be compatible with
Python 3.
Posted by Eric Chung <ci...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/#review203248
-----------------------------------------------------------
support/cpplint.py
Line 63 (original), 63 (patched)
<https://reviews.apache.org/r/67055/#comment285345>
python six (https://pythonhosted.org/six/) provides a lot of py2/3 compatibility tools, including a boolean flag to check at run time for the python version. can we use that instead? this will make the intention much more explicit.
support/cpplint.py
Line 963 (original), 981 (patched)
<https://reviews.apache.org/r/67055/#comment285348>
this can be replaced with `six.iteritems`
support/cpplint.py
Line 4632 (original), 4650 (patched)
<https://reviews.apache.org/r/67055/#comment285349>
this can be replaced with `six.itervalues`
- Eric Chung
On May 16, 2018, 2:29 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67055/
> -----------------------------------------------------------
>
> (Updated May 16, 2018, 2:29 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
>
>
> Bugs: MESOS-8770
> https://issues.apache.org/jira/browse/MESOS-8770
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch keeps the bundled cpplint to upstream '43d512b'.
> Cpplint now works with Python 2 and 3 by adding part of the
> content of https://github.com/google/styleguide/pull/349.
>
> After alias unicode to str and replace iteritems, itervalues, cpplint.py
> can work well using Python3 if the linted file hasn not any issues. If
> there is even only one violation of Google C++ Style Guide in the linted
> file, cpplint.py will need to output error message, then exceptions like
> "TypeError: write() argument must be str, not bytes" will appear.
>
> This is due to different supporting methods of Unicode between Python2
> and Python3. In Python2 `str` is a bytes sequence and `unicode` is used
> to handle Unicode strings but in Python3 `str` is a Unicode sequence
> and `unicode` does not exist. Type "bytes" is introduced to handle bytes
> sequences, thus Python3 `bytes` is equal to Python2 `str`.
>
> The type of argument of `sys.stderr.write()` is kept the same between
> Python2 and Python3 but the arguments of sys.stderr.write() are changed.
> It receives a bytes sequence in Python2 and a Unicode sequence in
> Python3. As a result, sys.stderr shouldn't be wrapped by a UTF-8 stream
> reader and writer of codecs when using Python3.
>
>
> Diffs
> -----
>
> support/cpplint.patch 69fbb5efd61962e8fef2c0188e181c789d16bae0
> support/cpplint.py 42a3dda2090a01950dcc2de7691cb3f247283e5e
>
>
> Diff: https://reviews.apache.org/r/67055/diff/4/
>
>
> Testing
> -------
>
> Used `mesos-style.py`:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> Updated `mesos-style.py` to use Python 3 with cpplint:
> ```
> process = subprocess.Popen(
> ['python3', 'support/cpplint.py', '--quiet',
> rules_filter] + source_paths,
> stderr=subprocess.PIPE,
> close_fds=True)
> ```
>
> Run `mesos-style.py` again:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> As we can see, the number of errors found by cpplint is the same as before.
>
> I have also used cpplint separately by running `python3 support/cpplint` on a few C++ files to see that it was running as expected.
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 67055: Updated cpplint to be compatible with
Python 3.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/#review203326
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Bannier
On May 16, 2018, 4:29 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67055/
> -----------------------------------------------------------
>
> (Updated May 16, 2018, 4:29 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
>
>
> Bugs: MESOS-8770
> https://issues.apache.org/jira/browse/MESOS-8770
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch keeps the bundled cpplint to upstream '43d512b'.
> Cpplint now works with Python 2 and 3 by adding part of the
> content of https://github.com/google/styleguide/pull/349.
>
> After alias unicode to str and replace iteritems, itervalues, cpplint.py
> can work well using Python3 if the linted file hasn not any issues. If
> there is even only one violation of Google C++ Style Guide in the linted
> file, cpplint.py will need to output error message, then exceptions like
> "TypeError: write() argument must be str, not bytes" will appear.
>
> This is due to different supporting methods of Unicode between Python2
> and Python3. In Python2 `str` is a bytes sequence and `unicode` is used
> to handle Unicode strings but in Python3 `str` is a Unicode sequence
> and `unicode` does not exist. Type "bytes" is introduced to handle bytes
> sequences, thus Python3 `bytes` is equal to Python2 `str`.
>
> The type of argument of `sys.stderr.write()` is kept the same between
> Python2 and Python3 but the arguments of sys.stderr.write() are changed.
> It receives a bytes sequence in Python2 and a Unicode sequence in
> Python3. As a result, sys.stderr shouldn't be wrapped by a UTF-8 stream
> reader and writer of codecs when using Python3.
>
>
> Diffs
> -----
>
> support/cpplint.patch 69fbb5efd61962e8fef2c0188e181c789d16bae0
> support/cpplint.py 42a3dda2090a01950dcc2de7691cb3f247283e5e
>
>
> Diff: https://reviews.apache.org/r/67055/diff/4/
>
>
> Testing
> -------
>
> Used `mesos-style.py`:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> Updated `mesos-style.py` to use Python 3 with cpplint:
> ```
> process = subprocess.Popen(
> ['python3', 'support/cpplint.py', '--quiet',
> rules_filter] + source_paths,
> stderr=subprocess.PIPE,
> close_fds=True)
> ```
>
> Run `mesos-style.py` again:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> As we can see, the number of errors found by cpplint is the same as before.
>
> I have also used cpplint separately by running `python3 support/cpplint` on a few C++ files to see that it was running as expected.
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 67055: Updated cpplint to be compatible with
Python 3.
Posted by Armand Grillet <ag...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/
-----------------------------------------------------------
(Updated May 16, 2018, 2:29 p.m.)
Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
Changes
-------
Rebased.
Bugs: MESOS-8770
https://issues.apache.org/jira/browse/MESOS-8770
Repository: mesos
Description (updated)
-------
This patch keeps the bundled cpplint to upstream '43d512b'.
Cpplint now works with Python 2 and 3 by adding part of the
content of https://github.com/google/styleguide/pull/349.
After alias unicode to str and replace iteritems, itervalues, cpplint.py
can work well using Python3 if the linted file hasn not any issues. If
there is even only one violation of Google C++ Style Guide in the linted
file, cpplint.py will need to output error message, then exceptions like
"TypeError: write() argument must be str, not bytes" will appear.
This is due to different supporting methods of Unicode between Python2
and Python3. In Python2 `str` is a bytes sequence and `unicode` is used
to handle Unicode strings but in Python3 `str` is a Unicode sequence
and `unicode` does not exist. Type "bytes" is introduced to handle bytes
sequences, thus Python3 `bytes` is equal to Python2 `str`.
The type of argument of `sys.stderr.write()` is kept the same between
Python2 and Python3 but the arguments of sys.stderr.write() are changed.
It receives a bytes sequence in Python2 and a Unicode sequence in
Python3. As a result, sys.stderr shouldn't be wrapped by a UTF-8 stream
reader and writer of codecs when using Python3.
Diffs (updated)
-----
support/cpplint.patch 69fbb5efd61962e8fef2c0188e181c789d16bae0
support/cpplint.py 42a3dda2090a01950dcc2de7691cb3f247283e5e
Diff: https://reviews.apache.org/r/67055/diff/4/
Changes: https://reviews.apache.org/r/67055/diff/3-4/
Testing
-------
Used `mesos-style.py`:
```
apache-mesos (cpplint) $ ./support/mesos-style.py
Checking 1289 C++ files
Total errors found: 0
Checking 4 JavaScript files
Total errors found: 0
Checking 47 Python files
py27-lint: commands succeeded
congratulations :)
py27-lint: commands succeeded
congratulations :)
Total errors found: 0
```
Updated `mesos-style.py` to use Python 3 with cpplint:
```
process = subprocess.Popen(
['python3', 'support/cpplint.py', '--quiet',
rules_filter] + source_paths,
stderr=subprocess.PIPE,
close_fds=True)
```
Run `mesos-style.py` again:
```
apache-mesos (cpplint) $ ./support/mesos-style.py
Checking 1289 C++ files
Total errors found: 0
Checking 4 JavaScript files
Total errors found: 0
Checking 47 Python files
py27-lint: commands succeeded
congratulations :)
py27-lint: commands succeeded
congratulations :)
Total errors found: 0
```
As we can see, the number of errors found by cpplint is the same as before.
I have also used cpplint separately by running `python3 support/cpplint` on a few C++ files to see that it was running as expected.
Thanks,
Armand Grillet
Re: Review Request 67055: Updated cpplint to be compatible with
Python 3.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/#review203182
-----------------------------------------------------------
Ship it!
Nice work patching cpplint to be single-source compatible without the future module!
- Andrew Schwartzmeyer
On May 14, 2018, 3:22 a.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67055/
> -----------------------------------------------------------
>
> (Updated May 14, 2018, 3:22 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
>
>
> Bugs: MESOS-8770
> https://issues.apache.org/jira/browse/MESOS-8770
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Uses the version distributed by Google on Apr 15, 2018.
> The diff includes the same patches as before.
> Works with Python 2 and 3.
>
>
> Diffs
> -----
>
> support/cpplint.patch 1b4d4ee62ccd40523fe9e8d6d476e1ee82e415bf
> support/cpplint.py 1e5d96ff2be4b9e80848e11eb0a8f4b20b31c340
> support/mesos-style.py 07074daa245ab503cf551201ccadeac8cc10206d
>
>
> Diff: https://reviews.apache.org/r/67055/diff/3/
>
>
> Testing
> -------
>
> Used `mesos-style.py`:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> Updated `mesos-style.py` to use Python 3 with cpplint:
> ```
> process = subprocess.Popen(
> ['python3', 'support/cpplint.py', '--quiet',
> rules_filter] + source_paths,
> stderr=subprocess.PIPE,
> close_fds=True)
> ```
>
> Run `mesos-style.py` again:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> As we can see, the number of errors found by cpplint is the same as before.
>
> I have also used cpplint separately by running `python3 support/cpplint` on a few C++ files to see that it was running as expected.
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 67055: Updated cpplint to be compatible with
Python 3.
Posted by Armand Grillet <ag...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/
-----------------------------------------------------------
(Updated May 14, 2018, 10:22 a.m.)
Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
Changes
-------
Updated to most recent official version of cpplint + small patch for Python 3 while keeping Python 2 compatibility.
Summary (updated)
-----------------
Updated cpplint to be compatible with Python 3.
Bugs: MESOS-8770
https://issues.apache.org/jira/browse/MESOS-8770
Repository: mesos
Description (updated)
-------
Uses the version distributed by Google on Apr 15, 2018.
The diff includes the same patches as before.
Works with Python 2 and 3.
Diffs (updated)
-----
support/cpplint.patch 1b4d4ee62ccd40523fe9e8d6d476e1ee82e415bf
support/cpplint.py 1e5d96ff2be4b9e80848e11eb0a8f4b20b31c340
support/mesos-style.py 07074daa245ab503cf551201ccadeac8cc10206d
Diff: https://reviews.apache.org/r/67055/diff/3/
Changes: https://reviews.apache.org/r/67055/diff/2-3/
Testing (updated)
-------
Used `mesos-style.py`:
```
apache-mesos (cpplint) $ ./support/mesos-style.py
Checking 1289 C++ files
Total errors found: 0
Checking 4 JavaScript files
Total errors found: 0
Checking 47 Python files
py27-lint: commands succeeded
congratulations :)
py27-lint: commands succeeded
congratulations :)
Total errors found: 0
```
Updated `mesos-style.py` to use the new version of cpplint:
```
process = subprocess.Popen(
['python3', 'support/cpplint.py', '--quiet',
rules_filter] + source_paths,
stderr=subprocess.PIPE,
close_fds=True)
```
Run `mesos-style.py` again:
```
apache-mesos (cpplint) $ ./support/mesos-style.py
Checking 1289 C++ files
Total errors found: 0
Checking 4 JavaScript files
Total errors found: 0
Checking 47 Python files
py27-lint: commands succeeded
congratulations :)
py27-lint: commands succeeded
congratulations :)
Total errors found: 0
```
As we can see, the number of errors found by cpplint is the same as before.
I have also used cpplint separately by running `python3 support/cpplint` on a few C++ files to see that it was running as expected.
Thanks,
Armand Grillet
Re: Review Request 67055: Updated cpplint to be compatible with
Python 3.
Posted by Armand Grillet <ag...@mesosphere.io>.
> On May 14, 2018, 8:53 a.m., Benjamin Bannier wrote:
> > Since the diff contains a lot of new noise, at least I don't understand why this is needed. There should be something in the commit message explaining it.
> >
> > As always, I'd prefer to not fork this script absent a very strong reason. I believe the upstream script should work fine with python2 or python3, let's make any new changes to a shared script if possible.
Thanks for the review, the updated version should solve the issues raised.
- Armand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/#review202991
-----------------------------------------------------------
On May 14, 2018, 10:22 a.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67055/
> -----------------------------------------------------------
>
> (Updated May 14, 2018, 10:22 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
>
>
> Bugs: MESOS-8770
> https://issues.apache.org/jira/browse/MESOS-8770
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Uses the version distributed by Google on Apr 15, 2018.
> The diff includes the same patches as before.
> Works with Python 2 and 3.
>
>
> Diffs
> -----
>
> support/cpplint.patch 1b4d4ee62ccd40523fe9e8d6d476e1ee82e415bf
> support/cpplint.py 1e5d96ff2be4b9e80848e11eb0a8f4b20b31c340
> support/mesos-style.py 07074daa245ab503cf551201ccadeac8cc10206d
>
>
> Diff: https://reviews.apache.org/r/67055/diff/3/
>
>
> Testing
> -------
>
> Used `mesos-style.py`:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> Updated `mesos-style.py` to use Python 3 with cpplint:
> ```
> process = subprocess.Popen(
> ['python3', 'support/cpplint.py', '--quiet',
> rules_filter] + source_paths,
> stderr=subprocess.PIPE,
> close_fds=True)
> ```
>
> Run `mesos-style.py` again:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> As we can see, the number of errors found by cpplint is the same as before.
>
> I have also used cpplint separately by running `python3 support/cpplint` on a few C++ files to see that it was running as expected.
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 67055: Added Python 3 version of cpplint.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/#review202991
-----------------------------------------------------------
Since the diff contains a lot of new noise, at least I don't understand why this is needed. There should be something in the commit message explaining it.
As always, I'd prefer to not fork this script absent a very strong reason. I believe the upstream script should work fine with python2 or python3, let's make any new changes to a shared script if possible.
- Benjamin Bannier
On May 11, 2018, 11:49 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67055/
> -----------------------------------------------------------
>
> (Updated May 11, 2018, 11:49 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
>
>
> Bugs: MESOS-8770
> https://issues.apache.org/jira/browse/MESOS-8770
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Python 3 supports scripts currently live in support/python3.
> Once all support scripts will have been updated to Python 3 and
> tested by the developers, we will replace the Python 2 scripts by
> the new ones in an ongoing effort of modernizing our Python codebase.
>
>
> Diffs
> -----
>
> support/python3/cpplint.patch PRE-CREATION
> support/python3/cpplint.py PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67055/diff/2/
>
>
> Testing
> -------
>
> Used `mesos-style.py` with the old `cpplint.py` (no change on `mesos-style.py`):
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> Updated `mesos-style.py` to use the new version of cpplint:
> ```
> # We do not use a version of cpplint available through pip as
> # we use a custom version (see cpplint.path) to lint C++ files.
> process = subprocess.Popen(
> ['python3', 'support/python3/cpplint.py', rules_filter] + source_paths,
> stderr=subprocess.PIPE,
> close_fds=True)
> ```
>
> Run `mesos-style.py` again:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> ************* Module mesos-style
> C:279, 0: Line too long (83/80) (line-too-long)
> Total errors found: 1
> ```
> As we can see, the number of errors found by cpplint is the same as before.
>
> I have also used cpplint separately bu running `python3 support/python3/cpplint` on a few C++ files to see that it was running as expected.
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 67055: Added Python 3 version of cpplint.
Posted by Armand Grillet <ag...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/
-----------------------------------------------------------
(Updated mai 11, 2018, 9:49 après-midi)
Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
Changes
-------
Changed Python version.
Bugs: MESOS-8770
https://issues.apache.org/jira/browse/MESOS-8770
Repository: mesos
Description
-------
The Python 3 supports scripts currently live in support/python3.
Once all support scripts will have been updated to Python 3 and
tested by the developers, we will replace the Python 2 scripts by
the new ones in an ongoing effort of modernizing our Python codebase.
Diffs (updated)
-----
support/python3/cpplint.patch PRE-CREATION
support/python3/cpplint.py PRE-CREATION
Diff: https://reviews.apache.org/r/67055/diff/2/
Changes: https://reviews.apache.org/r/67055/diff/1-2/
Testing
-------
Used `mesos-style.py` with the old `cpplint.py` (no change on `mesos-style.py`):
```
apache-mesos (cpplint) $ ./support/mesos-style.py
Checking 1289 C++ files
Total errors found: 0
Checking 4 JavaScript files
Total errors found: 0
Checking 47 Python files
py27-lint: commands succeeded
congratulations :)
py27-lint: commands succeeded
congratulations :)
Total errors found: 0
```
Updated `mesos-style.py` to use the new version of cpplint:
```
# We do not use a version of cpplint available through pip as
# we use a custom version (see cpplint.path) to lint C++ files.
process = subprocess.Popen(
['python3', 'support/python3/cpplint.py', rules_filter] + source_paths,
stderr=subprocess.PIPE,
close_fds=True)
```
Run `mesos-style.py` again:
```
apache-mesos (cpplint) $ ./support/mesos-style.py
Checking 1289 C++ files
Total errors found: 0
Checking 4 JavaScript files
Total errors found: 0
Checking 47 Python files
py27-lint: commands succeeded
congratulations :)
py27-lint: commands succeeded
congratulations :)
************* Module mesos-style
C:279, 0: Line too long (83/80) (line-too-long)
Total errors found: 1
```
As we can see, the number of errors found by cpplint is the same as before.
I have also used cpplint separately bu running `python3 support/python3/cpplint` on a few C++ files to see that it was running as expected.
Thanks,
Armand Grillet
Re: Review Request 67055: Added Python 3 version of cpplint.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67055/#review202842
-----------------------------------------------------------
PASS: Mesos patch 67055 was successfully built and tested.
Reviews applied: `['67055']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67055
- Mesos Reviewbot Windows
On May 10, 2018, 11:36 a.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67055/
> -----------------------------------------------------------
>
> (Updated May 10, 2018, 11:36 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin Klues.
>
>
> Bugs: MESOS-8770
> https://issues.apache.org/jira/browse/MESOS-8770
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Python 3 supports scripts currently live in support/python3.
> Once all support scripts will have been updated to Python 3 and
> tested by the developers, we will replace the Python 2 scripts by
> the new ones in an ongoing effort of modernizing our Python codebase.
>
>
> Diffs
> -----
>
> support/python3/cpplint.patch PRE-CREATION
> support/python3/cpplint.py PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67055/diff/1/
>
>
> Testing
> -------
>
> Used `mesos-style.py` with the old `cpplint.py` (no change on `mesos-style.py`):
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> Total errors found: 0
> ```
> Updated `mesos-style.py` to use the new version of cpplint:
> ```
> # We do not use a version of cpplint available through pip as
> # we use a custom version (see cpplint.path) to lint C++ files.
> process = subprocess.Popen(
> ['python3', 'support/python3/cpplint.py', rules_filter] + source_paths,
> stderr=subprocess.PIPE,
> close_fds=True)
> ```
>
> Run `mesos-style.py` again:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
> py27-lint: commands succeeded
> congratulations :)
> py27-lint: commands succeeded
> congratulations :)
> ************* Module mesos-style
> C:279, 0: Line too long (83/80) (line-too-long)
> Total errors found: 1
> ```
> As we can see, the number of errors found by cpplint is the same as before.
>
> I have also used cpplint separately bu running `python3 support/python3/cpplint` on a few C++ files to see that it was running as expected.
>
>
> Thanks,
>
> Armand Grillet
>
>