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

The idea of using Kconfiglib was given by Tom Rini.
It allows us to scan lots of defconfigs very quickly.
This commit also uses multiprocessing for further acceleration.
You will be able to generate boards.cfg in a few seconds.

Kconfiglib must be installed in advance to try this commit.

Fetch and install it as follows:

  $ git clone git://github.com/ulfalizer/Kconfiglib.git
  $ cd Kconfiglib
  # python setup.py install

You need to be root for the last step.
(Or you can copy kconfiglib.py to tools/ directory)

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
Suggested-by: Tom Rini <trini@ti.com>
---

My concern is License.

U-Boot is distributed under GPL, whereas Kconfiglib claims
ISC License.
(See https://github.com/ulfalizer/Kconfiglib)

I do not know if we can import ISC-licensed code to U-boot,
so I did not include Kconfiglib in this patch.

I am not familiar the license issues.
Comments please!


 tools/genboardscfg.py | 696 +++++++++++++++++++-------------------------------
 1 file changed, 266 insertions(+), 430 deletions(-)

Hi Masahiro,

On 31 August 2014 10:23, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> The idea of using Kconfiglib was given by Tom Rini.
> It allows us to scan lots of defconfigs very quickly.
> This commit also uses multiprocessing for further acceleration.
> You will be able to generate boards.cfg in a few seconds.
>
> Kconfiglib must be installed in advance to try this commit.
>
> Fetch and install it as follows:
>
>   $ git clone git://github.com/ulfalizer/Kconfiglib.git
>   $ cd Kconfiglib
>   # python setup.py install
>
> You need to be root for the last step.
> (Or you can copy kconfiglib.py to tools/ directory)
>
> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
> Suggested-by: Tom Rini <trini@ti.com>
> ---
>
> My concern is License.
>
> U-Boot is distributed under GPL, whereas Kconfiglib claims
> ISC License.
> (See https://github.com/ulfalizer/Kconfiglib)
>
> I do not know if we can import ISC-licensed code to U-boot,
> so I did not include Kconfiglib in this patch.
>
> I am not familiar the license issues.
> Comments please!

You must be trying for a speed record? This is really really fast now
- 1 second on my desktop!

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.

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.


>
>
>  tools/genboardscfg.py | 696 +++++++++++++++++++-------------------------------
>  1 file changed, 266 insertions(+), 430 deletions(-)
>
> diff --git a/tools/genboardscfg.py b/tools/genboardscfg.py
> index e6870f5..0c709c5 100755
> --- a/tools/genboardscfg.py
> +++ b/tools/genboardscfg.py
> @@ -6,34 +6,48 @@
>  #
>
>  """
> -Converter from Kconfig and MAINTAINERS to boards.cfg
> +Converter from Kconfig and MAINTAINERS to a board database.
>
> -Run 'tools/genboardscfg.py' to create boards.cfg file.
> +Run 'tools/genboardscfg.py' to create a board database.
>
>  Run 'tools/genboardscfg.py -h' for available options.
>
> -This script only works on python 2.6 or later, but not python 3.x.
> +Python 2.6 or later, but not Python 3.x is necessary to run this script.
> +This script also depends on Kconfiglib.  Please install it in advance.
>  """
>
>  import errno
>  import fnmatch
>  import glob
> +import multiprocessing
>  import optparse
>  import os
> -import re
> -import shutil
>  import subprocess
>  import sys
>  import tempfile
>  import time
>
> -BOARD_FILE = 'boards.cfg'
> -CONFIG_DIR = 'configs'
> -REFORMAT_CMD = [os.path.join('tools', 'reformat.py'),
> -                '-i', '-d', '-', '-s', '8']
> -SHOW_GNU_MAKE = 'scripts/show-gnu-make'
> -SLEEP_TIME=0.003
> +KCONFIGLIB_NOT_FOUND = """Kconfiglib not found.
> +
> +*** How to install Kconfiglib ***
> + $ git clone git://github.com/ulfalizer/Kconfiglib.git
> + $ cd Kconfiglib
> + # python setup.py install
> +
> +Or more simply, you can copy the file to tools directory like this:
> + $ cd tools
> + $ wget https://raw.githubusercontent.com/ulfalizer/Kconfiglib/master/\
> +kconfiglib.py"""
>
> +try:
> +    import kconfiglib
> +except:
> +    sys.exit(KCONFIGLIB_NOT_FOUND)
> +
> +### constant variables ###
> +OUTPUT_FILE = 'boards.cfg'
> +CONFIG_DIR = 'configs'
> +SLEEP_TIME = 0.03
>  COMMENT_BLOCK = '''#
>  # List of boards
>  #   Automatically generated by %s: don't edit
> @@ -43,35 +57,14 @@ COMMENT_BLOCK = '''#
>  ''' % __file__
>
>  ### helper functions ###
> -def get_terminal_columns():
> -    """Get the width of the terminal.
> -
> -    Returns:
> -      The width of the terminal, or zero if the stdout is not
> -      associated with tty.
> -    """
> -    try:
> -        return shutil.get_terminal_size().columns # Python 3.3~
> -    except AttributeError:
> -        import fcntl
> -        import termios
> -        import struct
> -        arg = struct.pack('hhhh', 0, 0, 0, 0)
> -        try:
> -            ret = fcntl.ioctl(sys.stdout.fileno(), termios.TIOCGWINSZ, arg)
> -        except IOError as exception:
> -            # If 'Inappropriate ioctl for device' error occurs,
> -            # stdout is probably redirected. Return 0.
> -            return 0
> -        return struct.unpack('hhhh', ret)[1]
> -
> -def get_devnull():
> -    """Get the file object of '/dev/null' device."""
> +def try_remove(f):
> +    """Remove a file ignoring 'No such file or directory' error."""
>      try:
> -        devnull = subprocess.DEVNULL # py3k
> -    except AttributeError:
> -        devnull = open(os.devnull, 'wb')
> -    return devnull
> +        os.remove(f)
> +    except OSError as exception:
> +        # Ignore 'No such file or directory' error
> +        if exception.errno != errno.ENOENT:
> +            raise
>
>  def check_top_directory():
>      """Exit if we are not at the top of source directory."""
> @@ -79,23 +72,15 @@ def check_top_directory():
>          if not os.path.exists(f):
>              sys.exit('Please run at the top of source directory.')
>
> -def get_make_cmd():
> -    """Get the command name of GNU Make."""
> -    process = subprocess.Popen([SHOW_GNU_MAKE], stdout=subprocess.PIPE)
> -    ret = process.communicate()
> -    if process.returncode:
> -        sys.exit('GNU Make not found')
> -    return ret[0].rstrip()
> -
> -def output_is_new():
> -    """Check if the boards.cfg file is up to date.
> +def output_is_new(output):
> +    """Check if the output file is up to date.
>
>      Returns:
> -      True if the boards.cfg file exists and is newer than any of
> +      True if the given output file exists and is newer than any of
>        *_defconfig, MAINTAINERS and Kconfig*.  False otherwise.
>      """
>      try:
> -        ctime = os.path.getctime(BOARD_FILE)
> +        ctime = os.path.getctime(output)
>      except OSError as exception:
>          if exception.errno == errno.ENOENT:
>              # return False on 'No such file or directory' error
> @@ -121,9 +106,9 @@ def output_is_new():
>              if ctime < os.path.getctime(filepath):
>                  return False
>
> -    # Detect a board that has been removed since the current boards.cfg
> +    # Detect a board that has been removed since the current board database
>      # was generated
> -    with open(BOARD_FILE) as f:
> +    with open(output) as f:
>          for line in f:
>              if line[0] == '#' or line == '\n':
>                  continue
> @@ -134,6 +119,166 @@ def output_is_new():
>      return True
>
>  ### classes ###
> +class KconfigScanner:
> +
> +    """Kconfig scanner."""
> +
> +    ### constant variable only used in this class ###
> +    _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' }

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'
> +    }

> +
> +    def __init__(self):
> +        """Scan all the Kconfig files and create a Config object."""
> +        # Define environment variables referenced from Kconfig
> +        os.environ['srctree'] = os.getcwd()
> +        os.environ['UBOOTVERSION'] = 'dummy'
> +        os.environ['KCONFIG_OBJDIR'] = ''
> +        self._conf = kconfiglib.Config()
> +
> +    def __del__(self):
> +        """Delete the temporary file before exit.
> +
> +        The scan() method of this class creates a temporay file and deletes
> +        it on success.  If scan() method throws an exception on the way,
> +        the temporary file might be left over.  In that case, it should be
> +        deleted in this destructor.
> +        """
> +        if hasattr(self, '_tmpfile') and self._tmpfile:
> +            try_remove(self._tmpfile)
> +
> +    def scan(self, defconfig):
> +        """Load a defconfig file and return a dictionary containing
> +        board parameters.

Nice to keep the title on one line, e.g.:

"""Load a defconfig file to obtain board parameters

> +
> +        Arguments:
> +          defconfig: path to the defconfig file to be processed
> +
> +        Returns:
> +          A dictionary of board parameters.  It has a form of:
> +          { 'arch': <arch_name>, 'cpu': <cpu_name>, 'soc': <soc_name>,
> +            'vendor': <vendor_name>, 'board': <board_name>,
> +            'target': <target_name>, 'config': <config_header_name>,
> +            'options': <extra_options> }
> +        """
> +        # strip special prefixes and save it in a temporary file
> +        fd, self._tmpfile = tempfile.mkstemp()
> +        with os.fdopen(fd, 'w') as f:
> +            for line in open(defconfig):
> +                colon = line.find(':CONFIG_')
> +                if colon == -1:
> +                    f.write(line)
> +                else:
> +                    f.write(line[colon + 1:])
> +
> +        self._conf.load_config(self._tmpfile)
> +
> +        try_remove(self._tmpfile)
> +        self._tmpfile = None
> +
> +        params = {}
> +
> +        # Get the value of CONFIG_SYS_ARCH, CONFIG_SYS_CPU, ... etc.
> +        # Set '-' if the value is empty.
> +        for key, symbol in self._SYMBOL_TABLE.items():
> +            value = self._conf.get_symbol(symbol).get_value()
> +            if value:
> +                params[key] = value
> +            else:
> +                params[key] = '-'
> +
> +        defconfig = os.path.basename(defconfig)
> +        params['target'], match, rear = defconfig.partition('_defconfig')
> +        assert match and not rear, '%s : invalid defconfig' % defconfig
> +
> +        # fix-up for aarch64
> +        if params['arch'] == 'arm' and params['cpu'] == 'armv8':
> +            params['arch'] = 'aarch64'
> +
> +        # fix-up options field. It should have the form:
> +        # <config name>[:comma separated config options]
> +        if params['options'] != '-':
> +            params['options'] = params['config'] + ':' + \
> +                                params['options'].replace(r'\"', '"')
> +        elif params['config'] != params['target']:
> +            params['options'] = params['config']
> +
> +        return params
> +
> +def scan_defconfigs_for_multiprocess(queue, defconfigs):
> +    """Scan given defconfig files and write the board parameters
> +    into the queue.

How about:

"""Scan a defconfig file and queue its board parameters

> +
> +    This function is intended to be passed to multiprocessing.Process()
> +    constructor.
> +
> +    Arguments:
> +      queue: An instance of multiprocessing.Queue().
> +             The resulting board parameters are written into it.
> +      defconfigs: A sequence of defconfig files to be scanned.
> +    """
> +    kconf_scanner = KconfigScanner()
> +    for defconfig in defconfigs:
> +        queue.put(kconf_scanner.scan(defconfig))
> +
> +def read_queues(queues, params_list):
> +    """Read the queues and append the data to the paramers list"""
> +    for q in queues:
> +        while not q.empty():
> +            params_list.append(q.get())
> +
> +def scan_defconfigs(jobs=1):
> +    """Scan all the defconfig files and collect board parameters
> +    for all of them.

This is shorter:

"""Collect board parameters for all defconfig files

> +
> +    This function invokes multiple processes for faster processing.
> +
> +    Arguments:
> +      jobs: The number of jobs to run simultaneously
> +    """
> +    all_defconfigs = []
> +    for (dirpath, dirnames, filenames) in os.walk(CONFIG_DIR):
> +        for filename in fnmatch.filter(filenames, '*_defconfig'):
> +            if fnmatch.fnmatch(filename, '.*'):
> +                continue
> +            all_defconfigs.append(os.path.join(dirpath, filename))
> +
> +    total = len(all_defconfigs)

Would total_boards be a better name?

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

/ jobs
(add space)

> +        q = multiprocessing.Queue(maxsize=-1)
> +        p = multiprocessing.Process(target=scan_defconfigs_for_multiprocess,
> +                                    args=(q, defconfigs))
> +        p.start()
> +        processes.append(p)
> +        queues.append(q)
> +
> +    # The resulting data should be accumulated to this list
> +    params_list = []
> +
> +    # Data in the queues should be retrieved preriodically.
> +    # Otherwise, the queues would become full and subprocesses would get stuck.
> +    while any([p.is_alive() for p in processes]):
> +        read_queues(queues, params_list)
> +        # sleep for a while until the queues are filled
> +        time.sleep(SLEEP_TIME)
> +
> +    # Joining subprocesses just in case
> +    # (All subprocesses should already have been finished)
> +    for p in processes:
> +        p.join()
> +
> +    # retrieve leftover data
> +    read_queues(queues, params_list)
> +
> +    return params_list
> +
>  class MaintainersDatabase:
>
>      """The database of board status and maintainers."""
> @@ -146,7 +291,8 @@ class MaintainersDatabase:
>          """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.

>          """
>          if not target in self.database:
>              print >> sys.stderr, "WARNING: no status info for '%s'" % target
> @@ -165,8 +311,9 @@ class MaintainersDatabase:
>      def get_maintainers(self, target):
>          """Return the maintainers of the given board.
>
> -        If the board has two or more maintainers, they are separated
> -        with colons.
> +        Returns:
> +          Maintainers of the board.  If the board has two or more maintainers,
> +          they are separated with colons.
>          """
>          if not target in self.database:
>              print >> sys.stderr, "WARNING: no maintainers for '%s'" % target
> @@ -177,8 +324,8 @@ class MaintainersDatabase:
>      def parse_file(self, file):
>          """Parse the given MAINTAINERS file.
>
> -        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/

>          Arguments:
>            file: MAINTAINERS file to be parsed
> @@ -210,414 +357,103 @@ class MaintainersDatabase:
>              for target in targets:
>                  self.database[target] = (status, maintainers)
>
> -class DotConfigParser:
> -
> -    """A parser of .config file.
> +def insert_maintainers_info(params_list):
> +    """Add Status and Maintainers information to the board parameters list.
>
> -    Each line of the output should have the form of:
> -    Status, Arch, CPU, SoC, Vendor, Board, Target, Options, Maintainers
> -    Most of them are extracted from .config file.
> -    MAINTAINERS files are also consulted for Status and Maintainers fields.
> +    Arguments:
> +      params_list: A list of the board parameters
>      """
> +    database = MaintainersDatabase()
> +    for (dirpath, dirnames, filenames) in os.walk('.'):
> +        if 'MAINTAINERS' in filenames:
> +            database.parse_file(os.path.join(dirpath, 'MAINTAINERS'))
>
> -    re_arch = re.compile(r'CONFIG_SYS_ARCH="(.*)"')
> -    re_cpu = re.compile(r'CONFIG_SYS_CPU="(.*)"')
> -    re_soc = re.compile(r'CONFIG_SYS_SOC="(.*)"')
> -    re_vendor = re.compile(r'CONFIG_SYS_VENDOR="(.*)"')
> -    re_board = re.compile(r'CONFIG_SYS_BOARD="(.*)"')
> -    re_config = re.compile(r'CONFIG_SYS_CONFIG_NAME="(.*)"')
> -    re_options = re.compile(r'CONFIG_SYS_EXTRA_OPTIONS="(.*)"')
> -    re_list = (('arch', re_arch), ('cpu', re_cpu), ('soc', re_soc),
> -               ('vendor', re_vendor), ('board', re_board),
> -               ('config', re_config), ('options', re_options))
> -    must_fields = ('arch', 'config')
> -
> -    def __init__(self, build_dir, output, maintainers_database):
> -        """Create a new .config perser.
> -
> -        Arguments:
> -          build_dir: Build directory where .config is located
> -          output: File object which the result is written to
> -          maintainers_database: An instance of class MaintainersDatabase
> -        """
> -        self.dotconfig = os.path.join(build_dir, '.config')
> -        self.output = output
> -        self.database = maintainers_database
> +    for i, params in enumerate(params_list):
> +        target = params['target']
> +        params['status'] = database.get_status(target)
> +        params['maintainers'] = database.get_maintainers(target)
> +        params_list[i] = params
>
> -    def parse(self, defconfig):
> -        """Parse .config file and output one-line database for the given board.
> +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.

>
> -        Arguments:
> -          defconfig: Board (defconfig) name
> -        """
> -        fields = {}
> -        for line in open(self.dotconfig):
> -            if not line.startswith('CONFIG_SYS_'):
> -                continue
> -            for (key, pattern) in self.re_list:
> -                m = pattern.match(line)
> -                if m and m.group(1):
> -                    fields[key] = m.group(1)
> -                    break
> -
> -        # sanity check of '.config' file
> -        for field in self.must_fields:
> -            if not field in fields:
> -                print >> sys.stderr, (
> -                    "WARNING: '%s' is not defined in '%s'. Skip." %
> -                    (field, defconfig))
> -                return
> -
> -        # fix-up for aarch64
> -        if fields['arch'] == 'arm' and 'cpu' in fields:
> -            if fields['cpu'] == 'armv8':
> -                fields['arch'] = 'aarch64'
> -
> -        target, match, rear = defconfig.partition('_defconfig')
> -        assert match and not rear, \
> -                                '%s : invalid defconfig file name' % defconfig
> -
> -        fields['status'] = self.database.get_status(target)
> -        fields['maintainers'] = self.database.get_maintainers(target)
> -
> -        if 'options' in fields:
> -            options = fields['config'] + ':' + \
> -                      fields['options'].replace(r'\"', '"')
> -        elif fields['config'] != target:
> -            options = fields['config']
> -        else:
> -            options = '-'
> -
> -        self.output.write((' '.join(['%s'] * 9) + '\n')  %
> -                          (fields['status'],
> -                           fields['arch'],
> -                           fields.get('cpu', '-'),
> -                           fields.get('soc', '-'),
> -                           fields.get('vendor', '-'),
> -                           fields.get('board', '-'),
> -                           target,
> -                           options,
> -                           fields['maintainers']))
> -
> -class Slot:
> -
> -    """A slot to store a subprocess.
> -
> -    Each instance of this class handles one subprocess.
> -    This class is useful to control multiple processes
> -    for faster processing.
> +    Arguments:
> +      params_list: The list of board parameters
> +      output: The path to the output file
>      """
> +    FIELDS = ('status', 'arch', 'cpu', 'soc', 'vendor', 'board', 'target',
> +              'options', 'maintainers')
>
> -    def __init__(self, output, maintainers_database, devnull, make_cmd):
> -        """Create a new slot.
> -
> -        Arguments:
> -          output: File object which the result is written to
> -          maintainers_database: An instance of class MaintainersDatabase
> -          devnull: file object of 'dev/null'
> -          make_cmd: the command name of Make
> -        """
> -        self.build_dir = tempfile.mkdtemp()
> -        self.devnull = devnull
> -        self.ps = subprocess.Popen([make_cmd, 'O=' + self.build_dir,
> -                                    'allnoconfig'], stdout=devnull)
> -        self.occupied = True
> -        self.parser = DotConfigParser(self.build_dir, output,
> -                                      maintainers_database)
> -        self.env = os.environ.copy()
> -        self.env['srctree'] = os.getcwd()
> -        self.env['UBOOTVERSION'] = 'dummy'
> -        self.env['KCONFIG_OBJDIR'] = ''
> -
> -    def __del__(self):
> -        """Delete the working directory"""
> -        if not self.occupied:
> -            while self.ps.poll() == None:
> -                pass
> -        shutil.rmtree(self.build_dir)
> -
> -    def add(self, defconfig):
> -        """Add a new subprocess to the slot.
> -
> -        Fails if the slot is occupied, that is, the current subprocess
> -        is still running.
> -
> -        Arguments:
> -          defconfig: Board (defconfig) name
> -
> -        Returns:
> -          Return True on success or False on fail
> -        """
> -        if self.occupied:
> -            return False
> -
> -        with open(os.path.join(self.build_dir, '.tmp_defconfig'), 'w') as f:
> -            for line in open(os.path.join(CONFIG_DIR, defconfig)):
> -                colon = line.find(':CONFIG_')
> -                if colon == -1:
> -                    f.write(line)
> -                else:
> -                    f.write(line[colon + 1:])
> -
> -        self.ps = subprocess.Popen([os.path.join('scripts', 'kconfig', 'conf'),
> -                                    '--defconfig=.tmp_defconfig', 'Kconfig'],
> -                                   stdout=self.devnull,
> -                                   cwd=self.build_dir,
> -                                   env=self.env)
> +    # First, decide the width of each column
> +    max_length = dict([ (f, 0) for f in FIELDS])
> +    for params in params_list:
> +        for f in FIELDS:
> +            max_length[f] = max(max_length[f], len(params[f]))
>
> -        self.defconfig = defconfig
> -        self.occupied = True
> -        return True
> +    output_lines = []
> +    for params in params_list:
> +        line = ''
> +        for f in FIELDS:
> +            # insert two spaces between fields like column -t would
> +            line += '  ' + params[f].ljust(max_length[f])
> +        output_lines.append(line.strip())
>
> -    def wait(self):
> -        """Wait until the current subprocess finishes."""
> -        while self.occupied and self.ps.poll() == None:
> -            time.sleep(SLEEP_TIME)
> -        self.occupied = False
> +    # ignore case when sorting
> +    output_lines.sort(key=str.lower)
>
> -    def poll(self):
> -        """Check if the subprocess is running and invoke the .config
> -        parser if the subprocess is terminated.
> -
> -        Returns:
> -          Return True if the subprocess is terminated, False otherwise
> -        """
> -        if not self.occupied:
> -            return True
> -        if self.ps.poll() == None:
> -            return False
> -        if self.ps.poll() == 0:
> -            self.parser.parse(self.defconfig)
> -        else:
> -            print >> sys.stderr, ("WARNING: failed to process '%s'. skip." %
> -                                  self.defconfig)
> -        self.occupied = False
> -        return True
> -
> -class Slots:
> -
> -    """Controller of the array of subprocess slots."""
> -
> -    def __init__(self, jobs, output, maintainers_database):
> -        """Create a new slots controller.
> -
> -        Arguments:
> -          jobs: A number of slots to instantiate
> -          output: File object which the result is written to
> -          maintainers_database: An instance of class MaintainersDatabase
> -        """
> -        self.slots = []
> -        devnull = get_devnull()
> -        make_cmd = get_make_cmd()
> -        for i in range(jobs):
> -            self.slots.append(Slot(output, maintainers_database,
> -                                   devnull, make_cmd))
> -        for slot in self.slots:
> -            slot.wait()
> -
> -    def add(self, defconfig):
> -        """Add a new subprocess if a vacant slot is available.
> -
> -        Arguments:
> -          defconfig: Board (defconfig) name
> -
> -        Returns:
> -          Return True on success or False on fail
> -        """
> -        for slot in self.slots:
> -            if slot.add(defconfig):
> -                return True
> -        return False
> -
> -    def available(self):
> -        """Check if there is a vacant slot.
> -
> -        Returns:
> -          Return True if a vacant slot is found, False if all slots are full
> -        """
> -        for slot in self.slots:
> -            if slot.poll():
> -                return True
> -        return False
> -
> -    def empty(self):
> -        """Check if all slots are vacant.
> -
> -        Returns:
> -          Return True if all slots are vacant, False if at least one slot
> -          is running
> -        """
> -        ret = True
> -        for slot in self.slots:
> -            if not slot.poll():
> -                ret = False
> -        return ret
> +    with open(output, 'w') as f:
> +        f.write(COMMENT_BLOCK + '\n'.join(output_lines) + '\n')
>
> -class Indicator:
> -
> -    """A class to control the progress indicator."""
> -
> -    MIN_WIDTH = 15
> -    MAX_WIDTH = 70
> -
> -    def __init__(self, total):
> -        """Create an instance.
> -
> -        Arguments:
> -          total: A number of boards
> -        """
> -        self.total = total
> -        self.cur = 0
> -        width = get_terminal_columns()
> -        width = min(width, self.MAX_WIDTH)
> -        width -= self.MIN_WIDTH
> -        if width > 0:
> -            self.enabled = True
> -        else:
> -            self.enabled = False
> -        self.width = width
> -
> -    def inc(self):
> -        """Increment the counter and show the progress bar."""
> -        if not self.enabled:
> -            return
> -        self.cur += 1
> -        arrow_len = self.width * self.cur // self.total
> -        msg = '%4d/%d [' % (self.cur, self.total)
> -        msg += '=' * arrow_len + '>' + ' ' * (self.width - arrow_len) + ']'
> -        sys.stdout.write('\r' + msg)
> -        sys.stdout.flush()
> -
> -class BoardsFileGenerator:
> -
> -    """Generator of boards.cfg."""
> -
> -    def __init__(self):
> -        """Prepare basic things for generating boards.cfg."""
> -        # All the defconfig files to be processed
> -        defconfigs = []
> -        for (dirpath, dirnames, filenames) in os.walk(CONFIG_DIR):
> -            dirpath = dirpath[len(CONFIG_DIR) + 1:]
> -            for filename in fnmatch.filter(filenames, '*_defconfig'):
> -                if fnmatch.fnmatch(filename, '.*'):
> -                    continue
> -                defconfigs.append(os.path.join(dirpath, filename))
> -        self.defconfigs = defconfigs
> -        self.indicator = Indicator(len(defconfigs))
> -
> -        # Parse all the MAINTAINERS files
> -        maintainers_database = MaintainersDatabase()
> -        for (dirpath, dirnames, filenames) in os.walk('.'):
> -            if 'MAINTAINERS' in filenames:
> -                maintainers_database.parse_file(os.path.join(dirpath,
> -                                                             'MAINTAINERS'))
> -        self.maintainers_database = maintainers_database
> -
> -    def __del__(self):
> -        """Delete the incomplete boards.cfg
> -
> -        This destructor deletes boards.cfg if the private member 'in_progress'
> -        is defined as True.  The 'in_progress' member is set to True at the
> -        beginning of the generate() method and set to False at its end.
> -        So, in_progress==True means generating boards.cfg was terminated
> -        on the way.
> -        """
> -
> -        if hasattr(self, 'in_progress') and self.in_progress:
> -            try:
> -                os.remove(BOARD_FILE)
> -            except OSError as exception:
> -                # Ignore 'No such file or directory' error
> -                if exception.errno != errno.ENOENT:
> -                    raise
> -            print 'Removed incomplete %s' % BOARD_FILE
> -
> -    def generate(self, jobs):
> -        """Generate boards.cfg
> -
> -        This method sets the 'in_progress' member to True at the beginning
> -        and sets it to False on success.  The boards.cfg should not be
> -        touched before/after this method because 'in_progress' is used
> -        to detect the incomplete boards.cfg.
> -
> -        Arguments:
> -          jobs: The number of jobs to run simultaneously
> -        """
> -
> -        self.in_progress = True
> -        print 'Generating %s ...  (jobs: %d)' % (BOARD_FILE, jobs)
> -
> -        # Output lines should be piped into the reformat tool
> -        reformat_process = subprocess.Popen(REFORMAT_CMD,
> -                                            stdin=subprocess.PIPE,
> -                                            stdout=open(BOARD_FILE, 'w'))
> -        pipe = reformat_process.stdin
> -        pipe.write(COMMENT_BLOCK)
> -
> -        slots = Slots(jobs, pipe, self.maintainers_database)
> -
> -        # Main loop to process defconfig files:
> -        #  Add a new subprocess into a vacant slot.
> -        #  Sleep if there is no available slot.
> -        for defconfig in self.defconfigs:
> -            while not slots.add(defconfig):
> -                while not slots.available():
> -                    # No available slot: sleep for a while
> -                    time.sleep(SLEEP_TIME)
> -            self.indicator.inc()
> -
> -        # wait until all the subprocesses finish
> -        while not slots.empty():
> -            time.sleep(SLEEP_TIME)
> -        print ''
> -
> -        # wait until the reformat tool finishes
> -        reformat_process.communicate()
> -        if reformat_process.returncode != 0:
> -            sys.exit('"%s" failed' % REFORMAT_CMD[0])
> -
> -        self.in_progress = False
> -
> -def gen_boards_cfg(jobs=1, force=False):
> -    """Generate boards.cfg file.
> -
> -    The incomplete boards.cfg is deleted if an error (including
> -    the termination by the keyboard interrupt) occurs on the halfway.
> +def gen_boards_cfg(output, jobs=1, force=False):
> +    """Generate a board database file.
>
>      Arguments:
> +      output: The name of the output file
>        jobs: The number of jobs to run simultaneously
> +      force: Force to generate the output even if it is new
>      """
>      check_top_directory()
> -    if not force and output_is_new():
> -        print "%s is up to date. Nothing to do." % BOARD_FILE
> +
> +    if not force and output_is_new(output):
> +        print "%s is up to date. Nothing to do." % output
>          sys.exit(0)
>
> -    generator = BoardsFileGenerator()
> -    generator.generate(jobs)
> +    params_list = scan_defconfigs(jobs)
> +    insert_maintainers_info(params_list)
> +    format_and_output(params_list, output)
>
>  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')

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.

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

>          except (OSError, ValueError):
> -            print 'info: failed to get the number of CPUs. Set jobs to 1'
>              jobs = 1
>
> -    gen_boards_cfg(jobs, force=options.force)
> +    if options.output:
> +        output = options.output
> +    else:
> +        output = OUTPUT_FILE
> +
> +    gen_boards_cfg(output, jobs=jobs, force=options.force)
>
>  if __name__ == '__main__':
>      main()
> --
> 1.9.1
>

Regards,
Simon

Dear Simon & Masahiro,

In message <CAPnjgZ3qPAPVYv68okF9Cgmn2a-wvJ1cEb56CkfFzYaHjSHzLQ@mail.gmail.com> you wrote:
> 
> The license looks fine to me - you can probably use a short form like this:
> 
> https://spdx.org/licenses/ISC

We have to add the license text to Licenses/ and add the respective
SPDX ID.

Best regards,

Wolfgang Denk

Hi Simon,



On Sun, 31 Aug 2014 21:52:41 -0700
Simon Glass <sjg@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