Re: [PATCH v2 2/2] tools: binman: ti_board_cfg: Check for linting problems

[Max Krummenacher <max.oss.09@gmail.com>]

Hi

On Fri, Jan 05, 2024 at 05:09:17PM +0530, Neha Malcom Francis wrote:
> Use yamllint for checking whether YAML configuration files are adhering
> to default yamllint rules.

If I understand this correctly this patch now runs checks to verify
that yaml files which are part of the U-Boot source tree are correct.

Shouldn't this be done when one commits a yaml file, i.e. in U-Boot CI
rather than repeating the process on every build and thus having an
additional dependency to build U-Boot and additional build time?

Note that in openembedded there is currently no python yamllint
recipe providing the used python module in the regularly used layers.
How do you plan to integrate the change in the OE U-Boot recipe?
At least our OE CI of latest master now fails (for a TI AM62 based SoM)
as the python module is missing.

Regards
Max

> 
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> Suggested-by: Nishanth Menon <nm@ti.com>
> ---
> Changes since v1:
> 	- add yamllint to requirements.txt (Nishanth)
> 
>  tools/binman/etype/ti_board_config.py        |  5 +++++
>  tools/binman/ftest.py                        |  6 ++++++
>  tools/binman/test/323_ti_board_cfg_phony.dts | 14 ++++++++++++++
>  tools/binman/test/yaml/config.yaml           |  4 ++--
>  tools/binman/test/yaml/config_phony.yaml     | 18 ++++++++++++++++++
>  tools/buildman/requirements.txt              |  1 +
>  6 files changed, 46 insertions(+), 2 deletions(-)
>  create mode 100644 tools/binman/test/323_ti_board_cfg_phony.dts
>  create mode 100644 tools/binman/test/yaml/config_phony.yaml
> 
> diff --git a/tools/binman/etype/ti_board_config.py b/tools/binman/etype/ti_board_config.py
> index 94f894c281..2c3bb8f7b5 100644
> --- a/tools/binman/etype/ti_board_config.py
> +++ b/tools/binman/etype/ti_board_config.py
> @@ -9,6 +9,7 @@
>  import os
>  import struct
>  import yaml
> +import yamllint
>  
>  from collections import OrderedDict
>  from jsonschema import validate
> @@ -18,6 +19,7 @@ from binman.entry import Entry
>  from binman.etype.section import Entry_section
>  from dtoc import fdt_util
>  from u_boot_pylib import tools
> +from yamllint import config
>  
>  BOARDCFG = 0xB
>  BOARDCFG_SEC = 0xD
> @@ -244,6 +246,9 @@ class Entry_ti_board_config(Entry_section):
>              with open(self._schema_file, 'r') as sch:
>                  self.schema_yaml = yaml.safe_load(sch)
>  
> +            yaml_config = config.YamlLintConfig("extends: default")
> +            for p in yamllint.linter.run(open(self._config_file, "r"), yaml_config):
> +                self.Raise(f"Yamllint error: {p.line}: {p.rule}")
>              try:
>                  validate(self.file_yaml, self.schema_yaml)
>              except Exception as e:
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index a4ac520cbb..1fbb0fef1a 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -7030,6 +7030,12 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>          data = self._DoReadFile('293_ti_board_cfg.dts')
>          self.assertEqual(TI_BOARD_CONFIG_DATA, data)
>  
> +    def testTIBoardConfigLint(self):
> +        """Test that an incorrectly linted config file would generate error"""
> +        with self.assertRaises(ValueError) as e:
> +            data = self._DoReadFile('323_ti_board_cfg_phony.dts')
> +        self.assertIn("Yamllint error", str(e.exception))
> +
>      def testTIBoardConfigCombined(self):
>          """Test that a schema validated combined board config file can be generated"""
>          data = self._DoReadFile('294_ti_board_cfg_combined.dts')
> diff --git a/tools/binman/test/323_ti_board_cfg_phony.dts b/tools/binman/test/323_ti_board_cfg_phony.dts
> new file mode 100644
> index 0000000000..441296de4f
> --- /dev/null
> +++ b/tools/binman/test/323_ti_board_cfg_phony.dts
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	binman {
> +		ti-board-config {
> +			config = "yaml/config_phony.yaml";
> +			schema = "yaml/schema.yaml";
> +		};
> +	};
> +};
> diff --git a/tools/binman/test/yaml/config.yaml b/tools/binman/test/yaml/config.yaml
> index 5f799a6e3a..c2be32128b 100644
> --- a/tools/binman/test/yaml/config.yaml
> +++ b/tools/binman/test/yaml/config.yaml
> @@ -10,9 +10,9 @@ main-branch:
>      b: 0
>    arr: [0, 0, 0, 0]
>    another-arr:
> -    - #1
> +    -  # 1
>        c: 0
>        d: 0
> -    - #2
> +    -  # 2
>        c: 0
>        d: 0
> diff --git a/tools/binman/test/yaml/config_phony.yaml b/tools/binman/test/yaml/config_phony.yaml
> new file mode 100644
> index 0000000000..d76fcb3b82
> --- /dev/null
> +++ b/tools/binman/test/yaml/config_phony.yaml
> @@ -0,0 +1,18 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Test config
> +#
> +---
> +
> +main-branch :
> +  obj :
> +    a : 0x0
> +    b: 0
> +  arr: [0, 0, 0, 0]
> +  another-arr:
> +    -  # 1
> +      c: 0
> +      d: 0
> +    -  # 2
> +      c: 0
> +      d: 0
> diff --git a/tools/buildman/requirements.txt b/tools/buildman/requirements.txt
> index a1efcb9d4b..4a31e69e4c 100644
> --- a/tools/buildman/requirements.txt
> +++ b/tools/buildman/requirements.txt
> @@ -1,2 +1,3 @@
>  jsonschema==4.17.3
>  pyyaml==6.0
> +yamllint==1.26.3
> -- 
> 2.34.1
>