[U-Boot] [PATCH 1/2] configs: Resync with savedefconfig

[Stephen Warren]

On 09/12/2016 08:23 AM, Tom Rini wrote:
> On Mon, Sep 12, 2016 at 11:24:35AM +0900, Masahiro Yamada wrote:
>> 2016-09-10 7:25 GMT+09:00 Stephen Warren <swarren at wwwdotorg.org>:
>>> On 09/09/2016 03:14 PM, Tom Rini wrote:
>>>>
>>>> On Fri, Sep 09, 2016 at 03:06:01PM -0600, Stephen Warren wrote:
>>>>>
>>>>> On 09/09/2016 01:11 PM, Tom Rini wrote:
>>>>>>
>>>>>> On Fri, Sep 09, 2016 at 01:09:43PM -0600, Stephen Warren wrote:
>>>>>>>
>>>>>>> On 09/09/2016 01:02 PM, Tom Rini wrote:
>>>>>>>>
>>>>>>>> On Fri, Sep 09, 2016 at 10:21:45AM -0600, Stephen Warren wrote:
>>>>>>>>>
>>>>>>>>> On 09/08/2016 07:19 PM, Tom Rini wrote:
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Tom Rini <trini at konsulko.com>
>>>>>>>>
>>>>>>>> /bin/bash: ess: command not found
>>>>>>>>>>
>>>>>>>>>> diff --git a/configs/harmony_defconfig b/configs/harmony_defconfig
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> -CONFIG_USE_PRIVATE_LIBGCC=y
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I assume that's because =y is the default for that now?
>>>>>>>>
>>>>>>>>
>>>>>>>> Yes.
>>>>>>>
>>>>>>>
>>>>>>>>>> diff --git a/configs/p2771-0000-500_defconfig
>>>>>>>>>> b/configs/p2771-0000-500_defconfig
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> -CONFIG_TARGET_P2771_0000=y
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I think we need to keep those two. Those two defconfigs are slightly
>>>>>>>>> different configurations of U-Boot's p2771-0000 board/target.
>>>>>>>>
>>>>>>>>
>>>>>>>> OK, then you need to submit a patch to fix the underlying problem, if
>>>>>>>> there is one, really.  Keep in mind that what this is, is re-running
>>>>>>>> savedefconfig for each target.  So if something odd drops out that
>>>>>>>> means
>>>>>>>> that it wasn't having any effect before.
>>>>>>>
>>>>>>>
>>>>>>> I don't believe there is any underlying problem. The Kconfig option
>>>>>>> in question is defined in arch/arm/mach-tegra/tegra186/Kconfig, when
>>>>>>> building for p2771-0000-000 the option makes it into .config just
>>>>>>> fine, and the value is used by board/nvidia/p2771-0000/Kconfig.
>>>>>>> Isn't this a bug in savedefconfig? I'm CC'ing Masahiro to get some
>>>>>>> insight.
>>>>>>
>>>>>>
>>>>>> It is the default value then and isn't saved is I believe the answer.
>>>>>
>>>>>
>>>>> I don't think it's the default; nothing selects that option and it
>>>>> has no "default y".
>>>>>
>>>>> Or maybe since the value is inside a choice, and is the only entry
>>>>> that's there, that does make it the default? If so, that seems a
>>>>> little fragile; what if someone comes along and adds a new board
>>>>> into the list, and puts it before the existing entry (e.g. to
>>>>> maintain alphabetical sorting). That would changing the meaning of
>>>>> the current defconfig file if the first entry is picked as default.
>>>>> If so, I'm surprised if nobody has had that issue yet.
>>>>
>>>>
>>>> It's all correct and I think we have had this hit once or twice when
>>>> adding or changing the order.  It is a side effect of how Kconfig just
>>>> works.
>>>
>>>
>>> I'm not convinced. If I apply your patch, and also the following:
>>>
>>>> diff --git a/arch/arm/mach-tegra/tegra186/Kconfig
>>>> b/arch/arm/mach-tegra/tegra186/Kconfig
>>>> index 97cf23f31f80..05f691ed3ede 100644
>>>> --- a/arch/arm/mach-tegra/tegra186/Kconfig
>>>> +++ b/arch/arm/mach-tegra/tegra186/Kconfig
>>>> @@ -7,6 +7,14 @@ if TEGRA186
>>>>  choice
>>>>         prompt "Tegra186 board select"
>>>>
>>>> +config TARGET_E9999_1234
>>>> +       bool "NVIDIA Tegra186 E9999-1234 board"
>>>> +       help
>>>> +         P2771-0000 is a P3310 CPU board married to a P2597 I/O board.
>>>> The
>>>> +         combination contains SoC, DRAM, eMMC, SD card slot, HDMI, USB
>>>> +         micro-B port, Ethernet, USB3 host port, SATA, PCIe, and two GPIO
>>>> +         expansion headers.
>>>> +
>>>>  config TARGET_P2771_0000
>>>>         bool "NVIDIA Tegra186 P2771-0000 board"
>>>>         help
>>>
>>>
>>> ... then I get a build failure for p2771-0000-000_defconfig since .config
>>> ends up containing CONFIG_TARGET_E9999_1234 rather than
>>> CONFIG_TARGET_P2771_0000. I think we want to avoid that don't we? If we
>>> don't, savedefconfig in the face of choice options seems rather too fragile,
>>> or we need to force people to update *_defconfig any time any Kconfig choice
>>> is edited.
>>
>>
>> We have discussed this problem several times before.
>>
>>   - The first entry in a "choice" is the default, unless otherwise specified.
>>   - "make savedefconfig" drops all the defaults to create a minimum
>> set of configs.
>>
>> So, we need to be careful when we insert a new entry at the first
>> or delete the first entry in a "choice".
>>
>> We are supposed to not re-sync defconfigs in Linux Kernel, so we are not hit
>> by this issue much there, but we still be careful when we edit the
>> first entry of a choice.
>> This potential fragileness exists in any projects that use Kconfig.
>>
>>
>> In U-Boot, unfortunately, we need defconfig resync periodically.
>>
>> As you noticed, "update *_defconfig any time any Kconfig choice is edited."
>> is one solution.
>>
>>
>> Or, you can specify a default explicitly, like I do in
>> arch/arm/mach-uniphier/Kconfig:
>>
>>
>> choice
>>         prompt "UniPhier SoC select"
>>         default ARCH_UNIPHIER_PRO4
>
> I am of the opinion, at least currently, that we need to make wider use
> of "default X" in choice statements and that configs/ should be treated
> as generated files.  I am frankly surprised that the Kernel stopped
> doing this because I would have sworn that wasn't true in the past (one
> shouldn't push unrelated "cleanups" to defconfig files but you also
> better put stuff where savedefconfig puts things as otherwise it would
> be the old time conflict hell madness of pre-Kconfig days).

Can we modify Kconfig in U-Boot (maybe upstream too) to require a 
default value for choices? Doing this by code review is likely to miss some.