[PATCH v2] Makefile: Fix lying comment re. silentoldconfig

[PATCH v2] Makefile: Fix lying comment re. silentoldconfig

[Ulf Magnusson]

The comment above the silentoldconfig invocation is outdated.
'make oldconfig' updates just .config and doesn't touch the
include/config/ tree.

This came up in https://lkml.org/lkml/2018/2/12/415.

While fixing the comment, make it more informative by explaining the
purpose of the unfortunately named silentoldconfig.

I can't make sense of the comment re. auto.conf.cmd and a cleaned tree.
include/config/auto.conf and include/config/auto.conf.cmd are both
created simultaneously by silentoldconfig (in
scripts/kconfig/confdata.c, by conf_write_autoconf()), and nothing seems
to remove auto.conf.cmd that wouldn't remove auto.conf. Remove that part
of the comment rather than blindly copying it. It might be a leftover
from an older way of doing things.

The include/config/auto.conf.cmd prerequisite might be there to ensure
that silentoldconfig gets rerun if conf_write_autoconf() fails between
writing out auto.conf.cmd and auto.conf (a comment in the function
indicates that auto.conf is deliberately written out last to mark
completion of the operation). It seems the Makefile dependency between
include/config/auto.conf and .config would already take care of that
though, since include/config/auto.conf would still be out of date re.
.config if the operation fails.

Cop out and leave the prerequisite in for now.

Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
---
Changes in v2:
include/config/auto.conf depends on .config, not the other way around, so swap
them in some places in the commit message to make it less confusing.

 Makefile | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 79ad2bfa24b6..61ed99ad4b1b 100644
--- a/Makefile
+++ b/Makefile
@@ -579,10 +579,13 @@ ifeq ($(KBUILD_EXTMOD),)
 # To avoid any implicit rule to kick in, define an empty command
 $(KCONFIG_CONFIG) include/config/auto.conf.cmd: ;
 
-# If .config is newer than include/config/auto.conf, someone tinkered
-# with it and forgot to run make oldconfig.
-# if auto.conf.cmd is missing then we are probably in a cleaned tree so
-# we execute the config step to be sure to catch updated Kconfig files
+# The actual configuration files used during the build are stored in
+# include/generated/ and include/config/. Update them if .config is newer than
+# include/config/auto.conf (which mirrors .config).
+#
+# The include/config/ tree manages dependencies between source files and
+# Kconfig symbols and lets us avoid doing a full rebuild whenever the
+# configuration is changed. See scripts/basic/fixdep.c
 include/config/%.conf: $(KCONFIG_CONFIG) include/config/auto.conf.cmd
 	$(Q)$(MAKE) -f $(srctree)/Makefile silentoldconfig
 else
-- 
2.14.1

Re: [PATCH v2] Makefile: Fix lying comment re. silentoldconfig

[Masahiro Yamada]

2018-02-13 16:58 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
> The comment above the silentoldconfig invocation is outdated.
> 'make oldconfig' updates just .config and doesn't touch the
> include/config/ tree.
>
> This came up in https://lkml.org/lkml/2018/2/12/415.
>
> While fixing the comment, make it more informative by explaining the
> purpose of the unfortunately named silentoldconfig.
>
> I can't make sense of the comment re. auto.conf.cmd and a cleaned tree.
> include/config/auto.conf and include/config/auto.conf.cmd are both
> created simultaneously by silentoldconfig (in
> scripts/kconfig/confdata.c, by conf_write_autoconf()), and nothing seems
> to remove auto.conf.cmd that wouldn't remove auto.conf. Remove that part
> of the comment rather than blindly copying it. It might be a leftover
> from an older way of doing things.
>
> The include/config/auto.conf.cmd prerequisite might be there to ensure
> that silentoldconfig gets rerun if conf_write_autoconf() fails between
> writing out auto.conf.cmd and auto.conf (a comment in the function
> indicates that auto.conf is deliberately written out last to mark
> completion of the operation). It seems the Makefile dependency between
> include/config/auto.conf and .config would already take care of that
> though, since include/config/auto.conf would still be out of date re.
> .config if the operation fails.
>
> Cop out and leave the prerequisite in for now.
>
> Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
> ---
> Changes in v2:
> include/config/auto.conf depends on .config, not the other way around, so swap
> them in some places in the commit message to make it less confusing.
>
>  Makefile | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 79ad2bfa24b6..61ed99ad4b1b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -579,10 +579,13 @@ ifeq ($(KBUILD_EXTMOD),)
>  # To avoid any implicit rule to kick in, define an empty command
>  $(KCONFIG_CONFIG) include/config/auto.conf.cmd: ;
>
> -# If .config is newer than include/config/auto.conf, someone tinkered
> -# with it and forgot to run make oldconfig.
> -# if auto.conf.cmd is missing then we are probably in a cleaned tree so
> -# we execute the config step to be sure to catch updated Kconfig files
> +# The actual configuration files used during the build are stored in
> +# include/generated/ and include/config/. Update them if .config is newer than
> +# include/config/auto.conf (which mirrors .config).


