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

Now the primary data for each board is in Kconfig, defconfig and
MAINTAINERS.

It is true boards.cfg is needed for MAKEALL and buildman and
might be useful to brouse boards in a single database.
But it would be painful to maintain the boards.cfg in sync.

So, this is the solution.
Add a tool to generate the equivalent boards.cfg file based on
the latest Kconfig, defconfig and MAINTAINERS.

We can keep all the functions of MAKEALL and buildman with it.

The best thing would be to change MAKEALL and buildman for not
depending on boards.cfg in future, but it would take some time.

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
---

Changes in v5:
  - Support wildcard pattern like 'F:  configs/foo_*_defconfig'
  - Rename genboardscfg to genboardscfg.py
  - Use assert statement for sanity check
  - Do not run if imported from another script
    if __name__ == '__main__':
         main()
  - Check if we are at the top of source directory

Changes in v4:
  - Newly added

Changes in v3: None
Changes in v2: None

 tools/genboardscfg.py | 449 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 449 insertions(+)
 create mode 100755 tools/genboardscfg.py

Hi Masahiro,

On 24 July 2014 05:00, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> Now the primary data for each board is in Kconfig, defconfig and
> MAINTAINERS.
>
> It is true boards.cfg is needed for MAKEALL and buildman and
> might be useful to brouse boards in a single database.
> But it would be painful to maintain the boards.cfg in sync.
>
> So, this is the solution.
> Add a tool to generate the equivalent boards.cfg file based on
> the latest Kconfig, defconfig and MAINTAINERS.
>
> We can keep all the functions of MAKEALL and buildman with it.
>
> The best thing would be to change MAKEALL and buildman for not
> depending on boards.cfg in future, but it would take some time.
>
> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>

Looks good, some nits beflow.

Acked-by: Simon Glass <sjg@chromium.org>

> ---
>
> Changes in v5:
>   - Support wildcard pattern like 'F:  configs/foo_*_defconfig'
>   - Rename genboardscfg to genboardscfg.py
>   - Use assert statement for sanity check
>   - Do not run if imported from another script
>     if __name__ == '__main__':
>          main()
>   - Check if we are at the top of source directory
>
> Changes in v4:
>   - Newly added
>
> Changes in v3: None
> Changes in v2: None
>
>  tools/genboardscfg.py | 449 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 449 insertions(+)
>  create mode 100755 tools/genboardscfg.py
>
> diff --git a/tools/genboardscfg.py b/tools/genboardscfg.py
> new file mode 100755
> index 0000000..3c68027
> --- /dev/null
> +++ b/tools/genboardscfg.py
> @@ -0,0 +1,449 @@
> +#!/usr/bin/env python
> +#
> +# Author: Masahiro Yamada <yamada.m@jp.panasonic.com>
> +#
> +# SPDX-License-Identifier:     GPL-2.0+
> +#
> +
> +'''
> +Converter from Kconfig and MAINTAINERS to boards.cfg
> +
> +Run 'tools/genboardscfg.py' to create boards.cfg file.
> +
> +Run 'tools/genboardscfg.py -h' for available options.
> +'''
> +
> +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.

> +
> +BUILD_DIR = '.' + os.path.basename(__file__)
> +BOARD_FILE = 'boards.cfg'
> +CONFIG_DIR = 'configs'
> +REFORMAT_CMD = [os.path.join('tools', 'reformat.py'),
> +                '-i', '-d', '-', '-s', '8']
> +SLEEP_TIME=0.03
> +
> +COMMENT_BLOCK = '''#
> +# List of boards
> +#   Automatically generated by %s: don't edit
> +#
> +# Status, Arch, CPU(:SPLCPU), SoC, Vendor, Board, Target, Options, Maintainers
> +
> +''' % __file__
> +
> +### 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?

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?

