Message-ID: <ghpnzroa0g.fsf@lena.gouders.net>
Date:   Fri, 13 Jul 2018 09:44:15 +0200
From:   Dirk Gouders <dirk@...ders.net>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Ulf Magnusson <ulfalizer@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Sam Ravnborg <sam@...nborg.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 00/12] kbuild/kconfig: do not update config during installation

Masahiro Yamada <yamada.masahiro@...ionext.com> writes:

> 2018-07-10 20:34 GMT+09:00 Dirk Gouders <dirk@...ders.net>:
>> Masahiro Yamada <yamada.masahiro@...ionext.com> writes:
>>
>>> The main motivation of this patch series is to suppress the syncconfig
>>> during running installation targets.
>>>
>>> V1 consisted of only two patches:
>>>   https://patchwork.kernel.org/patch/10468105/
>>>   https://patchwork.kernel.org/patch/10468103/
>>>
>>> I noticed that installation targets would continue running
>>> even if the source tree is not configured at all
>>> because the inclusion of include/config/auto.conf was optional.
>>>
>>> So, I added one more patch in V2:
>>>  https://patchwork.kernel.org/patch/10483637/
>>>
>>> However, kbuild test robot reported a new warning message was displayed:
>>>
>>> Makefile:592: include/config/auto.conf: No such file or directory
>>>
>>> This warning is displayed only for Make 4.1 or older.
>>>
>>> To fix this annoying warning, I changed Kconfig too,
>>> which leaded to more clean-up, improvements in Kconfig.
>>>
>>> So, V3 is a big patch series.
>>
>> Hello Masahiro,
>>
>> I tested your series for a while, now.  I did not notice real issues
>> with it but want to leave some remarks about what I noticed in
>> the surroundings of your patches.
>>
>>
>>> Masahiro Yamada (12):
>>>   kconfig: rename file_write_dep and move it to confdata.c
>>
>> I might be missing some trivial use-case, but when looking at this
>> patch, I noticed an inconsistency with the file names auto.conf and
>> auto.conf.cmd.
>>
>> The first can be modified by an environment variable but when this
>> happens, auto.conf.cmd remains as is.  I noticed that only the
>> Documentation mentions that KCONFIG_AUTOCONFIG exists and confdata.c
>> uses it to serve the file name -- no other use anywhere.
>
> Indeed.
>
> I had also noticed this.
>
> Probably, replacing the hardcoded 'include/config/auto.conf.cmd'
> with  get_env('KCONFIG_AUTOCONFIG') + 'cmd' will be nice.
>
>
> I did not touch it since it thought it was less important
> for this patch set.
>
> If you are willing to contribute to this,
> a patch is welcome (after this series).

Yes, it is not that important but it would probably help people new to
kbuild/kconfig if we make this a bit more consistent.  I will send a
patch that also adds some words to the documentation of
KCONFIG_AUTOCONFIG.

>> Now, I am wondering if I just don't see an important case when the
>> use of KCONFIG_AUTOCONFIG is really helpful or even mandatory.
>
>
> I do not know the historical background,
> but I guess predecessors wanted to implement Kconfig
> as generic/flexible as possible.
>
>
>>
>>>   kconfig: split out helpers to check file/directory, create directory
>>>   kconfig: remove unneeded directory generation from local*config
>>>   kconfig: create directories needed for syncconfig by itself
>>>   kconfig: make syncconfig update .config regardless of sym_change_count
>>
>> For this patch, I already mentioned that `conf --help' perhaps could be
>> updated.
>
> What do you want 'conf --help' to look like ?

As I said, --help does not say a lot about which files are updated, so I
probably was too pedantic.  For --syncconfig it currently says:

  --syncconfig            Similar to oldconfig but generates configuration in
                          include/{generated/,config/}

which could let someone guess .config isn't touched (because of the
"but").  If you also think so, the text could perhaps say:

  --syncconfig            Similar to oldconfig but generates configuration in
                          include/{generated/,config/} in addition.

>
>>  On the other side, none of the entries there tells us such
>> details, so there is probably no need for syncconfig to do so.
>>
>>>   kconfig: allow all config targets to write auto.conf if missing
>>>   kbuild: use 'include' directive to load auto.conf from top Makefile
>>>   kbuild: add .DELETE_ON_ERROR special target
>>>   kbuild: do not update config when running install targets
>>>   kbuild: do not update config for 'make kernelrelease'
>>>   kbuild: remove auto.conf and tristate.conf from prerequisites
>>
>> In the surrounding of this patch I noticed -include of auto.conf and
>> tristate.conf in scripts/Makfile.modbuildin.  I tried it in some ways
>> but was not able to trigger that file being used with a missing
>> auto.conf.
>
> Right.  auto.conf and tristate.conf are mandatory there.
>
> '-include' should be replaced with 'include'.
>
> Cater to send a patch?

I will do.

Dirk

>
>> On the other hand, if I now manually remove tristate.conf,
>> that would not be fixed or even noticed, because of -include and I
>> wonder if it is safer to also change the -includes in that file.
>>
>> It seems, if one of those files is missing, one must have done it
>> manually or some other serious issue is present that we probably want to
>> notice.
>
> You are right.  If somebody removes tristate.conf on purpose,
> it is not self-healing.
>
> I should be fixed by itself, or at least it should fail
> with clear message.
>
> I will reconsider this.
>
>
> Thanks.
>
>
>
>> Dirk
>>
>>>   kbuild: replace include/config/%.conf with include/config/auto.conf
>>>
>>>  Makefile                    |  46 +++++++++------
>>>  scripts/Kbuild.include      |   3 +
>>>  scripts/kconfig/Makefile    |  16 ++---
>>>  scripts/kconfig/conf.c      |  39 +++++++------
>>>  scripts/kconfig/confdata.c  | 139 +++++++++++++++++++++++++++++++++++++-------
>>>  scripts/kconfig/gconf.c     |   1 +
>>>  scripts/kconfig/lkc.h       |   1 -
>>>  scripts/kconfig/lkc_proto.h |   2 +-
>>>  scripts/kconfig/mconf.c     |   1 +
>>>  scripts/kconfig/nconf.c     |   1 +
>>>  scripts/kconfig/qconf.cc    |   2 +
>>>  scripts/kconfig/util.c      |  30 ----------
>>>  12 files changed, 182 insertions(+), 99 deletions(-)
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists