kbuild: Split up DT binding build targets

PATCH In review Rob Herring 2 participants 3 messages Aug 24, 2022 – Aug 29, 2022

Subsystems (3)

Device tree bindings Kbuild Device tree

lore

Share & tools

lore

Rob Herring

author maintainer

2022-08-24 20:39 UTC #0 lore raw

PATCH 1/1 kbuild: Split up DT binding build targets

The DT binding validation target, dt_binding_check, is composed of multiple steps which can't be run individually. This resulted in the passing of make variables to control which steps were run for 'dtbs_check'. Some steps are also doing multiple things in a single rule which is error prone[1].

Rework the build to split each of the steps into its own make target. This allows users more fine grained control over what's run and makes for easier make dependencies. The new targets are:

dt_binding_lint - Runs yamllint on the bindings dt_binding_schemas - Validates the binding schemas dt_binding_examples - Builds and validates the binding examples

As before, each can be limited by setting DT_SCHEMA_FILES to a match file pattern (sub-string).

This also has the side effect of enabling validation of %.dtb targets by specifying 'CHECK_DTBS=y' on the command line.

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

Cc Masahiro Yamada <[email protected]>

Cc Dmitry Baryshkov <[email protected]>

Cc Marijn Suijten <[email protected]>

Signed-off-by Rob Herring <[email protected]>

---
 Documentation/devicetree/bindings/Makefile | 42 ++++++++++++++--------
 Makefile                                   | 28 +++++++++------
 scripts/Makefile.lib                       |  2 +-
 scripts/dtc/Makefile                       |  2 +-
 4 files changed, 46 insertions(+), 28 deletions(-)

Documentation/devicetree/bindings/Makefile +27 ~~-15~~

index 1eaccf135b30..1ac3f47b854a 100644
@@ -29,16 +29,28 @@ find_all_cmd = find $(srctree)/$(src) \( -name '*.yaml' ! \
 		-name 'processed-schema*' \)
 
 find_cmd = $(find_all_cmd) | grep -F "$(DT_SCHEMA_FILES)"
-CHK_DT_DOCS := $(shell $(find_cmd))
+CHK_DT_DOCS := $(patsubst $(srctree)/%,%,$(shell $(find_cmd)))
 
 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) || true; \
+                     touch $@
 
-quiet_cmd_chk_bindings = CHKDT   $@
+dt_binding_lint: $(obj)/dt_binding_lint.checked
+
+$(obj)/dt_binding_lint.checked: $(CHK_DT_DOCS) $(src)/.yamllint FORCE
+	$(if $(DT_SCHEMA_LINT),$(call if_changed,yamllint),)
+
+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)) || true; \
+                         touch $@
+
+dt_binding_schemas: $(obj)/dt_binding_schemas.checked
+
+$(obj)/dt_binding_schemas.checked: $(CHK_DT_DOCS) check_dtschema_version FORCE
+	$(call if_changed,chk_bindings)
 
 quiet_cmd_mk_schema = SCHEMA  $@
       cmd_mk_schema = f=$$(mktemp) ; \

@@ -46,14 +58,13 @@ 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)))
 
+dt_binding_processed_schema: $(obj)/processed-schema.json
+
+$(obj)/processed-schema.json: $(DT_DOCS) check_dtschema_version FORCE
+	$(call if_changed,mk_schema)
+
 override DTC_FLAGS := \
 	-Wno-avoid_unnecessary_addr_size \
 	-Wno-graph_child_address \

@@ -64,12 +75,13 @@ override DTC_FLAGS := \
 # Disable undocumented compatible checks until warning free
 override DT_CHECKER_FLAGS ?=
 
-$(obj)/processed-schema.json: $(DT_DOCS) $(src)/.yamllint check_dtschema_version FORCE
-	$(call if_changed_rule,chkdt)
+dt_binding_examples: $(obj)/processed-schema.json $(patsubst %.yaml,%.example.dtb, $(CHK_DT_DOCS))
+
+dt_binding_check: dt_binding_lint dt_binding_examples dt_binding_schemas
 
-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-y += dt_binding_lint.checked dt_binding_schemas.checked processed-schema.json
+always-$(CHECK_DTBS) += $(patsubst $(src)/%.yaml,%.example.dts, $(CHK_DT_DOCS))
+always-$(CHECK_DTBS) += $(patsubst $(src)/%.yaml,%.example.dtb, $(CHK_DT_DOCS))
 
 # Hack: avoid 'Argument list too long' error for 'make clean'. Remove most of
 # build artifacts here before they are processed by scripts/Makefile.clean

