Message ID | 20210223181425.4010665-4-robh@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Build time gitignore checking | expand |
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
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
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
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 --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),\
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(+)