[Buildroot] [PATCH 0/4] support/docker: make the image more reproducible

[Buildroot] [PATCH 0/4] support/docker: make the image more reproducible

[Yann E. MORIN]

Hello All!

Currently, the base test image is not entirely reproducible, because we
install packages from PyPI without specifying all the versions of all
the packages we install from there.

Instead of relying on a third-party to provide packages, just use those
from te distribution, because we are using a pinned version and thus we
will always get the same packages from the distro.


Regards,
Yann E. MORIN.


The following changes since commit 6a29c8ae3ee2f2ef1a7bdac48fa96d29d9e9b62f

  librsync: bump to version 2.0.2 (2018-06-02 16:06:02 +0200)


are available in the git repository at:

  git://git.buildroot.org/~ymorin/git/buildroot.git

for you to fetch changes up to ff32083db8641c38fa6d02f731870583b24c4dab

  support/docker: use the distro-provided flake8 (2018-06-02 23:27:15 +0200)


----------------------------------------------------------------
Yann E. MORIN (4):
      support/docker: run apt-get update and apt-get install in two RUNs
      support/docker: sort the list of installed packages
      support/testing: fix python syntax
      support/docker: use the distro-provided flake8

 support/docker/Dockerfile         | 40 +++++++++++++++++++++++----------------
 support/testing/infra/__init__.py |  6 +++---
 support/testing/infra/basetest.py |  4 ++--
 3 files changed, 29 insertions(+), 21 deletions(-)

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/4] support/docker: run apt-get update and apt-get install in two RUNs

[Yann E. MORIN]

In commit 7517aef4d (support/docker: limit the number of layers),
we reduced the number of layers by coalescing multiple RUN commands
into less commands.

In doing so, we especially coalesced "apt-get update" with "apt-get
install".

However, the distribution we used is a pinned version of stretch, so
we know that running apt-get update will always yield the same apt
database.

If we split the two apt-get commands, then we can re-use any local
intermediate image when we need to update the list of packages to
install; this helps quite a bit when testing the docker files over
and over again, with just slight variants in the packages list.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Peter Korsgaard <peter@korsgaard.com>
---
 support/docker/Dockerfile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/support/docker/Dockerfile b/support/docker/Dockerfile
index f01ac25f93..8c525f7cf1 100644
--- a/support/docker/Dockerfile
+++ b/support/docker/Dockerfile
@@ -20,8 +20,8 @@ COPY apt-sources.list /etc/apt/sources.list
 
 # The container has no package lists, so need to update first
 RUN dpkg --add-architecture i386 && \
-    apt-get update -y && \
-    apt-get install -y --no-install-recommends \
+    apt-get update -y
+RUN apt-get install -y --no-install-recommends \
         build-essential cmake libc6:i386 g++-multilib \
         bc ca-certificates file locales rsync \
         cvs bzr git mercurial subversion wget \
-- 
2.14.1

[Buildroot] [PATCH 1/4] support/docker: run apt-get update and apt-get install in two RUNs

[Ricardo Martincoski]

Hello,

On Sat, Jun 02, 2018 at 07:19 PM, Yann E. MORIN wrote:

> In commit 7517aef4d (support/docker: limit the number of layers),
> we reduced the number of layers by coalescing multiple RUN commands
> into less commands.
> 
> In doing so, we especially coalesced "apt-get update" with "apt-get
> install".

That is exactly what is recommended in the 'best practices':
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#run
"Always combine RUN apt-get update with apt-get install in the same RUN
statement."

But the 'gotchas' used to justify the recommendation don't apply in our case,
as you describe below.

> However, the distribution we used is a pinned version of stretch, so
> we know that running apt-get update will always yield the same apt
> database.
> 
> If we split the two apt-get commands, then we can re-use any local
> intermediate image when we need to update the list of packages to
> install; this helps quite a bit when testing the docker files over
> and over again, with just slight variants in the packages list.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

[I use docker only with the buildroot image, so my experience is limited, but
 for what it's worth]
Reviewed-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>


Regards,
Ricardo

[Buildroot] [PATCH 2/4] support/docker: sort the list of installed packages

[Yann E. MORIN]

As suggested in the docker best practices [0], order the package list
alphabetically, and list only one package per line.

This will be much usefull later, we need to update the list of installed
packages, like adding new ones for example.

[0] https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#sort-multi-line-arguments

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
 support/docker/Dockerfile | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/support/docker/Dockerfile b/support/docker/Dockerfile
index 8c525f7cf1..4f08a54f06 100644
--- a/support/docker/Dockerfile
+++ b/support/docker/Dockerfile
@@ -22,13 +22,28 @@ COPY apt-sources.list /etc/apt/sources.list
 RUN dpkg --add-architecture i386 && \
     apt-get update -y
 RUN apt-get install -y --no-install-recommends \
-        build-essential cmake libc6:i386 g++-multilib \
-        bc ca-certificates file locales rsync \
-        cvs bzr git mercurial subversion wget \
-        cpio unzip \
+        bc \
+        build-essential \
+        bzr \
+        ca-certificates \
+        cpio \
+        cvs \
+        file \
+        g++-multilib \
+        git \
+        libc6:i386 \
         libncurses5-dev \
-        python-nose2 python-pexpect qemu-system-arm qemu-system-x86 \
-        python-pip && \
+        locales \
+        mercurial \
+        python-nose2 \
+        python-pexpect \
+        qemu-system-arm \
+        qemu-system-x86 \
+        rsync \
+        subversion \
+        unzip \
+        wget \
+        && \
     apt-get -y autoremove && \
     apt-get -y clean
 
-- 
2.14.1

[Buildroot] [PATCH 2/4] support/docker: sort the list of installed packages

[Ricardo Martincoski]

Hello,

On Sat, Jun 02, 2018 at 07:19 PM, Yann E. MORIN wrote:

[snip]
>  RUN apt-get install -y --no-install-recommends \
> -        build-essential cmake libc6:i386 g++-multilib \
> -        bc ca-certificates file locales rsync \
> -        cvs bzr git mercurial subversion wget \
> -        cpio unzip \
> +        bc \
> +        build-essential \
> +        bzr \
> +        ca-certificates \
> +        cpio \
> +        cvs \
> +        file \
> +        g++-multilib \
> +        git \
> +        libc6:i386 \
>          libncurses5-dev \
> -        python-nose2 python-pexpect qemu-system-arm qemu-system-x86 \
> -        python-pip && \

With only patches 1 and 2 applied the 'docker build' command will fail:
/usr/bin/python: No module named pip

This removal should be part of patch 4.

> +        locales \
> +        mercurial \
> +        python-nose2 \
> +        python-pexpect \
> +        qemu-system-arm \
> +        qemu-system-x86 \
> +        rsync \
> +        subversion \
> +        unzip \
> +        wget \
> +        && \

We usually don't put && in a separate line.
I am not really against it if no one else complains.


Regards,
Ricardo

[Buildroot] [PATCH 2/4] support/docker: sort the list of installed packages

[Yann E. MORIN]

Ricardo, All,

On 2018-06-03 01:21 -0300, Ricardo Martincoski spake thusly:
> On Sat, Jun 02, 2018 at 07:19 PM, Yann E. MORIN wrote:
> >  RUN apt-get install -y --no-install-recommends \
> > -        build-essential cmake libc6:i386 g++-multilib \
> > -        bc ca-certificates file locales rsync \
> > -        cvs bzr git mercurial subversion wget \
> > -        cpio unzip \
> > +        bc \
> > +        build-essential \
> > +        bzr \
> > +        ca-certificates \
> > +        cpio \
> > +        cvs \
> > +        file \
> > +        g++-multilib \
> > +        git \
> > +        libc6:i386 \
> >          libncurses5-dev \
> > -        python-nose2 python-pexpect qemu-system-arm qemu-system-x86 \
> > -        python-pip && \
> 
> With only patches 1 and 2 applied the 'docker build' command will fail:
> /usr/bin/python: No module named pip

Oooh dang... I missed that one when doing the interactive git-add...

> This removal should be part of patch 4.

Obviously so, yes.

> > +        locales \
> > +        mercurial \
> > +        python-nose2 \
> > +        python-pexpect \
> > +        qemu-system-arm \
> > +        qemu-system-x86 \
> > +        rsync \
> > +        subversion \
> > +        unzip \
> > +        wget \
> > +        && \
> 
> We usually don't put && in a separate line.
> I am not really against it if no one else complains.

Well, I'm not too sure either. We have two coflicting rules here:

  - one package per-line, so that we can add/remove packages without
    touching the existing lines, so if we were top add a package after
    the last one, we'd have to touch two lines,

  - we almost exclusively put the && at the end of a line...

Note that the docker best practices put the && at the beginning of the
line when they need to split lines.

So, I don't care what rule we apply (as I prefer neither, as I favour
the one in the docker best practices).

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 3/4] support/testing: fix python syntax

[Yann E. MORIN]

We're about to switch to using the flake8 from the distro, and that one
uncovers two more issues that the previously used pip-provided one did
not catch:

  - 'print' is now a function,
  - exceptions need to be caught-assigned with the 'as' keyword,

Additionally, old-style "%s"%() formatting is deprecated.

Fix those three issues.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
 support/testing/infra/__init__.py | 6 +++---
 support/testing/infra/basetest.py | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/support/testing/infra/__init__.py b/support/testing/infra/__init__.py
index b03e891771..a9352a652b 100644
--- a/support/testing/infra/__init__.py
+++ b/support/testing/infra/__init__.py
@@ -34,17 +34,17 @@ def download(dldir, filename):
         os.makedirs(dldir)
 
     tmpfile = tempfile.mktemp(dir=dldir)
-    print "Downloading to {}".format(tmpfile)
+    print("Downloading to {}".format(tmpfile))
 
     try:
         url_fh = urlopen(os.path.join(ARTIFACTS_URL, filename))
         with open(tmpfile, "w+") as tmpfile_fh:
             tmpfile_fh.write(url_fh.read())
-    except (HTTPError, URLError), err:
+    except (HTTPError, URLError) as err:
         os.unlink(tmpfile)
         raise err
 
-    print "Renaming from %s to %s" % (tmpfile, finalpath)
+    print("Renaming from {0} to {1}".format(tmpfile, finalpath))
     os.rename(tmpfile, finalpath)
     return finalpath
 
diff --git a/support/testing/infra/basetest.py b/support/testing/infra/basetest.py
index f3f13ad97f..5014fefafa 100644
--- a/support/testing/infra/basetest.py
+++ b/support/testing/infra/basetest.py
@@ -46,8 +46,8 @@ class BRTest(unittest.TestCase):
         self.config += "\nBR2_JLEVEL={}\n".format(self.jlevel)
 
     def show_msg(self, msg):
-        print "{} {:40s} {}".format(datetime.datetime.now().strftime("%H:%M:%S"),
-                                    self.testname, msg)
+        print("{} {:40s} {}".format(datetime.datetime.now().strftime("%H:%M:%S"),
+                                    self.testname, msg))
 
     def setUp(self):
         self.show_msg("Starting")
-- 
2.14.1

[Buildroot] [PATCH 3/4] support/testing: fix python syntax

[Ricardo Martincoski]

Hello,

On Sat, Jun 02, 2018 at 07:19 PM, Yann E. MORIN wrote:

> We're about to switch to using the flake8 from the distro, and that one
> uncovers two more issues that the previously used pip-provided one did
> not catch:

See my comments to patch 4. What generates more warnings is the fact that we
start using Python3 interpreter to call flake8.

We can call this patch a step towards Python 3 support for the test infra.

[snip]
> ---
>  support/testing/infra/__init__.py | 6 +++---
>  support/testing/infra/basetest.py | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)

By running one of below commands (after the others from .gitlab-ci.yml) I get:
$ python3 -m flake8 --statistics --count $(cat files.processed)
$ flake8 --statistics --count $(cat files.processed)

support/testing/infra/basetest.py:49:28: E999 SyntaxError: invalid syntax
support/testing/infra/__init__.py:37:29: E999 SyntaxError: invalid syntax
support/testing/run-tests:44:29: E999 SyntaxError: invalid syntax
3     E999 SyntaxError: invalid syntax
3

So it seems support/testing/run-tests also needs to be changed.


Regards,
Ricardo

[Buildroot] [PATCH 3/4] support/testing: fix python syntax

[Yann E. MORIN]

Ricardo, All,

On 2018-06-03 01:24 -0300, Ricardo Martincoski spake thusly:
> On Sat, Jun 02, 2018 at 07:19 PM, Yann E. MORIN wrote:
> > We're about to switch to using the flake8 from the distro, and that one
> > uncovers two more issues that the previously used pip-provided one did
> > not catch:
> 
> See my comments to patch 4. What generates more warnings is the fact that we
> start using Python3 interpreter to call flake8.

Oh, so the interpreter for flake8 has an impact onhow it interprets the
code it scans. Weird...

> We can call this patch a step towards Python 3 support for the test infra.

Yeah, I like this idea! ;-]

> [snip]
> > ---
> >  support/testing/infra/__init__.py | 6 +++---
> >  support/testing/infra/basetest.py | 4 ++--
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> By running one of below commands (after the others from .gitlab-ci.yml) I get:
> $ python3 -m flake8 --statistics --count $(cat files.processed)
> $ flake8 --statistics --count $(cat files.processed)
> 
> support/testing/infra/basetest.py:49:28: E999 SyntaxError: invalid syntax
> support/testing/infra/__init__.py:37:29: E999 SyntaxError: invalid syntax
> support/testing/run-tests:44:29: E999 SyntaxError: invalid syntax

Weird, I didn't have that last one here in the docker image... But
indeed, another case of print-is-a-function-now

> 3     E999 SyntaxError: invalid syntax
> 3
> 
> So it seems support/testing/run-tests also needs to be changed.

I'll do so when I respin the series. Thanks for the reviews! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 4/4] support/docker: use the distro-provided flake8

[Yann E. MORIN]

Currently, we install flake8 and its dependencies via pip. We
tried to be reproducible by pinning the version of those python
packages, but we did forget quite a few of them, and thus some
dependencies for flake8 are installed as uncontrolled versions.

Furthermore, before we install flake8 and its dependencies, we
forcibly update pip, setuptools, and wheels packages to their
latest versions. This explicitly breaks reproducibility.

While we could enforce a specific version of all those packages
and still grab them from PyPI, we can simply grab them from the
distribution-provided packages instead.

Since we're using a pinned versiomn of stretch, this already
guarantees we'll reproducibily get the same versions over and
over again. Besides, we just need to list flake8 as a package to
install to automatically get all its dependencies (again, in a
reproducible way).

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
 support/docker/Dockerfile | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/support/docker/Dockerfile b/support/docker/Dockerfile
index 4f08a54f06..57c9ef78fa 100644
--- a/support/docker/Dockerfile
+++ b/support/docker/Dockerfile
@@ -29,6 +29,7 @@ RUN apt-get install -y --no-install-recommends \
         cpio \
         cvs \
         file \
+        flake8 \
         g++-multilib \
         git \
         libc6:i386 \
@@ -47,14 +48,6 @@ RUN apt-get install -y --no-install-recommends \
     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
-- 
2.14.1

[Buildroot] [PATCH 4/4] support/docker: use the distro-provided flake8

[Ricardo Martincoski]

Hello,

+ Thomas DS, who first mentioned flake8 in the mailing list.

On Sat, Jun 02, 2018 at 07:19 PM, Yann E. MORIN wrote:

> Currently, we install flake8 and its dependencies via pip. We
> tried to be reproducible by pinning the version of those python
> packages, but we did forget quite a few of them, and thus some
> dependencies for flake8 are installed as uncontrolled versions.
> 
> Furthermore, before we install flake8 and its dependencies, we
> forcibly update pip, setuptools, and wheels packages to their
> latest versions. This explicitly breaks reproducibility.

