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

[Heinrich Schuchardt]

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?

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.

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

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

Best regards

Heinrich


>   backup:
>   	F=`basename $(srctree)` ; cd .. ; \
>   	gtar --force-local -zcvf `LC_ALL=C date "+$$F-%Y-%m-%d-%T.tar.gz"` $$F
> @@ -2257,6 +2299,7 @@ help:
>   	@echo  '  check           - Run all automated tests that use sandbox'
>   	@echo  '  qcheck          - Run quick automated tests that use sandbox'
>   	@echo  '  tcheck          - Run quick automated tests on tools'
> +	@echo  '  pylint          - Run pylint on all Python files'
>   	@echo  ''
>   	@echo  'Other generic targets:'
>   	@echo  '  all		  - Build all necessary images depending on configuration'
> diff --git a/doc/develop/index.rst b/doc/develop/index.rst
> index 9592d193fca..97a7f4ce14a 100644
> --- a/doc/develop/index.rst
> +++ b/doc/develop/index.rst
> @@ -61,3 +61,11 @@ Refactoring
>      checkpatch
>      coccinelle
>      moveconfig
> +
> +Code quality
> +------------
> +
> +.. toctree::
> +   :maxdepth: 1
> +
> +   python_cq
> diff --git a/doc/develop/python_cq.rst b/doc/develop/python_cq.rst
> new file mode 100644
> index 00000000000..3f99f1d9c0b
> --- /dev/null
> +++ b/doc/develop/python_cq.rst
> @@ -0,0 +1,80 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +
> +Python code quality
> +===================
> +
> +U-Boot has about 60k lines of Python code, mainly in the following areas:
> +
> +- tests
> +- pytest hooks
> +- patman patch submission tool
> +- buildman build / analysis tool
> +- dtoc devicetree-to-C tool
> +- binman firmware packaging tool
> +
> +`PEP 8`_ is used for the code style, with the single quote (') used by default for
> +strings and double quote for doc strings. All non-trivial functions should be
> +commented.
> +
> +Pylint is used to help check this code and keep a consistent code style. The
> +build system tracks the current 'score' of the source code and detects
> +regressions in any module.
> +
> +To run this locally you should use this version of pylint::
> +
> +    # pylint --version
> +    pylint 2.11.1
> +    astroid 2.8.6
> +    Python 3.8.10 (default, Sep 28 2021, 16:10:42)
> +    [GCC 9.3.0]
> +
> +
> +You should be able to select and this install other required tools with::
> +
> +    pip install pylint==2.11.1
> +    pip install -r test/py/requirements.txt
> +    pip install asteval pyopenssl
> +
> +Note that if your distribution is a year or two old, you make need to use `pip3`
> +instead.
> +
> +To configure pylint, make sure it has docparams enabled, e.g. with::
> +
> +    echo "[MASTER]" >> .pylintrc
> +    echo "load-plugins=pylint.extensions.docparams" >> .pylintrc
> +
> +Once everything is ready, use this to check the code::
> +
> +    make pylint
> +
> +This creates a directory called `pylint.out` which contains the pylint output
> +for each Python file in U-Boot. It also creates a summary file called
> +`pylint.cur` which shows the pylint score for each module::
> +
> +    _testing 0.83
> +    atf_bl31 -6.00
> +    atf_fip 0.49
> +    binman.cbfs_util 7.70
> +    binman.cbfs_util_test 9.19
> +    binman.cmdline 7.73
> +    binman.control 4.39
> +    binman.elf 6.42
> +    binman.elf_test 5.41
> +    ...
> +
> +This file is in alphabetical order. The build system compares the score of each
> +module to `scripts/pylint.base` (which must also be sorted and have exactly the
> +same modules in it) and reports any files where the score has dropped. Use
> +pylint to check what is wrong and fix up the code before you send out your
> +patches.
> +
> +New or removed files results in an error which can be resolved by updating the
> +`scripts/pylint.base` file to add/remove lines for those files, e.g.::
> +
> +    meld pylint.cur scripts/pylint.base
> +
> +If the pylint version is updated in CI, this may result in needing to regenerate
> +`scripts/pylint.base`.
> +
> +
> +.. _`PEP 8`: https://www.python.org/dev/peps/pep-0008/
> diff --git a/scripts/pylint.base b/scripts/pylint.base
> new file mode 100644
> index 00000000000..d848ebe9058
> --- /dev/null
> +++ b/scripts/pylint.base
> @@ -0,0 +1,211 @@
> +_testing 0.83
> +atf_bl31 -6.00
> +binman.cbfs_util 7.70
> +binman.cbfs_util_test 9.19
> +binman.cmdline 8.87
> +binman.control 4.39
> +binman.elf 6.42
> +binman.elf_test 5.41
> +binman.entry 2.39
> +binman.entry_test 5.29
> +binman.fdt_test 3.23
> +binman.fmap_util 6.67
> +binman.ftest 7.38
> +binman.image 6.39
> +binman.image_test 4.48
> +binman.main 4.26
> +binman.setup 5.00
> +binman.state 3.30
> +blob -1.94
> +blob_dtb -10.00
> +blob_ext -20.00
> +blob_named_by_arg -7.78
> +blob_phase -5.00
> +buildman.board 7.11
> +buildman.bsettings 0.98
> +buildman.builder 6.55
> +buildman.builderthread 7.35
> +buildman.cmdline 8.85
> +buildman.control 7.04
> +buildman.func_test 6.38
> +buildman.kconfiglib 7.48
> +buildman.main 1.43
> +buildman.test 6.10
> +buildman.toolchain 5.81
> +capsule_defs 5.00
> +cbfs -1.52
> +collection 2.33
> +concurrencytest 6.77
> +conftest -3.84
> +conftest 1.25
> +conftest 4.62
> +conftest 6.43
> +cros_ec_rw -6.00
> +defs 6.67
> +dtoc.dtb_platdata 7.82
> +dtoc.fdt 3.47
> +dtoc.fdt_util 4.53
> +dtoc.main 7.33
> +dtoc.setup 5.00
> +dtoc.src_scan 8.75
> +dtoc.test_dtoc 8.54
> +dtoc.test_fdt 6.92
> +dtoc.test_src_scan 9.43
> +efivar 6.71
> +endian-swap 8.93
> +fdtmap -4.00
> +files -7.43
> +fill -6.43
> +fit 5.32
> +fmap -0.59
> +fstest_defs 8.33
> +fstest_helpers 4.29
> +gbb -0.30
> +genboardscfg 7.27
> +image_header 5.58
> +intel_cmc -12.50
> +intel_descriptor 4.62
> +intel_fit 0.00
> +intel_fit_ptr 2.35
> +intel_fsp -12.50
> +intel_fsp_m -12.50
> +intel_fsp_s -12.50
> +intel_fsp_t -12.50
> +intel_ifwi 2.71
> +intel_me -12.50
> +intel_mrc -10.00
> +intel_refcode -10.00
> +intel_vbt -12.50
> +intel_vga -12.50
> +microcode-tool 7.19
> +mkimage 2.07
> +moveconfig 7.31
> +multiplexed_log 7.01
> +opensbi -6.00
> +patman 0.00
> +patman.checkpatch 7.61
> +patman.command 4.23
> +patman.commit 2.75
> +patman.control 8.14
> +patman.cros_subprocess 7.41
> +patman.func_test 7.87
> +patman.get_maintainer 4.71
> +patman.gitutil 4.55
> +patman.main 8.23
> +patman.patchstream 9.04
> +patman.project 3.33
> +patman.series 5.95
> +patman.settings 5.63
> +patman.setup 5.00
> +patman.status 8.43
> +patman.terminal 6.29
> +patman.test_checkpatch 6.81
> +patman.test_util 6.51
> +patman.tools 3.47
> +patman.tout 2.97
> +powerpc_mpc85xx_bootpg_resetvec -10.00
> +rkmux 6.76
> +rmboard 7.76
> +scp -6.00
> +section 4.25
> +sqfs_common 8.12
> +test 8.18
> +test_000_version 7.50
> +test_ab 6.50
> +test_abootimg 6.09
> +test_authvar 8.93
> +test_avb 5.52
> +test_basic 0.40
> +test_bind -2.99
> +test_button 3.33
> +test_capsule_firmware 3.89
> +test_dfu 5.45
> +test_dm 9.52
> +test_efi_fit 7.59
> +test_efi_loader 6.79
> +test_efi_selftest 6.12
> +test_env 6.68
> +test_ext -0.25
> +test_extension 2.14
> +test_fit 6.20
> +test_fit_ecdsa 7.50
> +test_fit_hashes 7.70
> +test_fpga 1.81
> +test_fs_cmd 8.00
> +test_gpio 6.09
> +test_gpt 7.67
> +test_handoff 5.00
> +test_help 5.00
> +test_hush_if_test 9.27
> +test_log 8.64
> +test_lsblk 8.00
> +test_md 3.64
> +test_mkdir 1.96
> +test_mmc_rd 6.05
> +test_mmc_wr 3.33
> +test_net 6.84
> +test_ofplatdata 5.71
> +test_part 8.00
> +test_pinmux 3.27
> +test_pstore 2.31
> +test_qfw 8.75
> +test_sandbox_exit 6.50
> +test_scp03 3.33
> +test_sf 7.02
> +test_shell_basics 9.58
> +test_signed 8.38
> +test_signed_intca 8.10
> +test_sleep 7.78
> +test_spl 2.22
> +test_sqfs_load 7.12
> +test_sqfs_ls 8.00
> +test_stackprotector 5.71
> +test_symlink 0.82
> +test_tpm2 8.51
> +test_ums 6.32
> +test_unknown_cmd 5.00
> +test_unlink 2.22
> +test_unsigned 8.00
> +test_ut 7.06
> +test_vboot 6.00
> +text -0.48
> +u_boot -15.71
> +u_boot_console_base 6.80
> +u_boot_console_exec_attach 8.46
> +u_boot_console_sandbox 6.94
> +u_boot_dtb -12.22
> +u_boot_dtb_with_ucode 0.39
> +u_boot_elf -8.42
> +u_boot_env 0.74
> +u_boot_expanded -10.00
> +u_boot_img -15.71
> +u_boot_nodtb -15.71
> +u_boot_spawn 6.67
> +u_boot_spl -10.91
> +u_boot_spl_bss_pad -9.29
> +u_boot_spl_dtb -12.22
> +u_boot_spl_elf -15.71
> +u_boot_spl_expanded -9.09
> +u_boot_spl_nodtb -10.91
> +u_boot_spl_with_ucode_ptr -5.00
> +u_boot_tpl -10.91
> +u_boot_tpl_bss_pad -9.29
> +u_boot_tpl_dtb -12.22
> +u_boot_tpl_dtb_with_ucode -7.50
> +u_boot_tpl_elf -15.71
> +u_boot_tpl_expanded -9.09
> +u_boot_tpl_nodtb -10.91
> +u_boot_tpl_with_ucode_ptr -20.83
> +u_boot_ucode 1.52
> +u_boot_utils 4.69
> +u_boot_with_ucode_ptr -0.71
> +vblock -1.94
> +vboot_evil 8.95
> +vboot_forge 9.22
> +x86_reset16 -15.71
> +x86_reset16_spl -15.71
> +x86_reset16_tpl -15.71
> +x86_start16 -15.71
> +x86_start16_spl -15.71
> +x86_start16_tpl -15.71
> +zynqmp_pm_cfg_obj_convert 6.67
>