2024-04-05 22:56:25

by Rob Herring (Arm)

[permalink] [raw]
Subject: [PATCH v2 0/3] dt-bindings: kbuild: Rework build rules and dependencies

This series reworks the DT binding build rules and dependencies. It
fixes a problem with if_changed_rule Masahiro reported some time back[1]
and improves the dependency handling for the DT validation targets.

Relative to v1, I've dropped all but 1 one of the top-level targets
added in v1. The only top-level target added it for building the
processed schema used by multiple targets.

Rob

v1: https://lore.kernel.org/all/[email protected]/

[1] https://lore.kernel.org/all/[email protected]/

Signed-off-by: Rob Herring <[email protected]>
---
Rob Herring (3):
dt-bindings: kbuild: Simplify examples target patsubst
dt-bindings: kbuild: Split targets out to separate rules
dt-bindings: kbuild: Add separate target/dependency for processed-schema.json

Documentation/devicetree/bindings/Makefile | 34 ++++++++++++++++++------------
Makefile | 24 ++++++++++-----------
scripts/Makefile.lib | 2 +-
3 files changed, 33 insertions(+), 27 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240405-dt-kbuild-rework-f356ab890d45

Best regards,
--
Rob Herring <[email protected]>



2024-04-05 22:56:36

by Rob Herring (Arm)

[permalink] [raw]
Subject: [PATCH v2 1/3] dt-bindings: kbuild: Simplify examples target patsubst

Instead of stripping off the $(srctree) multiple times do it once up
front, but keep the src/obj path as it is going to be needed in
subsequent commit.

Rename the variable to CHK_DT_EXAMPLES to better reflect what it
contains.

Signed-off-by: Rob Herring <[email protected]>
---
v2: New patch
---
Documentation/devicetree/bindings/Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/Makefile b/Documentation/devicetree/bindings/Makefile
index 5e08e3a6a97b..95f1436ebcd0 100644
--- a/Documentation/devicetree/bindings/Makefile
+++ b/Documentation/devicetree/bindings/Makefile
@@ -32,7 +32,7 @@ find_cmd = $(find_all_cmd) | \
sed 's|^$(srctree)/||' | \
grep -F -e "$(subst :," -e ",$(DT_SCHEMA_FILES))" | \
sed 's|^|$(srctree)/|'
-CHK_DT_DOCS := $(shell $(find_cmd))
+CHK_DT_EXAMPLES := $(patsubst $(srctree)/%.yaml,%.example.dtb, $(shell $(find_cmd)))

quiet_cmd_yamllint = LINT $(src)
cmd_yamllint = ($(find_cmd) | \
@@ -68,8 +68,8 @@ $(obj)/processed-schema.json: $(DT_DOCS) $(src)/.yamllint check_dtschema_version
$(call if_changed_rule,chkdt)

always-y += processed-schema.json
-always-$(CHECK_DT_BINDING) += $(patsubst $(srctree)/$(src)/%.yaml,%.example.dts, $(CHK_DT_DOCS))
-always-$(CHECK_DT_BINDING) += $(patsubst $(srctree)/$(src)/%.yaml,%.example.dtb, $(CHK_DT_DOCS))
+always-$(CHECK_DT_BINDING) += $(patsubst $(obj)/%,%, $(CHK_DT_EXAMPLES))
+always-$(CHECK_DT_BINDING) += $(patsubst $(obj)/%.dtb,%.dts, $(CHK_DT_EXAMPLES))

# Hack: avoid 'Argument list too long' error for 'make clean'. Remove most of
# build artifacts here before they are processed by scripts/Makefile.clean

--
2.43.0


2024-04-05 22:56:48

by Rob Herring (Arm)

[permalink] [raw]
Subject: [PATCH v2 2/3] dt-bindings: kbuild: Split targets out to separate rules

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.

Reported-by: Masahiro Yamada <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Rob Herring <[email protected]>
---
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)
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


2024-04-05 22:57:05

by Rob Herring (Arm)

[permalink] [raw]
Subject: [PATCH v2 3/3] dt-bindings: kbuild: Add separate target/dependency for processed-schema.json

Running dtbs_check and dt_compatible_check targets really only depend
on processed-schema.json, but the dependency is 'dt_binding_check'. That
was sort worked around with the CHECK_DT_BINDING variable in order to
skip some of the work that 'dt_binding_check' does. It still runs the
full checks of the schemas which is not necessary and adds 10s of
seconds to the build time. That's significant when checking only a few
DTBs and with recent changes that have improved the validation time by
6-7x.

Add a new target, dt_binding_schema, which just builds
processed-schema.json and can be used as the dependency for other
targets. The scripts_dtc dependency isn't needed either as the examples
aren't built for it.

Signed-off-by: Rob Herring <[email protected]>
---
v2:
- Just split out building processed-schema.json and drop running
yamllint, schema validation, or checking examples separately.
- Fix multiple targets in parallel build. (Thanks Masahiro!)
---
Documentation/devicetree/bindings/Makefile | 9 ++++++---
Makefile | 24 ++++++++++++------------
scripts/Makefile.lib | 2 +-
3 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/Documentation/devicetree/bindings/Makefile b/Documentation/devicetree/bindings/Makefile
index 3779405269ab..8cdda477987f 100644
--- a/Documentation/devicetree/bindings/Makefile
+++ b/Documentation/devicetree/bindings/Makefile
@@ -63,7 +63,7 @@ override DTC_FLAGS := \
$(obj)/processed-schema.json: $(DT_DOCS) check_dtschema_version FORCE
$(call if_changed,mk_schema)

-always-$(CHECK_DT_BINDING) += .dt-binding.checked .yamllint.checked
+targets += .dt-binding.checked .yamllint.checked
$(obj)/.yamllint.checked: $(DT_DOCS) $(src)/.yamllint FORCE
$(if $(DT_SCHEMA_LINT),$(call if_changed,yamllint),)

@@ -71,8 +71,8 @@ $(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))
-always-$(CHECK_DT_BINDING) += $(patsubst $(obj)/%.dtb,%.dts, $(CHK_DT_EXAMPLES))
+targets += $(patsubst $(obj)/%,%, $(CHK_DT_EXAMPLES))
+targets += $(patsubst $(obj)/%.dtb,%.dts, $(CHK_DT_EXAMPLES))