Sorry about this mess, created in:
"14aa15a5a5 support/dockerfile: install flake8"

> 
> While we could enforce a specific version of all those packages
> and still grab them from PyPI, we can simply grab them from the
> distribution-provided packages instead.
> 
> Since we're using a pinned versiomn of stretch, this already

s/versiomn/version/

> guarantees we'll reproducibily get the same versions over and

s/reproducibily/reproducibly/

> over again. Besides, we just need to list flake8 as a package to
> install to automatically get all its dependencies (again, in a
> reproducible way).

IMO it would be good to explicitly state here that this patch actually
downgrades flake8.
http://lists.busybox.net/pipermail/buildroot/2018-May/222376.html

Again, in this trade-off, using this simple approach makes sense to me because:
 - we get reproducible docker images with simple and easily maintainable code
   in the dockerfile;
 - I am not aware of any serious limitation of this old version, the release
   notes for a version in the between mentions 'Dramatically improve the
   performance' but we have a limited number of scripts and running on Gitlab
   for all of them still takes less than 5 minutes;
 - we will eventually upgrade flake8 version again when we bump stretch version
   and it contains a new version of flake8.

> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> ---
>  support/docker/Dockerfile | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/support/docker/Dockerfile b/support/docker/Dockerfile
> index 4f08a54f06..57c9ef78fa 100644
> --- a/support/docker/Dockerfile
> +++ b/support/docker/Dockerfile
> @@ -29,6 +29,7 @@ RUN apt-get install -y --no-install-recommends \
>          cpio \
>          cvs \
>          file \
> +        flake8 \

This package installs the Python3 version.
python-flake8 installs the Python2 version.

Using the Python3 version has following consequences:

1) .gitlab-ci.yml* would need to be changed, otherwise the job fails:
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/72023274

2) It seems flake8 detects when the Python3 interpreter is in use and enable
more warnings (causing the need of patch 3).

It seems using the Python3 version *is* an improvement as it will reduce the
burden when migrating everything to Python3 becomes unavoidable.
And also it doesn't have a big impact as we can still keep scripts
back-compatible to Python2.6 when we want. We want that for those scripts that
can run during the build and therefore on old distros. AFAIK we only need to
use in those cases:
from __future__ import foo

*But* I think this change (starting to use the Python3 version of flake8)
belongs to a separate patch.

>          g++-multilib \
>          git \
>          libc6:i386 \
> @@ -47,14 +48,6 @@ RUN apt-get install -y --no-install-recommends \
>      apt-get -y autoremove && \
>      apt-get -y clean
>  
> -# For check-flake8
> -RUN python -m pip install --upgrade pip setuptools wheel && \

The removal of python-pip from 'apt-get install' should be done in this patch.

> -    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
> -- 
> 2.14.1

So I propose these patches for a new iteration:

1) support/docker: run apt-get update and apt-get install in two RUNs
   as-is
2) support/docker: sort the list of installed packages
   without removing python-pip
3) support/docker: use the distro-provided flake8
   current patch 4, but using python-flake8 and removing python-pip

Note: you could stop on patch 3 if you want.

4) support/testing: fix python syntax
   current patch 3, fixing one more script
5) support/docker: use flake8 from Python 3
   new patch, changing python-flake8 to flake8 and changing .gitlab-ci.yml*


Regards,
Ricardo

[Buildroot] [PATCH 4/4] support/docker: use the distro-provided flake8

[Yann E. MORIN]

On 2018-06-03 01:50 -0300, Ricardo Martincoski spake thusly:
> Hello,
> 
> + Thomas DS, who first mentioned flake8 in the mailing list.
> 
> On Sat, Jun 02, 2018 at 07:19 PM, Yann E. MORIN wrote:
> 
> > Currently, we install flake8 and its dependencies via pip. We
> > tried to be reproducible by pinning the version of those python
> > packages, but we did forget quite a few of them, and thus some
> > dependencies for flake8 are installed as uncontrolled versions.
> > 
> > Furthermore, before we install flake8 and its dependencies, we
> > forcibly update pip, setuptools, and wheels packages to their
> > latest versions. This explicitly breaks reproducibility.
> 
> Sorry about this mess, created in:
> "14aa15a5a5 support/dockerfile: install flake8"

Meh, no problem, I ACKed it! ;-)

[--SNIP--]
> IMO it would be good to explicitly state here that this patch actually
> downgrades flake8.
> http://lists.busybox.net/pipermail/buildroot/2018-May/222376.html

ACK, will do.

> Again, in this trade-off, using this simple approach makes sense to me because:
>  - we get reproducible docker images with simple and easily maintainable code
>    in the dockerfile;
>  - I am not aware of any serious limitation of this old version, the release
>    notes for a version in the between mentions 'Dramatically improve the
>    performance' but we have a limited number of scripts and running on Gitlab
>    for all of them still takes less than 5 minutes;
>  - we will eventually upgrade flake8 version again when we bump stretch version
>    and it contains a new version of flake8.
> 
> > 
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> > ---
> >  support/docker/Dockerfile | 9 +--------
> >  1 file changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/support/docker/Dockerfile b/support/docker/Dockerfile
> > index 4f08a54f06..57c9ef78fa 100644
> > --- a/support/docker/Dockerfile
> > +++ b/support/docker/Dockerfile
> > @@ -29,6 +29,7 @@ RUN apt-get install -y --no-install-recommends \
> >          cpio \
> >          cvs \
> >          file \
> > +        flake8 \
> 
> This package installs the Python3 version.
> python-flake8 installs the Python2 version.

Yeah, I had noticed that, but when I just installed python-flake8, I
could not call ;flake8' at all (because presumably there was no
/usr/bin/flake8), so I was wondering how we were s'possed to use the
python2 version at all.

So I did the only sane thing I know: make it work, and let the experts
point out my mistakes during the review! Muhahaha, am I not so twisted?
:-)

And now I think I got it, see below...

> Using the Python3 version has following consequences:
> 
> 1) .gitlab-ci.yml* would need to be changed, otherwise the job fails:
> https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/72023274

Why did we invoke flake8 via an explicit call to python, rather than
flake8 directly? Was it because we installed it from PyPI, so that it
did not install /usr/bin/flake8?

But now, I see that is exactly what we must do anyway, if we switch to
installing python-flake8 instead of flake8. Thanks! :-)

> 2) It seems flake8 detects when the Python3 interpreter is in use and enable
> more warnings (causing the need of patch 3).

Meh, as I said in an earlier reply: why does the interpreter of flake8
have an impact on the code that flake8 anlyses? So, if flake8 was
written in python42, it would aply the python42 rules to python2 and
python3 code? That's just insane... :-/

> It seems using the Python3 version *is* an improvement as it will reduce the
> burden when migrating everything to Python3 becomes unavoidable.
> And also it doesn't have a big impact as we can still keep scripts
> back-compatible to Python2.6 when we want. We want that for those scripts that
> can run during the build and therefore on old distros. AFAIK we only need to
> use in those cases:
> from __future__ import foo
> 
> *But* I think this change (starting to use the Python3 version of flake8)
> belongs to a separate patch.

ACK.

> >          g++-multilib \
> >          git \
> >          libc6:i386 \
> > @@ -47,14 +48,6 @@ RUN apt-get install -y --no-install-recommends \
> >      apt-get -y autoremove && \
> >      apt-get -y clean
> >  
> > -# For check-flake8
> > -RUN python -m pip install --upgrade pip setuptools wheel && \
> 
> The removal of python-pip from 'apt-get install' should be done in this patch.

Yup...

> > -    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
> > -- 
> > 2.14.1
> 
> So I propose these patches for a new iteration:
> 
> 1) support/docker: run apt-get update and apt-get install in two RUNs
>    as-is
> 2) support/docker: sort the list of installed packages
>    without removing python-pip
> 3) support/docker: use the distro-provided flake8
>    current patch 4, but using python-flake8 and removing python-pip

ACK

> Note: you could stop on patch 3 if you want.
> 
> 4) support/testing: fix python syntax
>    current patch 3, fixing one more script

Well, this one is still interesting, because it makes the code more
future-proof anyway, but expecially makes it more homogeneous with the
rest of our code base.

> 5) support/docker: use flake8 from Python 3
>    new patch, changing python-flake8 to flake8 and changing .gitlab-ci.yml*

In the end, do we really need it? Yes, it is good to catch the legacy
exception trapping as well as the print-is-a-function change, but that
can also be caught during the reviews...

Anyway, I'll do the 5 patches, and we can apply up to whatever makes
sense.

Thanks for the reviews! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 4/4] support/docker: use the distro-provided flake8

[Ricardo Martincoski]

Hello,

On Sun, Jun 03, 2018 at 04:36 AM, Yann E. MORIN wrote:

[snip]
>> Using the Python3 version has following consequences:
>> 
>> 1) .gitlab-ci.yml* would need to be changed, otherwise the job fails:
>> https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/72023274
> 
> Why did we invoke flake8 via an explicit call to python, rather than
> flake8 directly? Was it because we installed it from PyPI, so that it
> did not install /usr/bin/flake8?

It's not because of pip/PyPI, since /usr/local/bin/flake8 is there in the
current docker image.
Sorry, I don't remember why I used the 'python -m' form. I guess I started
(before we had tagged images) using flake8 from the distro and then switched to
pip to get the bleeding-edge version, but I am not sure.

But I still think it's better that way, as one can know by looking on the
.gitlab-ci.yml that python2 is used.
We could, for instance, have 2 jobs on Gitlab: check-flake8-python2 and
check-flake8-python3. check-flake8-python2 could potentially (not tested!) warn
about constructs that would fail on old distros with only python2.
But since we don't have too many scripts I think we don't need it. And the
scripts that must run in old distros are tested in some autobuilders.

> 
> But now, I see that is exactly what we must do anyway, if we switch to
> installing python-flake8 instead of flake8. Thanks! :-)
> 
>> 2) It seems flake8 detects when the Python3 interpreter is in use and enable
>> more warnings (causing the need of patch 3).
> 
> Meh, as I said in an earlier reply: why does the interpreter of flake8
> have an impact on the code that flake8 anlyses? So, if flake8 was

This is by design. I don't know the internals, but it seems this is the way a
script can be checked to be compatible to multiple interpreter versions.

http://flake8.pycqa.org/en/3.2.1/#installation
"It is very important to install Flake8 on the correct version of Python for
your needs. If you want Flake8 to properly parse new language features in
Python 3.5 (for example), you need it to be installed on 3.5 for Flake8 to
understand those features. In many ways, Flake8 is tied to the version of
Python on which it runs."

Notice it is not only the major version that matters.
If we want to make sure our Python3 scripts are compatible to both Python 3.4
and Python 3.5 we should ensure we call flake8 under Python 3.4 (ideally under
both but I hope not much APIs are removed for minor versions). But it will
probably automagically works once you add more docker images for the big
distros in another series. So one more reason to use the flake8 version
packaged with each distro.

> written in python42, it would aply the python42 rules to python2 and
> python3 code? That's just insane... :-/

But one can have multiple installs of flake8, one for python2, one for
python3.4, one for python 3.5, one for python4.
People that write a lot of Python scripts usually use virtualenvs to keep this
manageable.

[snip]
>> 5) support/docker: use flake8 from Python 3
>>    new patch, changing python-flake8 to flake8 and changing .gitlab-ci.yml*
> 
> In the end, do we really need it? Yes, it is good to catch the legacy
> exception trapping as well as the print-is-a-function change, but that
> can also be caught during the reviews...

We don't really need it.
But it seems to me that by using it we will make any new script more
future-proof. And if something is missed during the review we will know sooner.
Notice it's only a guess. I don't use flake8 this way.


Regards,
Ricardo