Re: [RFC PATCH] Don't reset timestamps in include/generated if not needed

[Masahiro Yamada <yamada.masahiro@socionext.com>]

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