# Hack: avoid 'Argument list too long' error for 'make clean'. Remove most of
# build artifacts here before they are processed by scripts/Makefile.clean
@@ -81,3 +81,6 @@ clean-files = $(shell find $(obj) \( -name '*.example.dts' -o \

dt_compatible_check: $(obj)/processed-schema.json
$(Q)$(srctree)/scripts/dtc/dt-extract-compatibles $(srctree) | xargs dt-check-compatible -v -s $<
+
+PHONY += dt_binding_check
+dt_binding_check: $(obj)/.dt-binding.checked $(obj)/.yamllint.checked $(CHK_DT_EXAMPLES)
diff --git a/Makefile b/Makefile
index 763b6792d3d5..1356e48caa2b 100644
--- a/Makefile
+++ b/Makefile
@@ -1398,12 +1398,12 @@ dtbs: dtbs_prepare
# dtbs_install depend on it as dtbs_install may run as root.
dtbs_prepare: include/config/kernel.release scripts_dtc

-ifneq ($(filter dtbs_check, $(MAKECMDGOALS)),)
+ifneq ($(filter dt_binding_check dtbs_check, $(MAKECMDGOALS)),)
export CHECK_DTBS=y
endif

ifneq ($(CHECK_DTBS),)
-dtbs_prepare: dt_binding_check
+dtbs_prepare: dt_binding_schemas
endif

dtbs_check: dtbs
@@ -1421,16 +1421,15 @@ PHONY += scripts_dtc
scripts_dtc: scripts_basic
$(Q)$(MAKE) $(build)=scripts/dtc

-ifneq ($(filter dt_binding_check, $(MAKECMDGOALS)),)
-export CHECK_DT_BINDING=y
-endif
+PHONY += dt_binding_check dt_binding_schemas
+dt_binding_check: dt_binding_schemas scripts_dtc
+ $(Q)$(MAKE) $(build)=Documentation/devicetree/bindings $@

-PHONY += dt_binding_check
-dt_binding_check: scripts_dtc
+dt_binding_schemas:
$(Q)$(MAKE) $(build)=Documentation/devicetree/bindings

PHONY += dt_compatible_check
-dt_compatible_check: dt_binding_check
+dt_compatible_check: dt_binding_schemas
$(Q)$(MAKE) $(build)=Documentation/devicetree/bindings $@

# ---------------------------------------------------------------------------
@@ -1626,10 +1625,11 @@ help:
@echo ''
@$(if $(dtstree), \
echo 'Devicetree:'; \
- echo '* dtbs - Build device tree blobs for enabled boards'; \
- echo ' dtbs_install - Install dtbs to $(INSTALL_DTBS_PATH)'; \
- echo ' dt_binding_check - Validate device tree binding documents'; \
- echo ' dtbs_check - Validate device tree source files';\
+ echo '* dtbs - Build device tree blobs for enabled boards'; \
+ echo ' dtbs_install - Install dtbs to $(INSTALL_DTBS_PATH)'; \
+ echo ' dt_binding_check - Validate device tree binding documents and examples'; \
+ echo ' dt_binding_schema - Build processed device tree binding schemas'; \
+ echo ' dtbs_check - Validate device tree source files';\
echo '')

@echo 'Userspace tools targets:'
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 3179747cbd2c..d1d51e38b55d 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -410,7 +410,7 @@ $(multi-dtb-y): FORCE
$(call if_changed,fdtoverlay)
$(call multi_depend, $(multi-dtb-y), .dtb, -dtbs)

-ifneq ($(CHECK_DTBS)$(CHECK_DT_BINDING),)
+ifneq ($(CHECK_DTBS),)
DT_CHECKER ?= dt-validate
DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
DT_BINDING_DIR := Documentation/devicetree/bindings

--
2.43.0


2024-04-07 10:59:37

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] dt-bindings: kbuild: Add separate target/dependency for processed-schema.json

On Fri, Apr 05, 2024 at 05:56:03PM -0500, Rob Herring wrote:
> Running dtbs_check and dt_compatible_check targets really only depend
> on processed-schema.json, but the dependency is 'dt_binding_check'. That
> was sort worked around with the CHECK_DT_BINDING variable in order to
> skip some of the work that 'dt_binding_check' does. It still runs the
> full checks of the schemas which is not necessary and adds 10s of
> seconds to the build time. That's significant when checking only a few
> DTBs and with recent changes that have improved the validation time by
> 6-7x.
>
> Add a new target, dt_binding_schema, which just builds
> processed-schema.json and can be used as the dependency for other
> targets. The scripts_dtc dependency isn't needed either as the examples
> aren't built for it.
>
> Signed-off-by: Rob Herring <[email protected]>

Yoo, that's pretty nice. 20 seconds cut off my dtbs_check build time on
riscv with this change :) As you point out, when you're not checking all
that many it is pretty significant - 48 seconds before and 28 seconds now
Tested-by: Conor Dooley <[email protected]>

Thanks,
Conor.


Attachments:
(No filename) (1.13 kB)
signature.asc (235.00 B)
Download all attachments

2024-04-20 06:51:04

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] dt-bindings: kbuild: Split targets out to separate rules

On Sat, Apr 6, 2024 at 7:56 AM Rob Herring <[email protected]> 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 <[email protected]>
> Link: https://lore.kernel.org/all/20220817152027.16928-1-masahiroy@kernelorg/
> Signed-off-by: Rob Herring <[email protected]>
> ---
> 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

2024-04-22 17:53:29

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] dt-bindings: kbuild: Split targets out to separate rules