> +        except IOError as exception:
> +            if exception.errno != errno.ENOTTY:
> +                raise
> +            # 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'''
> +    try:
> +        DEVNULL = subprocess.DEVNULL # py3k
> +    except:
> +        DEVNULL = open(os.devnull, 'wb')
> +    return DEVNULL
> +
> +def check_top_directory():
> +    '''Exit if we are not at the top of source directory'''
> +    for f in ('README', 'Licenses'):
> +        if not os.path.exists(f):
> +            print >> sys.stderr, 'Please run at the top of source directory.'
> +            sys.exit(1)
> +
> +### 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.

> +    def get_status(self, target):
> +        '''
> +        Return the status of the given board

You can put the description on the same line immediately after the '''

> +
> +        The status is either 'Active' or 'Orphan'.
> +        '''
> +        tmp = self.database[target][0]
> +        if tmp.startswith('Maintained'):
> +            return 'Active'
> +        elif tmp.startswith('Orphan'):
> +            return 'Orphan'
> +        else:
> +            print >> sys.stderr, 'Error: %s: unknown status' % tmp
> +    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.
> +        '''
> +        return ':'.join(self.database[target][1])
> +    def parse_file(self, file):
> +        '''
> +        Parse the given MAINTAINERS file and add board status and
> +        maintainers information to the database
> +
> +        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'?

> +                maintainers.append(line[2:].strip())
> +            elif line[:2] == 'F:':
> +                # expand wildcard and filter by 'configs/*_defconfig'
> +                for f in glob.glob(line[2:].strip()):
> +                    front, match, rear = f.partition('configs/')
> +                    if not front and match:
> +                        front, match, rear = rear.rpartition('_defconfig')
> +                        if match and not rear:
> +                            targets.append(front)
> +            elif line[:2] == 'S:':
> +                status = line[2:].strip()
> +            elif line == '\n' and targets:
> +                for target in targets:
> +                    self.database[target] = (status, maintainers)
> +                targets = []
> +                maintainers = []
> +                status = '-'
> +        if targets:
> +            for target in targets:
> +                self.database[target] = (status, maintainers)
> +
> +class DotConfigParser:
> +    '''
> +    A parser of .config file
> +
> +    Each line of the boards.cfg has the form of:
> +    Status, Arch, CPU, SoC, Vendor, Board, Target, Options, Maintainers
> +    Most of the fields are extracted from .config file.
> +    MAINTAINERS files are also consulted for Status and Maintainers fields.
> +    '''
> +    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 - A instance of class MaintainersDatabase
> +        '''
> +        self.dotconfig = os.path.join(build_dir, '.config')
> +        self.output = output
> +        self.database = maintainers_database
> +    def parse(self, defconfig):
> +        '''
> +        Parse .config file and output one-line database for the given board
> +
> +        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
> +        for field in self.must_fields:
> +            # sanity check of '.config' file
> +            if not field in fields:
> +                print >> sys.stderr, 'Error: %s is not defined in %s' % \
> +                                                            (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?

> +            if 'soc' in fields and re.match('tegra[0-9]*$', fields['soc']):
> +                fields['cpu'] += ':arm720t'

This is unfortunate, but necessary I think.

> +        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:
> +    '''
> +    Subprocess slot

Slot?

> +
> +    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.

> +                 A new subprocess can be set if this flag is False.
> +      build_dir - Working directory of this slot
> +      parser - A instance of class DotConfigParser
> +    '''
> +    DEVNULL = get_devnull()
> +    def __init__(self, build_dir, output, maintainers_database):
> +        '''
> +        Create a new slot
> +
> +        Arguments:
> +          build_dir - Working directory of this slot
> +          output - File object which the result is written to
> +          maintainers_database - A instance of class MaintainersDatabase
> +        '''
> +        self.occupied = False
> +        self.build_dir = build_dir
> +        self.parser = DotConfigParser(build_dir, output, maintainers_database)
> +    def add(self, defconfig):
> +        '''
> +        Add a new subprocess to the slot
> +
> +        Fails if the slot is occupied (= current subprocess is still running)
> +
> +        Arguments:
> +          defconfig - Board (defconfig) name
> +
> +        Returns:
> +          Return True on success or False on fail
> +        '''
> +        if self.occupied:
> +            return False
> +        o = 'O=' + self.build_dir
> +        self.ps = subprocess.Popen(['make', o, defconfig], stdout=self.DEVNULL)
> +        self.defconfig = defconfig
> +        self.occupied = True
> +        return True
> +    def poll(self):
> +        '''
> +        Check if the subprocess is running or not 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
> +        self.parser.parse(self.defconfig)
> +        self.occupied = False
> +        return True
> +
> +class Slots:
> +    '''
> +    Controller of the array of subprocess slots
> +
> +    Private members:
> +      slots - A list of instances of class Slot
> +    '''
> +    def __init__(self, jobs, output, maintainers_database):
> +        '''
> +        Create a new slots controller
> +
> +        Arguments:
> +          jobs - A number of slots to instantiate
> +          output - Working directory. Each slot is allocated with
> +                   the working directory "output/<slot_number>".
> +          maintainers_database - A instance of class MaintainersDatabase
> +        '''
> +        self.slots = []
> +        for i in range(jobs):
> +            build_dir = os.path.join(BUILD_DIR, str(i))
> +            self.slots.append(Slot(build_dir, output, maintainers_database))
> +    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
> +
> +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?

Regards,
Simon

Hi Simon,



On Sat, 26 Jul 2014 01:17:02 +0100
Simon Glass <sjg@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

Hi Masahiro,

On 27 July 2014 10:05, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> Hi Simon,
>
>
>
> On Sat, 26 Jul 2014 01:17:02 +0100
> Simon Glass <sjg@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?

Not really. I have been following PEP8 (which I don't think mentions
this), but in general in U-Boot we try to sort include files.

>
>
>
>> > +
>> > +### 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 ?

Well your comment suggests that you do know what version supports this
feature so it could be changed to use 'if'. But it's not that
different - I think what you have is OK.

>
>
>> 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.)

OK, something to keep an eye on. I was more thinking of someone else
wanting access to the terminal code you have written here, from
another tool.

Regards,
Simon

Hi Simon,


On Mon, 28 Jul 2014 04:14:17 +0100
Simon Glass <sjg@chromium.org> wrote:

> Hi Masahiro,
> 
> On 27 July 2014 10:05, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> > Hi Simon,
> >
> >
> >
> > On Sat, 26 Jul 2014 01:17:02 +0100
> > Simon Glass <sjg@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?
> 
> Not really. I have been following PEP8 (which I don't think mentions
> this), but in general in U-Boot we try to sort include files.


I checked PEP8.


PEP8 says as follow in terms of imports:
--------------->8------------------
Imports should be grouped in the following order:

 1. standard library imports
 2. related third party imports
 3. local application/library specific imports
---------------8<-------------------

But the order in each group is not mentioned.





Best Regards
Masahiro Yamada

[ yes, old thread, but might be worth to have in archives ]

On Mon, 2014-07-28 at 04:14 +0100, Simon Glass wrote:
> 
> Hi Masahiro,
> 
> On 27 July 2014 10:05, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> > Hi Simon,
> >
> > On Sat, 26 Jul 2014 01:17:02 +0100
> > Simon Glass <sjg@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?
> 
> Not really. I have been following PEP8 (which I don't think mentions
> this), but in general in U-Boot we try to sort include files.

There are good reasons.  You immediately will spot duplicates,
you can check for items very quickly and without doubt, and
sorted lists reduce merge conflicts, or dramatically ease their
resolution.  It helps authors, and reviewers, and maintainers all
alike.

Since headers, as well as imports, all should be self contained
and not depend on a specific order, there is no downside either
in the alphasort order.  You might even spot where previously
undetected dependencies are.  One more advantage. :)


virtually yours
Gerhard Sittig