Makefile +17 ~~-11~~

index c7705f749601..db456a58a823 100644
@@ -1391,7 +1391,10 @@ dtbs_prepare: include/config/kernel.release scripts_dtc
 
 ifneq ($(filter dtbs_check, $(MAKECMDGOALS)),)
 export CHECK_DTBS=y
-dtbs: dt_binding_check
+endif
+
+ifeq ($(CHECK_DTBS),y)
+dtbs: dt_binding_processed_schema
 endif
 
 dtbs_check: dtbs

@@ -1409,13 +1412,13 @@ PHONY += scripts_dtc
 scripts_dtc: scripts_basic
 	$(Q)$(MAKE) $(build)=scripts/dtc
 
-ifneq ($(filter dt_binding_check, $(MAKECMDGOALS)),)
-export CHECK_DT_BINDING=y
+DT_BINDING_TARGETS := dt_binding_check dt_binding_lint dt_binding_schemas dt_binding_examples dt_binding_processed_schema
+PHONY += $(DT_BINDING_TARGETS)
+ifneq ($(filter dt_binding_examples dt_binding_check, $(MAKECMDGOALS)),)
+export CHECK_DTBS=y
 endif
-
-PHONY += dt_binding_check
-dt_binding_check: scripts_dtc
-	$(Q)$(MAKE) $(build)=Documentation/devicetree/bindings
+$(DT_BINDING_TARGETS): scripts_dtc
+	$(Q)$(MAKE) $(build)=Documentation/devicetree/bindings $@
 
 # ---------------------------------------------------------------------------
 # Modules

@@ -1625,10 +1628,13 @@ 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 '  dtbs_check          - Validate device tree source files';\
+		echo '  dt_binding_check    - Validate device tree binding documents and examples'; \
+		echo '  dt_binding_schemas  - Validate device tree binding documents'; \
+		echo '  dt_binding_examples - Validate device tree binding examples'; \
+		echo '  dt_binding_lint     - Run yamllint on device tree binding documents'; \
 		echo '')
 
 	@echo 'Userspace tools targets:'

scripts/Makefile.lib +1 -1

index 3fb6a99e78c4..2a9901377e57 100644
@@ -365,7 +365,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

scripts/dtc/Makefile +1 -1

index 4d32b9497da9..593af5d4e19c 100644
@@ -3,7 +3,7 @@
 
 # *** Also keep .gitignore in sync when changing ***
 hostprogs-always-$(CONFIG_DTC)		+= dtc fdtoverlay
-hostprogs-always-$(CHECK_DT_BINDING)	+= dtc
+hostprogs-always-$(CHECK_DTBS)	+= dtc
 
 dtc-objs	:= dtc.o flattree.o fstree.o data.o livetree.o treesource.o \
 		   srcpos.o checks.o util.o

Signature · 1 line

2.34.1

Masahiro Yamada

2022-08-26 19:31 UTC #1 lore raw

Quoted 10 lines · Rob Herring

The DT binding validation target, dt_binding_check, is composed of multiple steps which can't be run individually. This resulted in the passing of make variables to control which steps were run for 'dtbs_check'. Some steps are also doing multiple things in a single rule which is error prone[1].
Rework the build to split each of the steps into its own make target. This allows users more fine grained control over what's run and makes for easier make dependencies.

I do not think it makes the code easier.

A tricky case is that multiple targets run in parallel.

"make -j$(nproc) dtbs_check dt_binding_examples"

Two different threads dive into Documentation/devicetree/bindings/Makefile, and try to build the same file simultaneously.

If you run the command above, you will see two lines of

SCHEMA Documentation/devicetree/bindings/processed-schema.json

processed-schema.json may result in a corrupted file.

Quoted 5 lines

The new targets are:
dt_binding_lint - Runs yamllint on the bindings dt_binding_schemas - Validates the binding schemas dt_binding_examples - Builds and validates the binding examples

I still do not understand why so many phony targets are necessary.

Why isn't the change as simple as the attached file?

Signature · 2 lines

Best Regards
Masahiro Yamada

Documentation/devicetree/bindings/Makefile +20 ~~-16~~

index 1eaccf135b30..428eb01d2fc2 100644
@@ -3,9 +3,6 @@ DT_DOC_CHECKER ?= dt-doc-validate
 DT_EXTRACT_EX ?= dt-extract-example
 DT_MK_SCHEMA ?= dt-mk-schema
 
-DT_SCHEMA_LINT = $(shell which yamllint || \
-  echo "warning: python package 'yamllint' not installed, skipping" >&2)
-
 DT_SCHEMA_MIN_VERSION = 2022.3
 
 PHONY += check_dtschema_version

@@ -32,13 +29,25 @@ find_cmd = $(find_all_cmd) | grep -F "$(DT_SCHEMA_FILES)"
 CHK_DT_DOCS := $(shell $(find_cmd))
 
 quiet_cmd_yamllint = LINT    $(src)
-      cmd_yamllint = ($(find_cmd) | \
+      cmd_yamllint = if ! command -v yamllint >/dev/null; then \
+                         echo "warning: python package 'yamllint' not installed, skipping" >&2; \
+                         exit 0; \
+                     fi; \
+                     ($(find_cmd) | \
                      xargs -n200 -P$$(nproc) \
-		     $(DT_SCHEMA_LINT) -f parsable -c $(srctree)/$(src)/.yamllint >&2) || true
+                     yamllint -f parsable -c $(srctree)/$(src)/.yamllint >&2) || true; \
+                     touch $@
+
+$(obj)/dt_binding_lint.checked: $(CHK_DT_DOCS) $(src)/.yamllint FORCE
+	$(call if_changed,yamllint)
 
-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)) || true; \
+                         touch $@
+
+$(obj)/dt_binding_schemas.checked: $(CHK_DT_DOCS) check_dtschema_version FORCE
+	$(call if_changed,chk_bindings)
 
 quiet_cmd_mk_schema = SCHEMA  $@
       cmd_mk_schema = f=$$(mktemp) ; \

@@ -46,14 +55,11 @@ 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)))
 
+$(obj)/processed-schema.json: $(DT_DOCS) check_dtschema_version FORCE
+	$(call if_changed,mk_schema)
+
 override DTC_FLAGS := \
 	-Wno-avoid_unnecessary_addr_size \
 	-Wno-graph_child_address \

@@ -64,10 +70,8 @@ override DTC_FLAGS := \
 # Disable undocumented compatible checks until warning free
 override DT_CHECKER_FLAGS ?=
 
-$(obj)/processed-schema.json: $(DT_DOCS) $(src)/.yamllint check_dtschema_version FORCE
-	$(call if_changed_rule,chkdt)
-
 always-y += processed-schema.json
+always-$(CHECK_DT_BINDING) += dt_binding_lint.checked dt_binding_schemas.checked
 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))

Rob Herring

author maintainer

2022-08-29 01:07 UTC #2 lore raw

Quoted 23 lines · Masahiro Yamada

Quoted 10 lines · Rob Herring
The DT binding validation target, dt_binding_check, is composed of multiple steps which can't be run individually. This resulted in the passing of make variables to control which steps were run for 'dtbs_check'. Some steps are also doing multiple things in a single rule which is error prone[1].
Rework the build to split each of the steps into its own make target. This allows users more fine grained control over what's run and makes for easier make dependencies.
I do not think it makes the code easier.
A tricky case is that multiple targets run in parallel.
"make -j$(nproc) dtbs_check dt_binding_examples"

Quoted 8 lines

Two different threads dive into Documentation/devicetree/bindings/Makefile, and try to build the same file simultaneously.
If you run the command above, you will see two lines of
SCHEMA Documentation/devicetree/bindings/processed-schema.json
processed-schema.json may result in a corrupted file.

Indeed... :(

Quoted 9 lines

Quoted 6 lines
The new targets are:
dt_binding_lint - Runs yamllint on the bindings dt_binding_schemas - Validates the binding schemas dt_binding_examples - Builds and validates the binding examples
I still do not understand why so many phony targets are necessary.

I thought that's what you were suggesting, but I guess you meant just separate internal targets. Separate targets exposed to the user are useful as well. I've had some requests to be able to skip running yamllint for example. The processed schema can be used for a few other tools now, so being able to just build it is useful.

Rob