Received-SPF: pass (google.com: domain of linux-kernel+bounces-152161-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240405-dt-kbuild-rework-v2-0-3a035caee357@kernel.org> <20240405-dt-kbuild-rework-v2-2-3a035caee357@kernel.org>
In-Reply-To: <20240405-dt-kbuild-rework-v2-2-3a035caee357@kernel.org>
From: Masahiro Yamada <masahiroy@kernel.org>
Date: Sat, 20 Apr 2024 15:50:19 +0900
Message-ID: <CAK7LNATENrTeCt4bE+bXzq1-caMguiT+U0G5YyT09T032gNtWQ@mail.gmail.com>
Subject: Re: [PATCH v2 2/3] dt-bindings: kbuild: Split targets out to separate rules
To: Rob Herring <robh@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Nathan Chancellor <nathan@kernel.org>, Nicolas Schier <nicolas@fjasle.eu>, 
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, 
	Marijn Suijten <marijn.suijten@somainline.org>, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-kbuild@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Apr 6, 2024 at 7:56=E2=80=AFAM Rob Herring <robh@kernel.org> wrote:
>
> Masahiro pointed out the use of if_changed_rule is incorrect and command
> line changes are not correctly accounted for.
>
> To fix this, split up the DT binding validation target,
> dt_binding_check, into multiple rules for each step: yamllint, schema
> validtion with meta-schema, and building the processed schema.
>
> One change in behavior is the yamllint or schema validation will be
> re-run again when there are warnings present.


Is this intentional?

I think it is annoying to re-run the same check
when none of the schemas is updated.

'make dt_binding_check' is already warning-free.
So, I think it is OK to make it fail if any warning occurs.

Besides, yamllint exists with 0 even if it finds a format error.
Hence  "|| true" is not sensible.


I like this code:

cmd_yamllint =3D $(find_cmd) | \
        xargs -n200 -P$$(nproc) \
        $(DT_SCHEMA_LINT) -f parsable -c $(srctree)/$(src)/.yamllint >&2; \
         touch $@


Same for  cmd_chk_bindings.


>
> Reported-by: Masahiro Yamada <masahiroy@kernel.org>
> Link: https://lore.kernel.org/all/20220817152027.16928-1-masahiroy@kernel=
org/
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v2:
>  - Separated rework of build rules to fix if_changed_rule usage from
>    addition of top-level build rules.
> ---
>  Documentation/devicetree/bindings/Makefile | 25 ++++++++++++++----------=
-
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/Makefile b/Documentation/d=
evicetree/bindings/Makefile
> index 95f1436ebcd0..3779405269ab 100644
> --- a/Documentation/devicetree/bindings/Makefile
> +++ b/Documentation/devicetree/bindings/Makefile
> @@ -37,11 +37,13 @@ CHK_DT_EXAMPLES :=3D $(patsubst $(srctree)/%.yaml,%.e=
xample.dtb, $(shell $(find_cm
>  quiet_cmd_yamllint =3D LINT    $(src)
>        cmd_yamllint =3D ($(find_cmd) | \
>                       xargs -n200 -P$$(nproc) \
> -                    $(DT_SCHEMA_LINT) -f parsable -c $(srctree)/$(src)/.=
yamllint >&2) || true
> +                    $(DT_SCHEMA_LINT) -f parsable -c $(srctree)/$(src)/.=
yamllint >&2) \
> +                    && touch $@ || true
>
> -quiet_cmd_chk_bindings =3D CHKDT   $@
> +quiet_cmd_chk_bindings =3D CHKDT   $(src)


Nit.

If you want to avoid the long absolute path
when O=3D is given, you can do
"CHKDT   $(obj)" instead.


>        cmd_chk_bindings =3D ($(find_cmd) | \
> -                         xargs -n200 -P$$(nproc) $(DT_DOC_CHECKER) -u $(=
srctree)/$(src)) || true
> +                         xargs -n200 -P$$(nproc) $(DT_DOC_CHECKER) -u $(=
srctree)/$(src)) \
> +                         && touch $@ || true
>
>  quiet_cmd_mk_schema =3D SCHEMA  $@
>        cmd_mk_schema =3D f=3D$$(mktemp) ; \
> @@ -49,12 +51,6 @@ quiet_cmd_mk_schema =3D SCHEMA  $@
>                        $(DT_MK_SCHEMA) -j $(DT_MK_SCHEMA_FLAGS) @$$f > $@=
 ; \
>                       rm -f $$f
>
> -define rule_chkdt
> -       $(if $(DT_SCHEMA_LINT),$(call cmd,yamllint),)
> -       $(call cmd,chk_bindings)
> -       $(call cmd,mk_schema)
> -endef
> -
>  DT_DOCS =3D $(patsubst $(srctree)/%,%,$(shell $(find_all_cmd)))
>
>  override DTC_FLAGS :=3D \
> @@ -64,8 +60,15 @@ override DTC_FLAGS :=3D \
>         -Wno-unique_unit_address \
>         -Wunique_unit_address_if_enabled
>
> -$(obj)/processed-schema.json: $(DT_DOCS) $(src)/.yamllint check_dtschema=
_version FORCE
> -       $(call if_changed_rule,chkdt)
> +$(obj)/processed-schema.json: $(DT_DOCS) check_dtschema_version FORCE
> +       $(call if_changed,mk_schema)
> +
> +always-$(CHECK_DT_BINDING) +=3D .dt-binding.checked .yamllint.checked
> +$(obj)/.yamllint.checked: $(DT_DOCS) $(src)/.yamllint FORCE
> +       $(if $(DT_SCHEMA_LINT),$(call if_changed,yamllint),)
> +
> +$(obj)/.dt-binding.checked: $(DT_DOCS) FORCE
> +       $(call if_changed,chk_bindings)
>
>  always-y +=3D processed-schema.json
>  always-$(CHECK_DT_BINDING) +=3D $(patsubst $(obj)/%,%, $(CHK_DT_EXAMPLES=
))
>
> --
> 2.43.0
>


--
Best Regards
Masahiro Yamada