On Sat, Apr 20, 2024 at 1:50 AM Masahiro Yamada <[email protected]> wrote:
>
> On Sat, Apr 6, 2024 at 7:56 AM Rob Herring <[email protected]> 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?

Yes.

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

But the *only* reason to run dt_binding_check is to check the
bindings. When a schema is updated and we re-run the checks, *all* the
schemas are checked so you will get unrelated warnings as well. That's
because doing validation a file at a time is too slow. We could fix
that if there was a way to pass only the out of date dependencies, but
I didn't see a way to do that with make.

> 'make dt_binding_check' is already warning-free.

Right, so normally it won't re-run. If a developer introduces
warnings, then they are the only ones annoyed by this behavior and
that's what we want.

> So, I think it is OK to make it fail if any warning occurs.

Well, it has certainly gotten a lot better and we don't seem to end up
with last minute things breaking rc1, but I'm not sure I want to go
back to that yet. We started not erroring out because in the past I've
gotten broken schemas with the submitter saying they couldn't run the
checks because of errors. I must be in the minority that runs make
with --keep-going...

It is also not warning free sometimes with new versions of dtschema. I
usually fix those in parallel, but if anyone runs newer dtschema on
older kernels that doesn't help.

I suppose it would be better to keep the current behavior in this
series and make any changes on erroring out or re-running with
warnings a separate change.

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

yamllint has errors and warnings. errors exit with non-zero. There is
an option for warnings to return error too. Since we currently don't
distinguish, I'm not sure if our config is exactly the mix we'd want
to error out or not. I'll have to look and see before we change that.

>
> 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 <[email protected]>
> > Link: https://lore.kernel.org/all/[email protected]/
> > Signed-off-by: Rob Herring <[email protected]>
> > ---
> > 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.

I suppose that is only after your series on srctree/src because I
don't see that. But I will change it.

Rob

2024-04-23 09:35:11

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] dt-bindings: kbuild: Split targets out to separate rules

On Tue, Apr 23, 2024 at 2:46 AM Rob Herring <[email protected]> wrote:
>
> On Sat, Apr 20, 2024 at 1:50 AM Masahiro Yamada <masahiroy@kernelorg> wrote:
> >
> > On Sat, Apr 6, 2024 at 7:56 AM Rob Herring <[email protected]> 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?
>
> Yes.
>
> > I think it is annoying to re-run the same check
> > when none of the schemas is updated.
>
> But the *only* reason to run dt_binding_check is to check the
> bindings. When a schema is updated and we re-run the checks, *all* the
> schemas are checked so you will get unrelated warnings as well. That's
> because doing validation a file at a time is too slow. We could fix
> that if there was a way to pass only the out of date dependencies, but
> I didn't see a way to do that with make.
>
> > 'make dt_binding_check' is already warning-free.
>
> Right, so normally it won't re-run. If a developer introduces
> warnings, then they are the only ones annoyed by this behavior and
> that's what we want.
>
> > So, I think it is OK to make it fail if any warning occurs.
>
> Well, it has certainly gotten a lot better and we don't seem to end up
> with last minute things breaking rc1, but I'm not sure I want to go
> back to that yet. We started not erroring out because in the past I've
> gotten broken schemas with the submitter saying they couldn't run the
> checks because of errors. I must be in the minority that runs make
> with --keep-going...
>
> It is also not warning free sometimes with new versions of dtschema. I
> usually fix those in parallel, but if anyone runs newer dtschema on
> older kernels that doesn't help.
>
> I suppose it would be better to keep the current behavior in this
> series and make any changes on erroring out or re-running with
> warnings a separate change.
>
> > Besides, yamllint exists with 0 even if it finds a format error.
> > Hence "|| true" is not sensible.
>
> yamllint has errors and warnings. errors exit with non-zero. There is
> an option for warnings to return error too. Since we currently don't
> distinguish, I'm not sure if our config is exactly the mix we'd want
> to error out or not. I'll have to look and see before we change that.


OK, then.


I applied all of this series to linux-kbuild.
Thanks.










--
Best Regards
Masahiro Yamada