fix Python code style v3

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Ricardo" == Ricardo Martincoski <[email protected]> writes:

Fix these warnings:
E201 whitespace after '['
E202 whitespace before ']'
E501 line too long (138 > 132 characters)

to make the long lines shorter and more readable.

Signed-off-by Ricardo Martincoski <[email protected]>

Cc Arnout Vandecappelle <[email protected]>

Cc Thomas Petazzoni <[email protected]>

---
Changes v2 -> v3:
- no changes

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Ricardo" == Ricardo Martincoski <[email protected]> writes:

Fix these warnings:
E129 visually indented line with same indent as next logical line E302 expected 2 blank lines, found 1

Signed-off-by Ricardo Martincoski <[email protected]>

Cc Thomas De Schampheleire <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Yann E. MORIN <[email protected]>

Reviewed-by "Yann E. MORIN" <[email protected]>

---
Changes v2 -> v3:
- manually accepted review tag

Fix these warnings:
E122 continuation line missing indentation or outdented E301 expected 1 blank line, found 0
E302 expected 2 blank lines, found 1
F401 'os' imported but unused

Signed-off-by Ricardo Martincoski <[email protected]>

Cc Yegor Yefremov <[email protected]>

---
Changes v2 -> v3:
- no changes

Changes v1 -> v2:
- new patch (v1 of this series was not sent yet during the review of
http://patchwork.ozlabs.org/patch/811011/ so I asked to fix all in the same series.

https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/57046960
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/57046961

---
 .../tests/package/test_python_cryptography.py      | 23 ++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

support/testing/tests/package/test_python_cryptography.py +13 -11

index b60152d..78c3ef5 100644
@@ -1,7 +1,6 @@
-import os
-
 from tests.package.test_python import TestPythonBase
 
+
 class TestPythonCryptography(TestPythonBase):
     def fernet_test(self, timeout=-1):
         cmd = self.interpreter + " -c 'from cryptography.fernet import Fernet;"

@@ -10,22 +9,26 @@ class TestPythonCryptography(TestPythonBase):
         _, exit_code = self.emulator.run(cmd, timeout)
         self.assertEqual(exit_code, 0)
 
+
 class TestPythonPy2Cryptography(TestPythonCryptography):
     config = TestPythonBase.config + \
-"""
-BR2_PACKAGE_PYTHON=y
-BR2_PACKAGE_PYTHON_CRYPTOGRAPHY=y
-"""
+        """
+        BR2_PACKAGE_PYTHON=y
+        BR2_PACKAGE_PYTHON_CRYPTOGRAPHY=y
+        """
+
     def test_run(self):
         self.login()
         self.fernet_test(40)
 
+
 class TestPythonPy3Cryptography(TestPythonCryptography):
     config = TestPythonBase.config + \
-"""
-BR2_PACKAGE_PYTHON3=y
-BR2_PACKAGE_PYTHON_CRYPTOGRAPHY=y
-"""
+        """
+        BR2_PACKAGE_PYTHON3=y
+        BR2_PACKAGE_PYTHON_CRYPTOGRAPHY=y
+        """
+
     def test_run(self):
         self.login()
         self.fernet_test(40)
--

2.7.4

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Ricardo" == Ricardo Martincoski <[email protected]> writes:

Fix these warnings:
E122 continuation line missing indentation or outdented E301 expected 1 blank line, found 0
E302 expected 2 blank lines, found 1
F401 'os' imported but unused

Signed-off-by Ricardo Martincoski <[email protected]>

Cc Yegor Yefremov <[email protected]>

---
Changes v2 -> v3:
- no changes

Changes v1 -> v2:
- new patch (v1 of this series was not sent yet during the review of
http://patchwork.ozlabs.org/patch/811011/ so I asked to fix all in the same series.

Fix these warnings:
E401 multiple imports on one line

Signed-off-by Ricardo Martincoski <[email protected]>

Cc Yegor Yefremov <[email protected]>

---
Changes v2 -> v3:
- no changes

Changes v1 -> v2:
- new patch.

---
 utils/scanpypi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

utils/scanpypi +3 -2

index b9006ee..505ba0a 100755
@@ -10,7 +10,9 @@ from __future__ import print_function
 from __future__ import absolute_import
 import argparse
 import json
-import six.moves.urllib.request, six.moves.urllib.error, six.moves.urllib.parse
+import six.moves.urllib.request
+import six.moves.urllib.error
+import six.moves.urllib.parse
 import sys
 import os
 import shutil
--

2.7.4

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Ricardo" == Ricardo Martincoski <[email protected]> writes:

Fix these warnings:
E401 multiple imports on one line

Signed-off-by Ricardo Martincoski <[email protected]>

Cc Yegor Yefremov <[email protected]>

---
Changes v2 -> v3:
- no changes

Changes v1 -> v2:
- new patch.

Use the latest version of the tool because it is actively maintained. But use a fixed version of the tool and its dependencies to get stable results. It can be manually bumped from time to time.

Before installing any Python packages, ensure pip, setuptools, and wheel are up to date as recommended in the docs [1].

[1] https://packaging.python.org/tutorials/installing-packages/

Signed-off-by Ricardo Martincoski <[email protected]>

Cc Arnout Vandecappelle <[email protected]>

Cc Peter Korsgaard <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Yann E. MORIN <[email protected]>

---
Changes v2 -> v3: (suggested by Yann E. MORIN)
- minimise the number of intermediate layers;
- explain why install setuptools separately using the latest version (I actually just used the exact command line from the docs and referenced it in the commit log);
- use a single package on each line, sorted.

Changes v1 -> v2: (suggested by Yann E. MORIN)
- install flake8 to the base docker image instead of adding sudo to install tools on the fly.

---
 support/docker/Dockerfile | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

support/docker/Dockerfile +10 -1

index ce3fdd9..f01ac25 100644
@@ -27,10 +27,19 @@ RUN dpkg --add-architecture i386 && \
         cvs bzr git mercurial subversion wget \
         cpio unzip \
         libncurses5-dev \
-        python-nose2 python-pexpect qemu-system-arm qemu-system-x86 && \
+        python-nose2 python-pexpect qemu-system-arm qemu-system-x86 \
+        python-pip && \
     apt-get -y autoremove && \
     apt-get -y clean
 
+# For check-flake8
+RUN python -m pip install --upgrade pip setuptools wheel && \
+    pip install -q \
+        flake8==3.5.0 \
+        mccabe==0.6.1 \
+        pycodestyle==2.3.1 \
+        pyflakes==1.6.0
+
 # To be able to generate a toolchain with locales, enable one UTF-8 locale
 RUN sed -i 's/# \(en_US.UTF-8\)/\1/' /etc/locale.gen && \
     /usr/sbin/locale-gen

Signature · 1 line

2.7.4

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Ricardo" == Ricardo Martincoski <[email protected]> writes:

Use the latest version of the tool because it is actively maintained. But use a fixed version of the tool and its dependencies to get stable results. It can be manually bumped from time to time.

Before installing any Python packages, ensure pip, setuptools, and wheel are up to date as recommended in the docs [1].

[1] https://packaging.python.org/tutorials/installing-packages/

Signed-off-by Ricardo Martincoski <[email protected]>

Cc Arnout Vandecappelle <[email protected]>

Cc Peter Korsgaard <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Yann E. MORIN <[email protected]>

---
Changes v2 -> v3: (suggested by Yann E. MORIN)
- minimise the number of intermediate layers;
- explain why install setuptools separately using the latest version (I actually just used the exact command line from the docs and referenced it in the commit log);
- use a single package on each line, sorted.

Changes v1 -> v2: (suggested by Yann E. MORIN)
- install flake8 to the base docker image instead of adding sudo to install tools on the fly.

Use the latest version of the tool because it is actively maintained. But use a fixed version of the tool and its dependencies to get stable results. It can be manually bumped from time to time.

support/docker/Dockerfile +6 -0

+    pip install -q \
+        flake8==3.5.0 \
+        mccabe==0.6.1 \
+        pycodestyle==2.3.1 \
+        pyflakes==1.6.0
+
 # To be able to generate a toolchain with locales, enable one UTF-8 locale
 RUN sed -i 's/# \(en_US.UTF-8\)/\1/' /etc/locale.gen && \
     /usr/sbin/locale-gen

Signature · 1 line

2.7.4

On 2018-03-13 00:09 -0300, Ricardo Martincoski spake thusly:

Quoted 3 lines

Use the latest version of the tool because it is actively maintained. But use a fixed version of the tool and its dependencies to get stable results. It can be manually bumped from time to time.

[--SNIP--]

support/docker/Dockerfile RUN dpkg --add-architecture i386 && \ · line 36

Quoted 2 lines

support/docker/Dockerfile +2 -0

+# For check-flake8
+RUN python -m pip install --upgrade pip setuptools wheel && \

So, back when you submnitted this patch, I ACKed it. But I missed that this command really means we can't reproduce the docker file, because it will use versions of pip, setuptools, and wheel that are current at the time the command is run.

I.e. today I could very well get something that is different from the version we got back at 20180205.0730 when we last built the docker image.

This is not really nice... :-(

Do you think we could get away without running this command at all, and just run the pip-install one, below, since we force the version of the modules we isntall?

First of all: I don't think we need to hurry to fix it in the master branch. The release is so close. Unless of course we need to generate a new docker image right now for some reason.
Do you disagree?

Quoted 3 lines

Do you think we could get away without running this command at all, and just run the pip-install one, below, since we force the version of the modules we isntall?

No. But certainly we can come up with some command that generates reproducible images.

By trying to install packages without that line we would get errors [1] and [2]. The only version that we don't *need* to upgrade is pip. apt-get installs 9.0.1, and the pip --upgrade installed version 9.0.2 at the time the current image was generated.
An image generated today would have pip 10.0.1, demonstrating the problem you found in the current Dockerfile.

Using 'pip freeze' and 'pip list' inside the current image we can dump all the current package versions [3].
Since the list of packages to install will become longer we could even move it to a separate file, as suggested in [4] and also in the example (but not the text) from the 'Best practices' guide which url you shared a while ago [5].

So a way to generate an image equivalent to the current one would be:

@ support/docker/Dockerfile:
# For check-flake8
COPY requirements.txt /tmp/

RUN pip install -q \
        pip==9.0.2 \
        setuptools==39.0.1 \
        wheel==0.30.0 && \
    pip install -q -r /tmp/requirements.txt

@ support/docker/requirements.txt
bzr==2.8.0.dev1
configobj==5.0.6
configparser==3.5.0
enum34==1.1.6
flake8==3.5.0
mccabe==0.6.1
mercurial==4.0
nose2==0.6.5
pexpect==4.2.1
ptyprocess==0.5.1
pycodestyle==2.3.1
pyflakes==1.6.0
six==1.10.0

And maybe a comment at the first line of the new file would be nice:

# output from 'pip freeze'

What do you think about this approach?

Are you preparing a patch?

Why do we even use pip to install those? Can't we just rely on the verions actually packaged in the distro instead? I.e.

apt-get install python-configobj python-configparser etc...

Of course, provided that the module is indeed packaged in the distro... As far as I can see, there should be everything we need, no?

The only two for which I have a doubt are bzr (python-bzrlib) and mercurial (python-hglib); all the others are in stretch-20171210.

But if we apt-get install python-flake8, then it should bring all its dependencies I guess:

Note that I don't care if stretch packages a different version than what we curently have; I'd prefer that we use the distro-packaged versions, since we know that apt-get *is* reproducible as we use a fixed version of the distro.

Quoted 1 line

Are you preparing a patch?

When we agree on the directions, yes I can.

Quoted 2 lines

Why do we even use pip to install those? Can't we just rely on the verions actually packaged in the distro instead? I.e.

Quoted 2 lines

Quoted 1 line

Are you preparing a patch?

When we agree on the directions, yes I can.

Reproducible images with less code and less maintenance effort in exchange of using a not-so-bleeding-edge version. Makes sense to me.

I am only in doubt how future-proof is this solution, if some day we will *need* a new version, i.e. to something related to Python3 in 2020 (it's only a wild guess!, I don't know any current limitation, I don't know the internals of flake8).
But then probably debian will have a newer version of flake8, so we can update the stretch version. And we can always go back to using pip if really needed.

Add a test to check Python code style in the whole buildroot tree.

Search files by type in order to help flake8 to find the Python scripts without .py extension. But don't rely only in the output of 'file' as it uses heuristics and sometimes it is wrong (specially identifying Python files as C++ source for the 'file' version currently in the Docker image).

Include in the output:
- the list of Python files processed;
- statistics for each kind of warning;
- the total number of warnings;
- the number of Python files processed.

Signed-off-by Ricardo Martincoski <[email protected]>

Cc Arnout Vandecappelle <[email protected]>

Cc Yann E. MORIN <[email protected]>

Reviewed-by "Yann E. MORIN" <[email protected]>

---
BEFORE APPLYING, see this note from Yann:
Note: this can only be applied _after_ we regenerate the docker image. Then we'll have to update the version of the image at the top of the gitlab-ci.yml file.

Changes v2 -> v3:
- manually accepted review tag

Changes v1 -> v2: (suggested by Yann E. MORIN)
- install flake8 to the base docker image (in the previous patch)

https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/57046920

---
 .gitlab-ci.yml    | 11 +++++++++++
 .gitlab-ci.yml.in | 11 +++++++++++
 2 files changed, 22 insertions(+)

.gitlab-ci.yml +11 -0

index f44c3a9..9ad4c5f 100644
@@ -30,6 +30,17 @@ check-DEVELOPERS:
     script:
         - "! utils/get-developers | grep -v 'No action specified'"
 
+check-flake8:
+    before_script:
+        # Help flake8 to find the Python files without .py extension.
+        - find * -type f -name '*.py' > files.txt
+        - find * -type f -print0 | xargs -0 file | grep 'Python script' | cut -d':' -f1 >> files.txt
+        - sort -u files.txt | tee files.processed
+    script:
+        - python -m flake8 --statistics --count $(cat files.processed)
+    after_script:
+        - wc -l files.processed
+
 check-package:
     script:
         - find . -type f \( -name '*.mk' -o -name '*.hash' \) -exec ./utils/check-package {} +

.gitlab-ci.yml.in +11 -0

index cb3eb71..95fc025 100644
@@ -30,6 +30,17 @@ check-DEVELOPERS:
     script:
         - "! utils/get-developers | grep -v 'No action specified'"
 
+check-flake8:
+    before_script:
+        # Help flake8 to find the Python files without .py extension.
+        - find * -type f -name '*.py' > files.txt
+        - find * -type f -print0 | xargs -0 file | grep 'Python script' | cut -d':' -f1 >> files.txt
+        - sort -u files.txt | tee files.processed
+    script:
+        - python -m flake8 --statistics --count $(cat files.processed)
+    after_script:
+        - wc -l files.processed
+
 check-package:
     script:
         - find . -type f \( -name '*.mk' -o -name '*.hash' \) -exec ./utils/check-package {} +

Signature · 1 line

2.7.4

On 2018-03-13 00:09 -0300, Ricardo Martincoski spake thusly:

Quoted 5 lines

---
BEFORE APPLYING, see this note from Yann:
Note: this can only be applied _after_ we regenerate the docker image. Then we'll have to update the version of the image at the top of the gitlab-ci.yml file.

Indeed, so I've marked this patch as "deferred" in patchwork, so it does not get accidentally applied.

Quoted 7 lines

On 2018-03-13 00:09 -0300, Ricardo Martincoski spake thusly:

Quoted 5 lines

---
BEFORE APPLYING, see this note from Yann:
Note: this can only be applied _after_ we regenerate the docker image. Then we'll have to update the version of the image at the top of the gitlab-ci.yml file.

Quoted 2 lines

Indeed, so I've marked this patch as "deferred" in patchwork, so it does not get accidentally applied.

Could you regenerate the image, please?
I don't have the docker push permission.

Quoted 8 lines

On 2018-03-13 00:09 -0300, Ricardo Martincoski spake thusly:

Quoted 5 lines

---
BEFORE APPLYING, see this note from Yann:
Note: this can only be applied _after_ we regenerate the docker image. Then we'll have to update the version of the image at the top of the gitlab-ci.yml file.

Indeed, so I've marked this patch as "deferred" in patchwork, so it does not get accidentally applied.

Could you regenerate the image, please?
I don't have the docker push permission.

Not sure if you were aware of it, but this has been done a little while ago now. Tag 20180318.1724 is available:
https://hub.docker.com/r/buildroot/base/tags/

Add a test to check Python code style in the whole buildroot tree.

Search files by type in order to help flake8 to find the Python scripts without .py extension. But don't rely only in the output of 'file' as it uses heuristics and sometimes it is wrong (specially identifying Python files as C++ source for the 'file' version currently in the Docker image).

Include in the output:
- the list of Python files processed;
- statistics for each kind of warning;
- the total number of warnings;
- the number of Python files processed.

Signed-off-by Ricardo Martincoski <[email protected]>

Cc Arnout Vandecappelle <[email protected]>

Cc Yann E. MORIN <[email protected]>

Reviewed-by "Yann E. MORIN" <[email protected]>

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Ricardo" == Ricardo Martincoski <[email protected]> writes:

This script comes from the kernel source, so ignore any code style warnings for it in order to keep it as close as possible to the original one, making synchronization between repos easier.

The option --exclude for flake8/pycodestyle is an absolute list and has a default, so ideally the default values should be added too. But the use cases for flake8 in the tree are:
- when developing a new script or changing an existing one, the developer calls flake8 only on that script;
- in the GitLab job, a list of all Python files to be tested is created and then passed to flake8.
None of these involve calling 'flake8' without parameters, so don't care about adding the default value.

Signed-off-by Ricardo Martincoski <[email protected]>

Cc Marcus Folkesson <[email protected]>

Cc Thomas Petazzoni <[email protected]>

---
Changes v2 -> v3:
- no changes

Changes v1 -> v2:
- instead of tweaking even more the script from the kernel source, ignore any warnings for it; (suggested by Thomas Petazzoni)
- move the patch after the one adding the GitLab job so I can reference it in the commit message.