Yes, the first paragraph perfectly explains the code.


> +# The include/config/ tree manages dependencies between source files and
> +# Kconfig symbols and lets us avoid doing a full rebuild whenever the
> +# configuration is changed. See scripts/basic/fixdep.c

Do you insist on this paragraph?

I know silentoldconfig touches include/config/* for extra cleverness,
but, to me, this looks unnecessary information to
understand the following code.

>  include/config/%.conf: $(KCONFIG_CONFIG) include/config/auto.conf.cmd
>         $(Q)$(MAKE) -f $(srctree)/Makefile silentoldconfig
>  else




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] Makefile: Fix lying comment re. silentoldconfig

[Ulf Magnusson]

On Mon, Feb 19, 2018 at 12:37 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 2018-02-13 16:58 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
>> The comment above the silentoldconfig invocation is outdated.
>> 'make oldconfig' updates just .config and doesn't touch the
>> include/config/ tree.
>>
>> This came up in https://lkml.org/lkml/2018/2/12/415.
>>
>> While fixing the comment, make it more informative by explaining the
>> purpose of the unfortunately named silentoldconfig.
>>
>> I can't make sense of the comment re. auto.conf.cmd and a cleaned tree.
>> include/config/auto.conf and include/config/auto.conf.cmd are both
>> created simultaneously by silentoldconfig (in
>> scripts/kconfig/confdata.c, by conf_write_autoconf()), and nothing seems
>> to remove auto.conf.cmd that wouldn't remove auto.conf. Remove that part
>> of the comment rather than blindly copying it. It might be a leftover
>> from an older way of doing things.
>>
>> The include/config/auto.conf.cmd prerequisite might be there to ensure
>> that silentoldconfig gets rerun if conf_write_autoconf() fails between
>> writing out auto.conf.cmd and auto.conf (a comment in the function
>> indicates that auto.conf is deliberately written out last to mark
>> completion of the operation). It seems the Makefile dependency between
>> include/config/auto.conf and .config would already take care of that
>> though, since include/config/auto.conf would still be out of date re.
>> .config if the operation fails.
>>
>> Cop out and leave the prerequisite in for now.
>>
>> Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
>> ---
>> Changes in v2:
>> include/config/auto.conf depends on .config, not the other way around, so swap
>> them in some places in the commit message to make it less confusing.
>>
>>  Makefile | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 79ad2bfa24b6..61ed99ad4b1b 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -579,10 +579,13 @@ ifeq ($(KBUILD_EXTMOD),)
>>  # To avoid any implicit rule to kick in, define an empty command
>>  $(KCONFIG_CONFIG) include/config/auto.conf.cmd: ;
>>
>> -# If .config is newer than include/config/auto.conf, someone tinkered
>> -# with it and forgot to run make oldconfig.
>> -# if auto.conf.cmd is missing then we are probably in a cleaned tree so
>> -# we execute the config step to be sure to catch updated Kconfig files
>> +# The actual configuration files used during the build are stored in
>> +# include/generated/ and include/config/. Update them if .config is newer than
>> +# include/config/auto.conf (which mirrors .config).
>
>
> Yes, the first paragraph perfectly explains the code.
>
>
>> +# The include/config/ tree manages dependencies between source files and
>> +# Kconfig symbols and lets us avoid doing a full rebuild whenever the
>> +# configuration is changed. See scripts/basic/fixdep.c
>
> Do you insist on this paragraph?
>
> I know silentoldconfig touches include/config/* for extra cleverness,
> but, to me, this looks unnecessary information to
> understand the following code.
>
>>  include/config/%.conf: $(KCONFIG_CONFIG) include/config/auto.conf.cmd
>>         $(Q)$(MAKE) -f $(srctree)/Makefile silentoldconfig
>>  else

I think it's information that many people digging into the Makefiles
to figure out how the build works would find helpful, and my test is
more "is it helpful?" than "does it minimally describe just this
code?"

I'm not hardcore about it though. Remove it if you think it's
irrelevant or might become outdated. :)

Cheers,
Ulf

Re: [PATCH v2] Makefile: Fix lying comment re. silentoldconfig

[Masahiro Yamada]

2018-02-19 8:45 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
> On Mon, Feb 19, 2018 at 12:37 AM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> 2018-02-13 16:58 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
>>> The comment above the silentoldconfig invocation is outdated.
>>> 'make oldconfig' updates just .config and doesn't touch the
>>> include/config/ tree.
>>>
>>> This came up in https://lkml.org/lkml/2018/2/12/415.
>>>
>>> While fixing the comment, make it more informative by explaining the
>>> purpose of the unfortunately named silentoldconfig.
>>>
>>> I can't make sense of the comment re. auto.conf.cmd and a cleaned tree.
>>> include/config/auto.conf and include/config/auto.conf.cmd are both
>>> created simultaneously by silentoldconfig (in
>>> scripts/kconfig/confdata.c, by conf_write_autoconf()), and nothing seems
>>> to remove auto.conf.cmd that wouldn't remove auto.conf. Remove that part
>>> of the comment rather than blindly copying it. It might be a leftover
>>> from an older way of doing things.
>>>
>>> The include/config/auto.conf.cmd prerequisite might be there to ensure
>>> that silentoldconfig gets rerun if conf_write_autoconf() fails between
>>> writing out auto.conf.cmd and auto.conf (a comment in the function
>>> indicates that auto.conf is deliberately written out last to mark
>>> completion of the operation). It seems the Makefile dependency between
>>> include/config/auto.conf and .config would already take care of that
>>> though, since include/config/auto.conf would still be out of date re.
>>> .config if the operation fails.
>>>
>>> Cop out and leave the prerequisite in for now.
>>>
>>> Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
>>> ---
>>> Changes in v2:
>>> include/config/auto.conf depends on .config, not the other way around, so swap
>>> them in some places in the commit message to make it less confusing.
>>>
>>>  Makefile | 11 +++++++----
>>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 79ad2bfa24b6..61ed99ad4b1b 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -579,10 +579,13 @@ ifeq ($(KBUILD_EXTMOD),)
>>>  # To avoid any implicit rule to kick in, define an empty command
>>>  $(KCONFIG_CONFIG) include/config/auto.conf.cmd: ;
>>>
>>> -# If .config is newer than include/config/auto.conf, someone tinkered
>>> -# with it and forgot to run make oldconfig.
>>> -# if auto.conf.cmd is missing then we are probably in a cleaned tree so
>>> -# we execute the config step to be sure to catch updated Kconfig files
>>> +# The actual configuration files used during the build are stored in
>>> +# include/generated/ and include/config/. Update them if .config is newer than
>>> +# include/config/auto.conf (which mirrors .config).
>>
>>
>> Yes, the first paragraph perfectly explains the code.
>>
>>
>>> +# The include/config/ tree manages dependencies between source files and
>>> +# Kconfig symbols and lets us avoid doing a full rebuild whenever the
>>> +# configuration is changed. See scripts/basic/fixdep.c
>>
>> Do you insist on this paragraph?
>>
>> I know silentoldconfig touches include/config/* for extra cleverness,
>> but, to me, this looks unnecessary information to
>> understand the following code.
>>
>>>  include/config/%.conf: $(KCONFIG_CONFIG) include/config/auto.conf.cmd
>>>         $(Q)$(MAKE) -f $(srctree)/Makefile silentoldconfig
>>>  else
>
> I think it's information that many people digging into the Makefiles
> to figure out how the build works would find helpful, and my test is
> more "is it helpful?" than "does it minimally describe just this
> code?"
>
> I'm not hardcore about it though. Remove it if you think it's
> irrelevant or might become outdated. :)
>
> Cheers,
> Ulf
> --
> 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

Applied to linux-kbuild.

I removed the second paragraph due to my preference of brief comments.
:)


-- 
Best Regards
Masahiro Yamada