[U-Boot] Buildman Kconfig issue with consecutive builds

[U-Boot] Buildman Kconfig issue with consecutive builds

[Schrempf Frieder]

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

for a build where the added Kconfig options are missing in the resulting 
u-boot-spl.cfg.

And:

buildman -b spi_flash_kconfig_cleanup_3^..spi_flash_kconfig_cleanup_3 
xilinx_zynqmp_virt

for a build of only the last commit with expected output.

Thanks,
Frieder

[1]: https://github.com/fschrempf/u-boot/commits/spi_flash_kconfig_cleanup_3

[U-Boot] Buildman Kconfig issue with consecutive builds

[Schrempf Frieder]

On 06.11.19 17:16, Frieder Schrempf 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

Forgot to mention that my master is set to 
32d870a82203a16a6e05958e2a02287af4dd825a (which is not upstream) in this 
case.

> 
> for a build where the added Kconfig options are missing in the resulting 
> u-boot-spl.cfg.
> 
> And:
> 
> buildman -b spi_flash_kconfig_cleanup_3^..spi_flash_kconfig_cleanup_3 
> xilinx_zynqmp_virt
> 
> for a build of only the last commit with expected output.
> 
> Thanks,
> Frieder
> 
> [1]: 
> https://github.com/fschrempf/u-boot/commits/spi_flash_kconfig_cleanup_3

[U-Boot] Buildman Kconfig issue with consecutive builds

[Bin Meng]

Hi Schrempf,

On Thu, Nov 7, 2019 at 12:17 AM Schrempf Frieder
<frieder.schrempf@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?

> for a build where the added Kconfig options are missing in the resulting
> u-boot-spl.cfg.
>
> And:
>
> buildman -b spi_flash_kconfig_cleanup_3^..spi_flash_kconfig_cleanup_3
> xilinx_zynqmp_virt
>
> for a build of only the last commit with expected output.
>
> Thanks,
> Frieder
>
> [1]: https://github.com/fschrempf/u-boot/commits/spi_flash_kconfig_cleanup_3

Regards,
Bin

[U-Boot] Buildman Kconfig issue with consecutive builds

[Schrempf Frieder]

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@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?

Thanks,
Frieder

> 
>> for a build where the added Kconfig options are missing in the resulting
>> u-boot-spl.cfg.
>>
>> And:
>>
>> buildman -b spi_flash_kconfig_cleanup_3^..spi_flash_kconfig_cleanup_3
>> xilinx_zynqmp_virt
>>
>> for a build of only the last commit with expected output.
>>
>> Thanks,
>> Frieder
>>
>> [1]: https://github.com/fschrempf/u-boot/commits/spi_flash_kconfig_cleanup_3
> 
> Regards,
> Bin
> 

[U-Boot] Buildman Kconfig issue with consecutive builds

[Bin Meng]

Hi Frieder,

On Thu, Nov 7, 2019 at 9:28 PM Schrempf Frieder
<frieder.schrempf@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@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.

Regards,
Bin

[U-Boot] Buildman Kconfig issue with consecutive builds

[Schrempf Frieder]

On 07.11.19 15:02, Bin Meng wrote:
> Hi Frieder,
> 
> On Thu, Nov 7, 2019 at 9:28 PM Schrempf Frieder
> <frieder.schrempf@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@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.

[U-Boot] Buildman Kconfig issue with consecutive builds

[Simon Glass]

Hi Schrempf,

On Thu, 7 Nov 2019 at 08:15, Schrempf Frieder
<frieder.schrempf@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@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@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.

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

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

[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@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@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@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
> 

[U-Boot] Buildman Kconfig issue with consecutive builds

[Simon Glass]

Hi Schrempf,

On Thu, 7 Nov 2019 at 12:14, Schrempf Frieder <frieder.schrempf@kontron.de>
wrote:

> 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@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@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@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 ;)
>

May you be plagued with downpours :-)

- Simon