| Message ID | 20240405-dt-kbuild-rework-v2-2-3a035caee357@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dt-bindings: kbuild: Rework build rules and dependencies | expand |
On Sat, Apr 6, 2024 at 7:56 AM 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 = $(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/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
On Sat, Apr 20, 2024 at 1:50 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Sat, Apr 6, 2024 at 7:56 AM 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? 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 <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/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
On Tue, Apr 23, 2024 at 2:46 AM Rob Herring <robh@kernel.org> wrote: > > On Sat, Apr 20, 2024 at 1:50 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > On Sat, Apr 6, 2024 at 7:56 AM 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? > > 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.
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))
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 <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(-)