[PATCH 0/3] Build time gitignore checking

[PATCH 0/3] Build time gitignore checking

[Rob Herring]

Linus wasn't happy that I forgot a gitignore entry in my recent PR.
Checking that requires doing in tree build (which is not my usual
workflow) and checking git status afterwards. Given either one I'll
easily forget again, I came up with a build time check which works for
in tree and out of tree builds. It should also show up in any CI builds.

The first 2 patches are fixes which the 3rd patch found. They can be
picked up regardless of whether folks like the 3rd patch or not.

Rob


Rob Herring (3):
  kbuild: Make old-atomics and missing-syscalls phony targets
  x86: Drop generated syscall headers from 'targets'
  kbuild: Add a build check for missing gitignore entries

 Kbuild                           | 2 ++
 arch/x86/entry/syscalls/Makefile | 2 --
 scripts/Makefile.lib             | 4 ++++
 3 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.27.0

[PATCH 1/3] kbuild: Make old-atomics and missing-syscalls phony targets

[Rob Herring]

The old-atomics and missing-syscalls targets are not files, so they
should be marked as PHONY.

Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Michal Marek <michal.lkml@markovi.net>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 Kbuild | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Kbuild b/Kbuild
index fa441b98c9f6..032157c3ffd2 100644
--- a/Kbuild
+++ b/Kbuild
@@ -44,6 +44,7 @@ always-y += missing-syscalls
 quiet_cmd_syscalls = CALL    $<
       cmd_syscalls = $(CONFIG_SHELL) $< $(CC) $(c_flags) $(missing_syscalls_flags)
 
+PHONY += missing-syscalls
 missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE
 	$(call cmd,syscalls)
 
@@ -55,5 +56,6 @@ always-y += old-atomics
 quiet_cmd_atomics = CALL    $<
       cmd_atomics = $(CONFIG_SHELL) $<
 
+PHONY += old-atomics
 old-atomics: scripts/atomic/check-atomics.sh FORCE
 	$(call cmd,atomics)
-- 
2.27.0

Re: [PATCH 1/3] kbuild: Make old-atomics and missing-syscalls phony targets

[Masahiro Yamada]

On Wed, Feb 24, 2021 at 3:14 AM Rob Herring <robh@kernel.org> wrote:
>
> The old-atomics and missing-syscalls targets are not files, so they
> should be marked as PHONY.
>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Michal Marek <michal.lkml@markovi.net>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  Kbuild | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Kbuild b/Kbuild
> index fa441b98c9f6..032157c3ffd2 100644
> --- a/Kbuild
> +++ b/Kbuild
> @@ -44,6 +44,7 @@ always-y += missing-syscalls
>  quiet_cmd_syscalls = CALL    $<
>        cmd_syscalls = $(CONFIG_SHELL) $< $(CC) $(c_flags) $(missing_syscalls_flags)
>
> +PHONY += missing-syscalls
>  missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE
>         $(call cmd,syscalls)
>
> @@ -55,5 +56,6 @@ always-y += old-atomics
>  quiet_cmd_atomics = CALL    $<
>        cmd_atomics = $(CONFIG_SHELL) $<
>
> +PHONY += old-atomics


I do not think this is the right fix.

always-y (specified a few lines above) adds $(obj)/ prefix,
and is not supposed to work with PHONY.


It is wrong to blindly eliminate
the errors detected by your 3/3



>  old-atomics: scripts/atomic/check-atomics.sh FORCE
>         $(call cmd,atomics)
> --
> 2.27.0
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/3] kbuild: Make old-atomics and missing-syscalls phony targets

[Rob Herring]

