[PATCH 5/6] Makefile: Add a pylint checker to the build

[Simon Glass]

Hi Heinrich,

On Mon, 22 Nov 2021 at 01:05, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 11/22/21 04:48, Simon Glass wrote:
> > At present the Python code in U-Boot is somewhat inconsistent, with some
> > files passing pylint quite cleanly and others not.
> >
> > Add a way to track progress on this clean-up, by checking that no module
> > has got any worse as a result of changes.
> >
> > This can be used with 'make pylint'.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> >   .gitignore                |   4 +
> >   Makefile                  |  45 +++++++-
> >   doc/develop/index.rst     |   8 ++
> >   doc/develop/python_cq.rst |  80 +++++++++++++++
> >   scripts/pylint.base       | 211 ++++++++++++++++++++++++++++++++++++++
> >   5 files changed, 347 insertions(+), 1 deletion(-)
> >   create mode 100644 doc/develop/python_cq.rst
> >   create mode 100644 scripts/pylint.base
> >
> > diff --git a/.gitignore b/.gitignore
> > index 35034de6556..28c439f09fd 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -98,3 +98,7 @@ __pycache__
> >
> >   # Python code coverage output (python3-coverage html)
> >   /htmlcov/
> > +
> > +# pylint files
> > +/pylint.cur
> > +/pylint.out/
> > diff --git a/Makefile b/Makefile
> > index 338ae3341e6..ef2b0a853ea 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -521,7 +521,7 @@ env_h := include/generated/environment.h
> >
> >   no-dot-config-targets := clean clobber mrproper distclean \
> >                        help %docs check% coccicheck \
> > -                      ubootversion backup tests check qcheck tcheck
> > +                      ubootversion backup tests check qcheck tcheck pylint
> >
> >   config-targets := 0
> >   mixed-targets  := 0
> > @@ -2239,6 +2239,48 @@ distclean: mrproper
> >               -type f -print | xargs rm -f
> >       @rm -f boards.cfg CHANGELOG
> >
> > +# See doc/develop/python_cq.rst
> > +PHONY += pylint
> > +PYLINT_BASE := scripts/pylint.base
> > +PYLINT_CUR := pylint.cur
> > +PYLINT_DIFF := pylint.diff
> > +pylint:
> > +     $(Q)echo "Running pylint on all files (summary in $(PYLINT_CUR); output in pylint.out/)"
> > +     $(Q)mkdir -p pylint.out
> > +     $(Q)rm -f pylint.out/out*
> > +     $(Q)find tools test -name "*.py" \
> > +             | xargs -n1 -P$(shell nproc 2>/dev/null || echo 1) \
> > +                     sh -c 'pylint --reports=y --exit-zero -f parseable --ignore-imports=yes $$@ > pylint.out/$$(echo $$@ | tr / _ | sed s/.py//)' _
> > +     $(Q)sed -n 's/Your code has been rated at \([-0-9.]*\).*/\1/p; s/\*\** Module \(.*\)/\1/p' pylint.out/* \
> > +             |sed '$!N;s/\n/ /' \
> > +             |sort > $(PYLINT_CUR)
> > +     $(Q)base=$$(mktemp) cur=$$(mktemp); cut -d' ' -f1 $(PYLINT_BASE) >$$base; \
> > +             cut -d' ' -f1 $(PYLINT_CUR) >$$cur; \
> > +             comm -3 $$base $$cur > $(PYLINT_DIFF); \
> > +             if [ -s $(PYLINT_DIFF) ]; then \
> > +                     echo "Files have been added/removed. Try:\n\tcp $(PYLINT_CUR) $(PYLINT_BASE)"; \
> > +                     echo; \
> > +                     echo "Added files:"; \
> > +                     comm -13 $$base $$cur; \
> > +                     echo; \
> > +                     echo "Removed files:"; \
> > +                     comm -23 $$base $$cur; \
> > +                     false; \
> > +             else \
> > +                     rm $$base $$cur $(PYLINT_DIFF); \
> > +             fi
> > +     $(Q)bad=false; while read base_file base_val <&3 && read cur_file cur_val <&4; do \
> > +             if awk "BEGIN {exit !($$cur_val < $$base_val)}"; then \
> > +                     echo "$$base_file: Score was $$base_val, now $$cur_val"; \
> > +                     bad=true; fi; \
> > +             done 3<$(PYLINT_BASE) 4<$(PYLINT_CUR); \
> > +             if $$bad; then \
> > +                     echo "Some files have regressed, please fix"; \
> > +                     false; \
> > +             else \
> > +                     echo "No pylint regressions"; \
> > +             fi
> > +
>
> This is over-engineered.
>
> ./scripts/pylint.base would have to be updated after every patch. Who
> will do this?

It doesn't really need to be updated. Areyou suggesting I over-engeer
it further by making ti do that automatically?

>
> Deleting superfluous but (according to pylint) correct code lines may
> decrease the score. This is not because any new errors where introduced
> but because the number of errors per line increases. But such a change
> should pass CI.
>
> This needs to be fixed before adding this series.

Yes that's right, it is a pain. What do you suggest?

The nice thing is that you have to fix a few things when adding new code, right?

>
> Your test does not detect new Python units with abysmal code.

They need to have a line added, which will show the score.

>
> We should simply set a minimum score (>= 8) for all units including old
> ones.

Unfortunately I don't think that will have any effect. The point of
this is to encourage things to be improved. If it just stays at 2.4 no
one will care...?

Regards,
Simon