[U-Boot] [PATCH v5 09/15] tools: add genboardscfg.py

[Masahiro Yamada]

Hi Simon,



On Sat, 26 Jul 2014 01:17:02 +0100
Simon Glass <sjg at chromium.org> wrote:

> > +
> > +import sys
> > +import os
> > +import errno
> > +import shutil
> > +import time
> > +import subprocess
> > +import fnmatch
> > +import glob
> > +import re
> > +import optparse
> 
> You can sort the imports.


Sorted alphabetially in v6.

Pardon my ignorance, but is there any coding style guide
about sorting imports?



> > +
> > +### helper functions ###
> > +def get_terminal_columns():
> > +    '''
> > +    Get the width of the terminal
> > +    Return 0 if the stdout is not associated with tty.
> > +    '''
> > +    try:
> > +        return shutil.get_terminal_size().columns # Python 3.3~
> > +    except:
> > +        import fcntl
> > +        import termios
> > +        import struct
> > +        arg = struct.pack('hhhh', 0, 0, 0, 0)
> > +        try:
> > +            ret = fcntl.ioctl(sys.stdout.fileno(), termios.TIOCGWINSZ, arg)
> 
> Perhaps could use sys.version_info here and below?


I think it will also work. But we should exactly know which version support
the new method.

Isn't it easier to use "try: ... except: ..." statement ? 

 
> Also I hesitate to suggest this, but there is a terminal.py library in
> patman. Should we consider moving that into a common directory and
> putting all terminal-related code there?


I do not see the advantage to do that for now.
The terminal.py library has only Color class and this tool does not need it.
(We could display warning messages in red text, but it would not be mandatory.)


> > +### classes ###
> > +class MaintainersDatabase:
> > +    '''The database of board status and maintainers'''
> > +    def __init__(self):
> > +        '''Create an empty database'''
> > +        self.database = {}
> 
> Suggest a blank line after this and other functions.


Done in v6.


> > +    def get_status(self, target):
> > +        '''
> > +        Return the status of the given board
> 
> You can put the description on the same line immediately after the '''


Fixed in v6.


> > +        Arguments:
> > +          file - MAINTAINERS file to be parsed
> > +        '''
> > +        targets = []
> > +        maintainers = []
> > +        status = '-'
> > +        for line in open(file):
> > +            if line[:2] == 'M:':
> 
> Perhaps put line[:2] in a variable like 'tag'?

Good suggestion!
I use  (tag, rest) instead of (line[:2], line[2:])

                             (field, defconfig)
> > +                sys.exit(1)
> > +        # fix-up for aarch64 and tegra
> > +        if fields['arch'] == 'arm' and 'cpu' in fields:
> > +            if fields['cpu'] == 'armv8':
> > +                fields['arch'] = 'aarch64'
> 
> Is this always true? Do we not support aarch32 yet?


It is true at least for now.





> > +            if 'soc' in fields and re.match('tegra[0-9]*$', fields['soc']):
> > +                fields['cpu'] += ':arm720t'
> 
> This is unfortunate, but necessary I think.


Actually not necessary anymore.

SPLCPU is defined in Kconfig.

I just focused on generating equivalent boards.cfg in this patch.

I will drop this in a follow-up patch.

(The difference is that
"tools/buildman/buildman arm720t" will not build Tegra boards.)




> > +class Slot:
> > +    '''
> > +    Subprocess slot
> 
> Slot?

Fixed in v6.


> > +
> > +    Each instance of this class handles one subprocess.
> > +    This class is useful to control multiple processes of
> > +    "make <board>_defconfig" for faster processing.
> > +
> > +    Private members:
> > +      occupied - Show if this slot is ocuppied or not.
> 
> To keep consistent with patman, use a : instead of - after occupied.
> Same for other functions.

OK.
Fixed both this patch and scripts/multiconfig.py in 06/15.


> > +class Indicator:
> > +    '''
> > +    A class to control the progress indicator
> > +
> > +    Private members:
> > +      total - A number of boards to be processed
> > +      cur - The current counter
> > +      width - The width of the prograss bar
> > +      enabled - Show the progress bar only when this flag is True
> > +    '''
> > +    def __init__(self, total):
> > +        '''
> > +        Create a instance getting the width of the terminal
> 
> Out of date?
> 

Comments were fixed.



Thanks for your review!


Best Regards
Masahiro Yamada