[U-Boot] Buildman Kconfig issue with consecutive builds

[Schrempf Frieder]

Hi Simon,

On 07.11.19 17:23, Simon Glass wrote:
> Hi Schrempf,
> 
> On Thu, 7 Nov 2019 at 08:15, Schrempf Frieder
> <frieder.schrempf at kontron.de> wrote:
>>
>> On 07.11.19 15:02, Bin Meng wrote:
>>> Hi Frieder,
>>>
>>> On Thu, Nov 7, 2019 at 9:28 PM Schrempf Frieder
>>> <frieder.schrempf at kontron.de> wrote:
>>>>
>>>> Hi Bin,
>>>>
>>>> On 07.11.19 13:41, Bin Meng wrote:
>>>>> Hi Schrempf,
>>>>>
>>>>> On Thu, Nov 7, 2019 at 12:17 AM Schrempf Frieder
>>>>> <frieder.schrempf at kontron.de> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I'm having some trouble using buildman to test the impact of some
>>>>>> Kconfig cleanup patches ([1]).
>>>>>>
>>>>>> The patches introduce a new CONFIG_SPL_* option and I try to find out
>>>>>> which defconfigs need to be fixed, by comparing build sizes.
>>>>>>
>>>>>> Now when I added a patch to fix a defconfig I noticed that buildman
>>>>>> wouldn't report the expected size changes and upon looking more closely
>>>>>> I found that the added Kconfig options are still missing in u-boot-spl.cfg.
>>>>>>
>>>>>> The strange thing is, that when I try to build only the last commit then
>>>>>> the Kconfig options are there, which is why I suspect a bug in buildman
>>>>>> not handling Kconfig changes correctly with consecutive builds.
>>>>>>
>>>>>> Can anyone have a look what is wrong or how I can debug this issue?
>>>>>>
>>>>>> The issue can be reproduced with the branch at [1], running:
>>>>>>
>>>>>> buildman -b spi_flash_kconfig_cleanup_3 --step 0 xilinx_zynqmp_virt
>>>>>>
>>>>>
>>>>> Could you please add "-C" to the buildman command line and have a try?
>>>>
>>>> Indeed forcing the reconfig between the build steps with '-C' fixes the
>>>> issue.
>>>>
>>>> Is it a known problem, that buildman doesn't handle Kconfig changes
>>>> correctly without '-C' in some cases?
>>>
>>> AFAIK, this is an intended design of calling buildman w/o '-C' to save
>>> some build time.
>>
>> Ok, if that's the case I will try to come up with a patch that adds a
>> note to the README. This has cost me a few hours because I was thinking
>> buildman does the right thing and Kconfig options are messed up somewhere.
> 
> An incremental build means that it does not run 'make xxx_defconfig'
> on every commit. Doing it this way saves *a lot* of time for large
> builds and the main purpose of buildman is to validate that U-Boot
> builds.
> 
> However it might be possible to have it both ways...the code fragment
> below compares the Kconfig files and configs/ directory against the
> data of the 'u-boot' output file, and could trigger a full rebuild if
> newer.

Ok, thanks for the explanation.

> 
> If you have time (sounds like you do!), you could incorporate that
> into buildman.

It's kind of funny that you got the impression, that I have time ;)
Actually I do not have much time to work on U-Boot in general among all 
the other things.

And now I went deep down into the rabbit hole from "I want to get some 
boards upstreamed" to "I need to port a QSPI controller driver first" to 
"the driver port affects existing CONFIG options that are a total mess 
and need to be fixed" to "I need to run buildman on my cleanup patches" 
to "buildman could need some tweaking".

So unless there will be a lot of rainy weekends, I probably won't start 
working on optimizing buildman ;)

Regards,
Frieder

> 
> files = ['%s/u-boot' % outdir]
>    if os.path.exists(files[0]):
>      if options.incremental:
>        cmd = ['find', 'configs/', '-cnewer', files[0]]
>        result = cros_build_lib.RunCommand(cmd, capture_output=True, **kwargs)
>        if result.output:
>          logging.warning('config/ dir has changed - dropping -i')
>          options.incremental = False
> 
>      if options.incremental:
>        cmd = ['find', '.', '-name', 'Kconfig', '-and', '-cnewer', files[0]]
>        result = cros_build_lib.RunCommand(cmd, capture_output=True, **kwargs)
>        if result.output:
>          logging.warning('Kconfig file(s) changed - dropping -i')
>          options.incremental = False
> 
> 
> The current logic is in RunJob() and do_config is the thing that
> causes a reconfig.
> 
> Regards,
> Simon
>