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
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 <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: [email protected]
Signed-off-by: Rob Herring <[email protected]>
---
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
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 <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: [email protected]
Cc: "H. Peter Anvin" <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Michal Marek <[email protected]>
Signed-off-by: Rob Herring <[email protected]>
---
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
The old-atomics and missing-syscalls targets are not files, so they
should be marked as PHONY.
Cc: Masahiro Yamada <[email protected]>
Cc: Michal Marek <[email protected]>
Signed-off-by: Rob Herring <[email protected]>
---
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
On Tue, Feb 23, 2021 at 10:14 AM Rob Herring <[email protected]> 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
On Tue, Feb 23, 2021 at 5:20 PM Linus Torvalds
<[email protected]> wrote:
>
> On Tue, Feb 23, 2021 at 10:14 AM Rob Herring <[email protected]> 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
On Wed, Feb 24, 2021 at 3:14 AM Rob Herring <[email protected]> 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 <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: [email protected]
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Michal Marek <[email protected]>
> Signed-off-by: Rob Herring <[email protected]>
> ---
> 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 <[email protected]>
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
On Tue, Feb 23, 2021 at 6:47 PM Masahiro Yamada <[email protected]> wrote:
>
> On Wed, Feb 24, 2021 at 3:14 AM Rob Herring <[email protected]> wrote:
> >
> > The old-atomics and missing-syscalls targets are not files, so they
> > should be marked as PHONY.
> >
> > Cc: Masahiro Yamada <[email protected]>
> > Cc: Michal Marek <[email protected]>
> > Signed-off-by: Rob Herring <[email protected]>
> > ---
> > 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
On Wed, Feb 24, 2021 at 8:59 AM Rob Herring <[email protected]> wrote:
>
> On Tue, Feb 23, 2021 at 5:20 PM Linus Torvalds
> <[email protected]> wrote:
> >
> > On Tue, Feb 23, 2021 at 10:14 AM Rob Herring <[email protected]> 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
On Wed, Feb 24, 2021 at 3:14 AM Rob Herring <[email protected]> wrote:
>
> The old-atomics and missing-syscalls targets are not files, so they
> should be marked as PHONY.
>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Michal Marek <[email protected]>
> Signed-off-by: Rob Herring <[email protected]>
> ---
> 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
Hi Rob,
On Tue, Feb 23, 2021 at 7:18 PM Rob Herring <[email protected]> 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 <[email protected]>
> Cc: Michal Marek <[email protected]>
> Cc: [email protected]
> Signed-off-by: Rob Herring <[email protected]>
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 -- [email protected]
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