On Tue, Feb 23, 2021 at 6:47 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Wed, Feb 24, 2021 at 3:14 AM Rob Herring <robh@kernel.org> wrote:
> >
> > The old-atomics and missing-syscalls targets are not files, so they
> > should be marked as PHONY.
> >
> > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > Cc: Michal Marek <michal.lkml@markovi.net>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> >  Kbuild | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/Kbuild b/Kbuild
> > index fa441b98c9f6..032157c3ffd2 100644
> > --- a/Kbuild
> > +++ b/Kbuild
> > @@ -44,6 +44,7 @@ always-y += missing-syscalls
> >  quiet_cmd_syscalls = CALL    $<
> >        cmd_syscalls = $(CONFIG_SHELL) $< $(CC) $(c_flags) $(missing_syscalls_flags)
> >
> > +PHONY += missing-syscalls
> >  missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE
> >         $(call cmd,syscalls)
> >
> > @@ -55,5 +56,6 @@ always-y += old-atomics
> >  quiet_cmd_atomics = CALL    $<
> >        cmd_atomics = $(CONFIG_SHELL) $<
> >
> > +PHONY += old-atomics
>
>
> I do not think this is the right fix.
>
> always-y (specified a few lines above) adds $(obj)/ prefix,
> and is not supposed to work with PHONY.
>
>
> It is wrong to blindly eliminate
> the errors detected by your 3/3

What about checking just hostprogs and userprogs? That's what I
initially had, but thought we could widen the net a little.

Rob

[PATCH 2/3] x86: Drop generated syscall headers from 'targets'

[Rob Herring]

Including the generated syscall headers in 'targets' is wrong because they
are not built in $(obj)/ and the Makefile does its own path prefix and
build rules.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Michal Marek <michal.lkml@markovi.net>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 arch/x86/entry/syscalls/Makefile | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/entry/syscalls/Makefile b/arch/x86/entry/syscalls/Makefile
index 6fb9b57ed5ba..b0dcb7e41554 100644
--- a/arch/x86/entry/syscalls/Makefile
+++ b/arch/x86/entry/syscalls/Makefile
@@ -62,8 +62,6 @@ syshdr-$(CONFIG_X86_64)		+= unistd_32_ia32.h unistd_64_x32.h
 syshdr-$(CONFIG_X86_64)		+= syscalls_64.h
 syshdr-$(CONFIG_XEN)		+= xen-hypercalls.h
 
-targets	+= $(uapisyshdr-y) $(syshdr-y)
-
 PHONY += all
 all: $(addprefix $(uapi)/,$(uapisyshdr-y))
 all: $(addprefix $(out)/,$(syshdr-y))
-- 
2.27.0

Re: [PATCH 2/3] x86: Drop generated syscall headers from 'targets'

[Masahiro Yamada]

On Wed, Feb 24, 2021 at 3:14 AM Rob Herring <robh@kernel.org> wrote:
>
> Including the generated syscall headers in 'targets' is wrong because they
> are not built in $(obj)/ and the Makefile does its own path prefix and
> build rules.
>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: x86@kernel.org
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Michal Marek <michal.lkml@markovi.net>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  arch/x86/entry/syscalls/Makefile | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/arch/x86/entry/syscalls/Makefile b/arch/x86/entry/syscalls/Makefile
> index 6fb9b57ed5ba..b0dcb7e41554 100644
> --- a/arch/x86/entry/syscalls/Makefile
> +++ b/arch/x86/entry/syscalls/Makefile
> @@ -62,8 +62,6 @@ syshdr-$(CONFIG_X86_64)               += unistd_32_ia32.h unistd_64_x32.h
>  syshdr-$(CONFIG_X86_64)                += syscalls_64.h
>  syshdr-$(CONFIG_XEN)           += xen-hypercalls.h
>
> -targets        += $(uapisyshdr-y) $(syshdr-y)
> -

This is also a wrong fix.

The correct fix exists in linux-next.

commit 865fa29f7dd1b6af8498fe08f19b4028c1c8a153
Author: Masahiro Yamada <masahiroy@kernel.org>
Date:   Mon Feb 15 09:48:22 2021 +0900

    arch: syscalls: add missing FORCE and fix 'targets' to make if_changed work


I will send a PR this week.



>  PHONY += all
>  all: $(addprefix $(uapi)/,$(uapisyshdr-y))
>  all: $(addprefix $(out)/,$(syshdr-y))
> --
> 2.27.0
>


-- 
Best Regards
Masahiro Yamada

[PATCH 3/3] kbuild: Add a build check for missing gitignore entries

[Rob Herring]

Any non-phony targets need to be in gitignore. The normal way to check
this is doing an in-tree build and running git-status which is easy to
miss. Git provides an easy way to check whether a file is ignored with
git-check-ignore. Let's add a build time check using it. If the build is
not in a git tree, the check will silently fail.

This also has the side effect of a sanity check for 'always-y',
'extra-y' and 'targets' entries which are not correctly marked as PHONY
or have the wrong path.

Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: linux-kbuild@vger.kernel.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
 scripts/Makefile.lib | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index b00855b247e0..84ac8b74bbe9 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -103,6 +103,10 @@ real-obj-m	:= $(addprefix $(obj)/,$(real-obj-m))
 multi-used-m	:= $(addprefix $(obj)/,$(multi-used-m))
 subdir-ym	:= $(addprefix $(obj)/,$(subdir-ym))
 
+$(foreach f, $(filter-out $(patsubst %,$(obj)/%,$(PHONY)),$(extra-y) $(always-y) $(targets)), \
+	$(if $(shell git -C $(srctree) check-ignore -q $(f) 2> /dev/null || echo $(f)), \
+		$(warning $(f) is missing gitignore entry)))
+
 # Finds the multi-part object the current object will be linked into.
 # If the object belongs to two or more multi-part objects, list them all.
 modname-multi = $(sort $(foreach m,$(multi-used),\
-- 
2.27.0

Re: [PATCH 3/3] kbuild: Add a build check for missing gitignore entries

[Linus Torvalds]

On Tue, Feb 23, 2021 at 10:14 AM Rob Herring <robh@kernel.org> wrote:
>
> Any non-phony targets need to be in gitignore. The normal way to check
> this is doing an in-tree build and running git-status which is easy to
> miss. Git provides an easy way to check whether a file is ignored with
> git-check-ignore. Let's add a build time check using it.

This looks ridiculously expensive with a shell and git invocation for
every single target just for this check.

Considering that I just had to fight my build suddenly getting much
slower, I'm a bit sensitive about these things.

          Linus

Re: [PATCH 3/3] kbuild: Add a build check for missing gitignore entries

[Rob Herring]

On Tue, Feb 23, 2021 at 5:20 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, Feb 23, 2021 at 10:14 AM Rob Herring <robh@kernel.org> wrote:
> >
> > Any non-phony targets need to be in gitignore. The normal way to check
> > this is doing an in-tree build and running git-status which is easy to
> > miss. Git provides an easy way to check whether a file is ignored with
> > git-check-ignore. Let's add a build time check using it.
>
> This looks ridiculously expensive with a shell and git invocation for
> every single target just for this check.

I was a bit worried too initially, but casually didn't notice any
difference so I didn't do any measurements. Now I have, and it looks
like it adds about 2 sec on a rebuild with no changes. I probably can
rework it to a single shell and git call per invocation of
Makefile.lib. What I really need is git-check-ignore to take '-n'
without '-v', but grep can solve that.

Here's the raw data:

clean x86 defconfig:
1805.08user 165.87system 5:05.15elapsed 645%CPU (0avgtext+0avgdata
260180maxresident)k
110536inputs+1390704outputs (11major+52491225minor)pagefaults 0swaps

rebuild with no changes:
12.61user 3.56system 0:04.32elapsed 374%CPU (0avgtext+0avgdata
38876maxresident)k
0inputs+1984outputs (0major+755708minor)pagefaults 0swaps

adding this commit and rebuild:
14.90user 4.80system 0:06.50elapsed 303%CPU (0avgtext+0avgdata
39160maxresident)k
80inputs+1992outputs (0major+1402830minor)pagefaults 0swaps

clean x86 defconfig with this commit:
1799.10user 165.84system 5:06.19elapsed 641%CPU (0avgtext+0avgdata
259932maxresident)k
8inputs+1390712outputs (0major+53146757minor)pagefaults 0swaps

another rebuild with this commit:
14.55user 4.85system 0:06.14elapsed 315%CPU (0avgtext+0avgdata
38664maxresident)k
0inputs+1992outputs (0major+1402878minor)pagefaults 0swaps

Rob

Re: [PATCH 3/3] kbuild: Add a build check for missing gitignore entries

[Masahiro Yamada]

On Wed, Feb 24, 2021 at 8:59 AM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Feb 23, 2021 at 5:20 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Tue, Feb 23, 2021 at 10:14 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > Any non-phony targets need to be in gitignore. The normal way to check
> > > this is doing an in-tree build and running git-status which is easy to
> > > miss. Git provides an easy way to check whether a file is ignored with
> > > git-check-ignore. Let's add a build time check using it.
> >
> > This looks ridiculously expensive with a shell and git invocation for
> > every single target just for this check.



I run "git status" in my usual development flow,
and my eyes eventually catch non-tracked files, if any.

As a fact, Linus noticed the untracked fdtoverlay soon
(but not soon enough to push back the pull request).

So, I am not convinced with doing this in the build time.
This is ugly and expensive.

Maybe we can ask Intel's 0day bot team to run "git status"
after the build test.
(Or, we can ask Stephen Rothwell to do this.)


As it turns out, this detects more than just missed .gitignore addition,
but looking at 1/3 and 2/3, people cannot fix the issues properly,
rather just try to blindly suppress the warnings to make the code even worse.








> I was a bit worried too initially, but casually didn't notice any
> difference so I didn't do any measurements. Now I have, and it looks
> like it adds about 2 sec on a rebuild with no changes. I probably can
> rework it to a single shell and git call per invocation of
> Makefile.lib. What I really need is git-check-ignore to take '-n'
> without '-v', but grep can solve that.
>
> Here's the raw data:
>
> clean x86 defconfig:
> 1805.08user 165.87system 5:05.15elapsed 645%CPU (0avgtext+0avgdata
> 260180maxresident)k
> 110536inputs+1390704outputs (11major+52491225minor)pagefaults 0swaps
>
> rebuild with no changes:
> 12.61user 3.56system 0:04.32elapsed 374%CPU (0avgtext+0avgdata
> 38876maxresident)k
> 0inputs+1984outputs (0major+755708minor)pagefaults 0swaps
>
> adding this commit and rebuild:
> 14.90user 4.80system 0:06.50elapsed 303%CPU (0avgtext+0avgdata
> 39160maxresident)k
> 80inputs+1992outputs (0major+1402830minor)pagefaults 0swaps
>
> clean x86 defconfig with this commit:
> 1799.10user 165.84system 5:06.19elapsed 641%CPU (0avgtext+0avgdata
> 259932maxresident)k
> 8inputs+1390712outputs (0major+53146757minor)pagefaults 0swaps
>
> another rebuild with this commit:
> 14.55user 4.85system 0:06.14elapsed 315%CPU (0avgtext+0avgdata
> 38664maxresident)k
> 0inputs+1992outputs (0major+1402878minor)pagefaults 0swaps
>
> Rob



--
Best Regards
Masahiro Yamada

Re: [PATCH 3/3] kbuild: Add a build check for missing gitignore entries

[Geert Uytterhoeven]

Hi Rob,

On Tue, Feb 23, 2021 at 7:18 PM Rob Herring <robh@kernel.org> wrote:
> Any non-phony targets need to be in gitignore. The normal way to check
> this is doing an in-tree build and running git-status which is easy to
> miss. Git provides an easy way to check whether a file is ignored with
> git-check-ignore. Let's add a build time check using it. If the build is
> not in a git tree, the check will silently fail.
>
> This also has the side effect of a sanity check for 'always-y',
> 'extra-y' and 'targets' entries which are not correctly marked as PHONY
> or have the wrong path.
>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Michal Marek <michal.lkml@markovi.net>
> Cc: linux-kbuild@vger.kernel.org
> Signed-off-by: Rob Herring <robh@kernel.org>

Thanks for your patch!

After removing the .git directory from my repository clone, or
removing the git command from $PATH:

    scripts/Makefile.lib:106: scripts/basic/fixdep is missing gitignore entry
    scripts/Makefile.lib:106: scripts/sorttable is missing gitignore entry
    [...]

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds