* Re: [RFC PATCH] Don't reset timestamps in include/generated if not needed [not found] <15447.1425431256@turing-police.cc.vt.edu> @ 2015-03-08 23:08 ` Linus Torvalds 2015-03-11 10:01 ` Michal Marek 0 siblings, 1 reply; 6+ messages in thread From: Linus Torvalds @ 2015-03-08 23:08 UTC (permalink / raw) To: Valdis Kletnieks, Michal Marek Cc: Andrew Morton, Linux Kernel Mailing List, Linux Kbuild mailing list 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. :) > > 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 kind of thing that just does $(call move-if-changed,build.h.tmp,build.h) Hmm? That move-if-changed can be useful for other cases too, I've seen something like it in a lot of makefiles. I'm surprised we don't have one already. That said, we already have that "filechk" define that does pretty much all of this. So I don't like the exact patch, but I do like the effect it gives. Michal? Linus ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] Don't reset timestamps in include/generated if not needed 2015-03-08 23:08 ` [RFC PATCH] Don't reset timestamps in include/generated if not needed Linus Torvalds @ 2015-03-11 10:01 ` Michal Marek 2015-03-13 4:59 ` Masahiro Yamada 0 siblings, 1 reply; 6+ messages in thread From: Michal Marek @ 2015-03-11 10:01 UTC (permalink / raw) To: Linus Torvalds Cc: Valdis Kletnieks, Andrew Morton, Linux Kernel Mailing List, Linux Kbuild mailing list 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. However, I can see that especially bounds.h ends up being included in many places, so cutting its dependencies helps in some cases. > > 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? 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] Don't reset timestamps in include/generated if not needed 2015-03-11 10:01 ` Michal Marek @ 2015-03-13 4:59 ` Masahiro Yamada 2015-03-13 10:00 ` Michal Marek 0 siblings, 1 reply; 6+ messages in thread From: Masahiro Yamada @ 2015-03-13 4:59 UTC (permalink / raw) To: Michal Marek Cc: Linus Torvalds, Valdis Kletnieks, Andrew Morton, Linux Kernel Mailing List, Linux Kbuild mailing list 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. -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] Don't reset timestamps in include/generated if not needed 2015-03-13 4:59 ` Masahiro Yamada @ 2015-03-13 10:00 ` Michal Marek 2015-03-24 15:36 ` [PATCH] kbuild: " Michal Marek 0 siblings, 1 reply; 6+ messages in thread From: Michal Marek @ 2015-03-13 10:00 UTC (permalink / raw) To: Masahiro Yamada Cc: Linus Torvalds, Valdis Kletnieks, Andrew Morton, Linux Kernel Mailing List, Linux Kbuild mailing list 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] kbuild: Don't reset timestamps in include/generated if not needed 2015-03-13 10:00 ` Michal Marek @ 2015-03-24 15:36 ` Michal Marek 2015-03-24 16:08 ` Valdis.Kletnieks 0 siblings, 1 reply; 6+ messages in thread From: Michal Marek @ 2015-03-24 15:36 UTC (permalink / raw) To: linux-kbuild; +Cc: Valdis Kletnieks, Masahiro Yamada Use filechk to generate asm-offsets.h and bounds.h. Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu> Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com> Signed-off-by: Michal Marek <mmarek@suse.cz> --- Kbuild | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/Kbuild b/Kbuild index ab8ded9..96d0629 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 ##### @@ -42,9 +43,8 @@ kernel/bounds.s: kernel/bounds.c FORCE $(Q)mkdir -p $(dir $@) $(call if_changed_dep,cc_s_c) -$(obj)/$(bounds-file): kernel/bounds.s Kbuild - $(Q)mkdir -p $(dir $@) - $(call cmd,offsets,__LINUX_BOUNDS_H__) +$(obj)/$(bounds-file): kernel/bounds.s FORCE + $(call filechk,offsets,__LINUX_BOUNDS_H__) ##### # 2) Generate asm-offsets.h @@ -62,8 +62,8 @@ arch/$(SRCARCH)/kernel/asm-offsets.s: arch/$(SRCARCH)/kernel/asm-offsets.c \ $(Q)mkdir -p $(dir $@) $(call if_changed_dep,cc_s_c) -$(obj)/$(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s Kbuild - $(call cmd,offsets,__ASM_OFFSETS_H__) +$(obj)/$(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s FORCE + $(call filechk,offsets,__ASM_OFFSETS_H__) ##### # 3) Check for missing system calls -- 2.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] kbuild: Don't reset timestamps in include/generated if not needed 2015-03-24 15:36 ` [PATCH] kbuild: " Michal Marek @ 2015-03-24 16:08 ` Valdis.Kletnieks 0 siblings, 0 replies; 6+ messages in thread From: Valdis.Kletnieks @ 2015-03-24 16:08 UTC (permalink / raw) To: Michal Marek; +Cc: linux-kbuild, Masahiro Yamada [-- Attachment #1: Type: text/plain, Size: 458 bytes --] On Tue, 24 Mar 2015 16:36:04 +0100, Michal Marek said: > Use filechk to generate asm-offsets.h and bounds.h. > > Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu> > Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com> > Signed-off-by: Michal Marek <mmarek@suse.cz> > --- > Kbuild | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) Different from my patch, but that's OK. Feel free to stick an Acked-By: on there. :) [-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-03-24 16:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <15447.1425431256@turing-police.cc.vt.edu>
2015-03-08 23:08 ` [RFC PATCH] Don't reset timestamps in include/generated if not needed Linus Torvalds
2015-03-11 10:01 ` Michal Marek
2015-03-13 4:59 ` Masahiro Yamada
2015-03-13 10:00 ` Michal Marek
2015-03-24 15:36 ` [PATCH] kbuild: " Michal Marek
2015-03-24 16:08 ` Valdis.Kletnieks
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox