[U-Boot] [PATCH] tools/genboardscfg.py: improve performance more by using Kconfiglib

[Masahiro Yamada]

Hi Simon,



On Sun, 31 Aug 2014 21:52:41 -0700
Simon Glass <sjg at chromium.org> wrote:

> One thing is that buildman doesn't really need the boards.cfg file.
> After all it just reads it in board.py. If you make it so that
> genboardscfg.py can be a library that returns a dictionary, then
> buildman can just import it and use the data directly.

Yes, that's exactly what I was thinking.

This tool should be moved (and renamed) into tools/buildman/ directory
and become a library imported from Buildman, but I think it should be done
after MAKEALL is deleted.

(Maybe can we delete it round at the end of this year?)

After that, board.py will get much simpler or perhaps go away eventually.




> The license looks fine to me - you can probably use a short form like this:
> 
> https://spdx.org/licenses/ISC
> 
> Some nits below.
> 
> I think you should include Kconfiglib in a separate patch rather than
> people having to install it.
> 

OK.  Everyone will feel happy about that as long as the license is OK.



> 
> I think this should be formatted like this:
> 
> > +    _SYMBOL_TABLE = {
> > +            'arch' : 'SYS_ARCH',
> > +            'cpu' : 'SYS_CPU',
> > +            'soc' : 'SYS_SOC',
> > +            'vendor' : 'SYS_VENDOR',
> > +            'board' : 'SYS_BOARD',
> > +            'config' : 'SYS_CONFIG_NAME',
> > +            'options' : 'SYS_EXTRA_OPTIONS'
> > +    }


Fixed in v2.



> 
> Nice to keep the title on one line, e.g.:
> 
> """Load a defconfig file to obtain board parameters

Fixed in v2.



> How about:
> 
> """Scan a defconfig file and queue its board parameters

Instead I used
"Scan defconfig files and queue their board parameters"
because this function takes multiple defconfig files.



> This is shorter:
> 
> """Collect board parameters for all defconfig files

Fixed in v2.



> > +    total = len(all_defconfigs)
> 
> Would total_boards be a better name?

Fixed in v2.


> > +    processes = []
> > +    queues = []
> > +    for i in range(jobs):
> > +        defconfigs = all_defconfigs[total * i / jobs : total * (i + 1) /jobs]
> 
> / jobs
> (add space)

Fixed in v2.


> >          """Return the status of the given board.
> >
> >          Returns:
> > -          Either 'Active' or 'Orphan'
> > +          Either 'Active' or 'Orphan' usually.
> > +          Return '-' displaying a warning if something strange happens.
> 
> This refers to behaviour rather than return value, so should probably
> go above the 'Returns' section.

Fixed in v2.


> > -        This method parses MAINTAINERS and add board status and
> > -        maintainers information to the database.
> > +        This method parses the given MAINTAINERS file and accumulates
> > +        board status and maintainers information.
> >
> 
> s/This method parses/Parse/


Fixed in v2.



> > +def format_and_output(params_list, output):
> > +    """Columnate the board parameters, sort lines alphabetically,
> > +    and then write them to a file.
> 
> Most of this could go into the description - the title could be shorter.

Fixed in v2.



> >  def main():
> >      parser = optparse.OptionParser()
> >      # Add options here
> > -    parser.add_option('-j', '--jobs',
> > -                      help='the number of jobs to run simultaneously')
> >      parser.add_option('-f', '--force', action="store_true", default=False,
> >                        help='regenerate the output even if it is new')
> > +    parser.add_option('-j', '--jobs',
> > +                      help='the number of jobs to run simultaneously')
> 
> How about:
> 
>    parser.add_option('-j', '--jobs', type='int',
> default=multiprocessing.cpu_count().
>                       help='the number of jobs to run simultaneously')


I did not this function. This is much simpler.
Great!



> and avoid the checking below.
> 
> > +    parser.add_option('-o', '--output',
> > +                      help='output file [default=%s]' % OUTPUT_FILE)
> 
>     parser.add_option('-o', '--output', default=OUTPUT_FILE,
>                       help='output file')
> 
> and hopefully avoid the if...then below.

Removed if...then.
I left 'output file [default=boards.cfg]' just in case.



> >      (options, args) = parser.parse_args()
> >
> >      if options.jobs:
> >          try:
> >              jobs = int(options.jobs)
> >          except ValueError:
> > -            sys.exit('Option -j (--jobs) takes a number')
> > +            sys.exit('Option -j (--jobs) takes an integer')
> > +        if jobs <= 0:
> > +            sys.exit('Option -j (--jobs) takes a positive integer')
> >      else:
> >          try:
> >              jobs = int(subprocess.Popen(['getconf', '_NPROCESSORS_ONLN'],
> > -                                     stdout=subprocess.PIPE).communicate()[0])
> > +                                      stdout=subprocess.PIPE).communicate()[0])
> 
> Does multiprocessing.cpu_count() work?


Yes it worked, but I added try ... except  just in case.



Your comments are very helpful and appreciated. Thanks!!


Best Regards
Masahiro Yamada