diff mbox series

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

Message ID 20210223181425.4010665-4-robh@kernel.org (mailing list archive)
State New
Headers show
Series Build time gitignore checking | expand

Commit Message

Rob Herring Feb. 23, 2021, 6:14 p.m. UTC
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(+)

Comments

Linus Torvalds Feb. 23, 2021, 11:19 p.m. UTC | #1
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
Rob Herring Feb. 23, 2021, 11:59 p.m. UTC | #2
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
Masahiro Yamada Feb. 24, 2021, 1:14 a.m. UTC | #3
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
Geert Uytterhoeven Feb. 24, 2021, 11:26 a.m. UTC | #4
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
diff mbox series

Patch

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),\