HI Masahiro,
On 20 August 2014 05:47, Masahiro Yamada <yamad...@jp.panasonic.com> wrote:
> This tool deletes the incomplete boards.cfg
> if it encounters an error or is is terminated by the user.
>
> I notice some problems even though they rarely happen.
>
> [1] The boards.cfg is removed if the program is terminated
> during __gen_boards_cfg() function but before boards.cfg
> is actually touched. In this case, the previous boards.cfg
> should be kept as it is.
>
> [2] If an error occurs while deleting the incomplete boards.cfg,
> the program throws another exception. This hides the privious
> exception and we will not be able to know the real cause.
>
> Signed-off-by: Masahiro Yamada <yamad...@jp.panasonic.com>
Acked-by: Simon Glass <s...@chromium.org>
A few suggestions below (please ignore as you wish, they are not important)
> ---
>
> tools/genboardscfg.py | 156
> +++++++++++++++++++++++++++++---------------------
> 1 file changed, 90 insertions(+), 66 deletions(-)
>
> diff --git a/tools/genboardscfg.py b/tools/genboardscfg.py
> index 9b3a9bf..13bb424 100755
> --- a/tools/genboardscfg.py
> +++ b/tools/genboardscfg.py
> @@ -417,63 +417,95 @@ class Indicator:
> sys.stdout.write('\r' + msg)
> sys.stdout.flush()
>
> -def __gen_boards_cfg(jobs):
> - """Generate boards.cfg file.
> +class BoardsFileGenerator:
>
> - Arguments:
> - jobs: The number of jobs to run simultaneously
> + """Generator of boards.cfg."""
>
> - Note:
> - The incomplete boards.cfg is left over when an error (including
> - the termination by the keyboard interrupt) occurs on the halfway.
> - """
> - check_top_directory()
> - print 'Generating %s ... (jobs: %d)' % (BOARD_FILE, jobs)
> -
> - # 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))
> -
> - # 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'))
> -
> - # 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)
> -
> - indicator = Indicator(len(defconfigs))
> - slots = Slots(jobs, pipe, 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 defconfigs:
> - while not slots.add(defconfig):
> - while not slots.available():
> - # No available slot: sleep for a while
> - time.sleep(SLEEP_TIME)
> - 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])
> + 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))
I try to put an _ before private members to indicate that they should
not be used outside the class. But It is not particularly important -
just thought I'd mention it.
> +
> + # 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:
Would it be better to initialise in_progress to None in the constructor?
> + try:
> + os.remove(BOARD_FILE)
> + except OSError, 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):
> """Generate boards.cfg file.
> @@ -484,17 +516,9 @@ def gen_boards_cfg(jobs):
> Arguments:
> jobs: The number of jobs to run simultaneously
> """
> - try:
> - __gen_boards_cfg(jobs)
> - except:
> - # We should remove incomplete boards.cfg
> - try:
> - os.remove(BOARD_FILE)
> - except OSError, exception:
> - # Ignore 'No such file or directory' error
> - if exception.errno != errno.ENOENT:
> - raise
> - raise
> + check_top_directory()
> + generator = BoardsFileGenerator()
> + generator.generate(jobs)
>
> def main():
> parser = optparse.OptionParser()
> --
> 1.9.1
>
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
