linux-kernel - Re: [PATCH v2 2/3] dt-bindings: kbuild: Split targets out to separate rules

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL_JsqKiwtVwD6snXqyEuHTx7CB2aZcLGxo7xVL9pVD0qC+8qA@mail.gmail.com>
Date: Mon, 22 Apr 2024 12:46:33 -0500
From: Rob Herring <robh@...nel.org>
To: Masahiro Yamada <masahiroy@...nel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Nathan Chancellor <nathan@...nel.org>, Nicolas Schier <nicolas@...sle.eu>, 
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, 
	Marijn Suijten <marijn.suijten@...ainline.org>, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org
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 <masahiroy@...nel.org> wrote:
>
> On Sat, Apr 6, 2024 at 7:56 AM Rob Herring <robh@...nel.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@...nel.org>
> > Link: https://lore.kernel.org/all/20220817152027.16928-1-masahiroy@kernel.org/
> > Signed-off-by: Rob Herring <robh@...nel.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