| Message ID | 1408535269-24066-6-git-send-email-yamada.m@jp.panasonic.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
HI Masahiro, On 20 August 2014 05:47, Masahiro Yamada <yamada.m@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 <yamada.m@jp.panasonic.com> Acked-by: Simon Glass <sjg@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
Hi Simon, On Wed, 20 Aug 2014 13:10:52 -0600 Simon Glass <sjg@chromium.org> wrote: > 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. I will try my best to keep this in mind when I send the next version. (and when I write other Python scripts.) But I do not have an enough motivation to do so for now. > > + > > + # 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? > At first I thought of that. If the constructor fails before setting in_progress to None, the destructor is invoked with undefined in_progress. Of course, it rarely (never) happens. But I am trying to be as safe as possible. Best Regards Masahiro Yamada
Hi Masahiro, On 20 August 2014 22:00, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > Hi Simon, > > > > On Wed, 20 Aug 2014 13:10:52 -0600 > Simon Glass <sjg@chromium.org> wrote: > >> 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. > > > I will try my best to keep this in mind > when I send the next version. > (and when I write other Python scripts.) > > But I do not have an enough motivation to do so for now. > > > >> > + >> > + # 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? >> > > > At first I thought of that. > > > If the constructor fails before setting in_progress to None, > the destructor is invoked with undefined in_progress. > > Of course, it rarely (never) happens. > But I am trying to be as safe as possible. I think it's good practice to avoid doing any processing in a constructor that can fail. You can have a separate method for that, thus guaranteeing that the constructor finishes correctly. Anyway this is not an important point. Regards, Simon
Hi Simon, On Wed, 20 Aug 2014 13:10:52 -0600 Simon Glass <sjg@chromium.org> wrote: > 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. I tried this before posing v3. On the way, I stopped and found it getting unreadable because I had to put an _ to lots of variables. At least, in my script, all the methods are public and all the variables are private, so I did not do that after all. Best Regards Masahiro Yamada
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)) + + # 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, 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()
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 <yamada.m@jp.panasonic.com> --- tools/genboardscfg.py | 156 +++++++++++++++++++++++++++++--------------------- 1 file changed, 90 insertions(+), 66 deletions(-)