On Sat, Apr 6, 2024 at 7:56 AM Rob Herring <robh@xxxxxxxxxx> 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 = $(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@xxxxxxxxxx>
> Link: https://lore.kernel.org/all/20220817152027.16928-1-masahiroy@xxxxxxxxxx/
> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> ---
> 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/devicetree/bindings/Makefile
> index 95f1436ebcd0..3779405269ab 100644
> --- a/Documentation/devicetree/bindings/Makefile
> +++ b/Documentation/devicetree/bindings/Makefile
> @@ -37,11 +37,13 @@ CHK_DT_EXAMPLES := $(patsubst $(srctree)/%.yaml,%.example.dtb, $(shell $(find_cm
> quiet_cmd_yamllint = LINT $(src)
> cmd_yamllint = ($(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 = CHKDT $@
> +quiet_cmd_chk_bindings = CHKDT $(src)
Nit.
If you want to avoid the long absolute path
when O= is given, you can do
"CHKDT $(obj)" instead.
> cmd_chk_bindings = ($(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 = SCHEMA $@
> cmd_mk_schema = f=$$(mktemp) ; \
> @@ -49,12 +51,6 @@ quiet_cmd_mk_schema = 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 = $(patsubst $(srctree)/%,%,$(shell $(find_all_cmd)))
>
> override DTC_FLAGS := \
> @@ -64,8 +60,15 @@ override DTC_FLAGS := \
> -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) += .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 += processed-schema.json
> always-$(CHECK_DT_BINDING) += $(patsubst $(obj)/%,%, $(CHK_DT_EXAMPLES))
>
> --
> 2.43.0
>
--
Best Regards
Masahiro Yamada
