| Message ID | 20150311100101.GA30787@sepie.suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi. 2015-03-11 19:01 GMT+09:00 Michal Marek <mmarek@suse.cz>: > On Sun, Mar 08, 2015 at 04:08:06PM -0700, Linus Torvalds wrote: >> On Tue, Mar 3, 2015 at 5:07 PM, Valdis Kletnieks >> <Valdis.Kletnieks@vt.edu> wrote: >> > >> > Kbuild regenerates bounds.h and asm-offsets.h, resetting the timestamps >> > and forcing rebuilds even if the contents haven't changed. Add a bit of >> > shell magic to only replace the file if the contents have in fact changed, >> > which should speed up git bisects and similar. >> > ... >> > RFC because I can't wrap my head around why this wasn't done ages ago. >> > If I'm missing something something obvious, please apply a cluestick. :) > > We do not regenerate the files *always*, but only if bounds.c / > asm-offsets.c or one of the headers they include change. So the rule is > not any worse than the rule for compiling a regular .c file into an .o > file. You can use ccache to avoid repeated rebuilds. Right. > However, I can see that especially bounds.h ends up being included in > many places, so cutting its dependencies helps in some cases. Agreed. > >> > Lightly tested - if I rm one of those two files, it gets rebuilt. If I >> > insert some whitespace, it gets replaced. If I don't touch it, the datestamp >> > doesn't change. >> >> I don't think this is wrong, but I'd really prefer to do the whole >> move-if-changed thing as a separate rule, and a separate command. >> >> IOW, could we please have something that generates the "build.h" file >> by separately creating a new temporary file, and then having the >> traditional kind of move-if-changed definition >> >> define move-if-changed >> if ! cmp -s $(1) $(2); then mv -f $(1) $(2); else rm -f $(1); fi >> endef > > We already have it and it is called "filechk." Valdis, can you check if > the below patch works equally well for you? This looks almost nice, but a few comments below. > diff --git a/Kbuild b/Kbuild > index ab8ded9..47d0630 100644 > --- a/Kbuild > +++ b/Kbuild > @@ -13,8 +13,9 @@ define sed-y > s:->::; p;}" > endef > > -quiet_cmd_offsets = GEN $@ > -define cmd_offsets > +# Use filechk to avoid rebuilds when a header changes, but the resulting file > +# does not > +define filechk_offsets > (set -e; \ > echo "#ifndef $2"; \ > echo "#define $2"; \ > @@ -24,9 +25,9 @@ define cmd_offsets > echo " * This file was generated by Kbuild"; \ > echo " */"; \ > echo ""; \ > - sed -ne $(sed-y) $<; \ > + sed -ne $(sed-y); \ > echo ""; \ > - echo "#endif" ) > $@ > + echo "#endif" ) > endef > > ##### > @@ -44,7 +45,7 @@ kernel/bounds.s: kernel/bounds.c FORCE > > $(obj)/$(bounds-file): kernel/bounds.s Kbuild You are checking the resulting file content, so the dependency on "Kbuild" is not necessary. Instead, you need to add "FORCE" so that this rule is always invoked. > $(Q)mkdir -p $(dir $@) You can drop this line because filechk automatically creates the output directory. > - $(call cmd,offsets,__LINUX_BOUNDS_H__) > + $(call filechk,offsets,__LINUX_BOUNDS_H__) To sum up, I want this part to look like this: $(obj)/$(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s FORCE $(call cmd,filechk,__ASM_OFFSETS_H__) > ##### > # 2) Generate asm-offsets.h > @@ -63,7 +64,7 @@ arch/$(SRCARCH)/kernel/asm-offsets.s: arch/$(SRCARCH)/kernel/asm-offsets.c \ > $(call if_changed_dep,cc_s_c) > > $(obj)/$(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s Kbuild > - $(call cmd,offsets,__ASM_OFFSETS_H__) > + $(call filechk,offsets,__ASM_OFFSETS_H__) Ditto.
Dne 13.3.2015 v 05:59 Masahiro Yamada napsal(a): > 2015-03-11 19:01 GMT+09:00 Michal Marek <mmarek@suse.cz>: >> We already have it and it is called "filechk." Valdis, can you check if >> the below patch works equally well for you? > > This looks almost nice, but a few comments below. Thanks for the review! >> $(obj)/$(bounds-file): kernel/bounds.s Kbuild > > You are checking the resulting file content, > so the dependency on "Kbuild" is not necessary. > > Instead, you need to add "FORCE" so that this rule is always invoked. Good point. >> $(Q)mkdir -p $(dir $@) > > You can drop this line because filechk automatically creates the > output directory. Likewise. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Kbuild b/Kbuild index ab8ded9..47d0630 100644 --- a/Kbuild +++ b/Kbuild @@ -13,8 +13,9 @@ define sed-y s:->::; p;}" endef -quiet_cmd_offsets = GEN $@ -define cmd_offsets +# Use filechk to avoid rebuilds when a header changes, but the resulting file +# does not +define filechk_offsets (set -e; \ echo "#ifndef $2"; \ echo "#define $2"; \ @@ -24,9 +25,9 @@ define cmd_offsets echo " * This file was generated by Kbuild"; \ echo " */"; \ echo ""; \ - sed -ne $(sed-y) $<; \ + sed -ne $(sed-y); \ echo ""; \ - echo "#endif" ) > $@ + echo "#endif" ) endef ##### @@ -44,7 +45,7 @@ kernel/bounds.s: kernel/bounds.c FORCE $(obj)/$(bounds-file): kernel/bounds.s Kbuild $(Q)mkdir -p $(dir $@) - $(call cmd,offsets,__LINUX_BOUNDS_H__) + $(call filechk,offsets,__LINUX_BOUNDS_H__) ##### # 2) Generate asm-offsets.h @@ -63,7 +64,7 @@ arch/$(SRCARCH)/kernel/asm-offsets.s: arch/$(SRCARCH)/kernel/asm-offsets.c \ $(call if_changed_dep,cc_s_c) $(obj)/$(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s Kbuild - $(call cmd,offsets,__ASM_OFFSETS_H__) + $(call filechk,offsets,__ASM_OFFSETS_H__) ##### # 3) Check for missing system calls