| Message ID | 1464838207-10020-2-git-send-email-joe.hershberger@ni.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Masahiro Yamada |
| Headers | show |
Hi Joe, 2016-06-02 12:30 GMT+09:00 Joe Hershberger <joe.hershberger@ni.com>: > This option allows the 'make *_defconfig' step to run against a former > repo state, while the savedefconfig step runs against the current repo > state. This is convenient for the case where something in the Kconfig > has changed such that the defconfig is no longer complete with the new > Kconfigs. This feature allows the .config to be built assuming those old > Kconfigs, but then savedefconfig based on the new state of the Kconfigs. > > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> Basically, your idea seems cool, but please improve the implementation. It looks like your patch includes various noises (more changes than needed), and things are getting complex. > @@ -412,6 +416,9 @@ class KconfigParser: > self.options = options > self.dotconfig = os.path.join(build_dir, '.config') > self.autoconf = os.path.join(build_dir, 'include', 'autoconf.mk') > + if options.git_ref: > + self.autoconf_orig = os.path.join(build_dir, 'include', > + 'autoconf.orig.mk') This change is not needed. (see below) > self.config_autoconf = os.path.join(build_dir, 'include', 'config', > 'auto.conf') > self.defconfig = os.path.join(build_dir, 'defconfig') > @@ -464,14 +471,6 @@ class KconfigParser: > """ > not_set = '# %s is not set' % config > > - for line in dotconfig_lines: > - line = line.rstrip() > - if line.startswith(config + '=') or line == not_set: > - old_val = line > - break > - else: > - return (ACTION_NO_ENTRY, config) > - > for line in autoconf_lines: > line = line.rstrip() > if line.startswith(config + '='): > @@ -480,6 +479,14 @@ class KconfigParser: > else: > new_val = not_set > > + for line in dotconfig_lines: > + line = line.rstrip() > + if line.startswith(config + '=') or line == not_set: > + old_val = line > + break > + else: > + return (ACTION_NO_ENTRY, config) > + Why did you move this hunk? > if old_val == new_val: > return (ACTION_NO_CHANGE, new_val) > > @@ -515,8 +522,14 @@ class KconfigParser: > with open(self.dotconfig) as f: > dotconfig_lines = f.readlines() > > - with open(self.autoconf) as f: > - autoconf_lines = f.readlines() > + > + if self.options.git_ref: > + # Pull the target value from the original autoconf.mk > + with open(self.autoconf_orig) as f: > + autoconf_lines = f.readlines() > + else: > + with open(self.autoconf) as f: > + autoconf_lines = f.readlines() Unneeded. (see below) > for config in self.configs: > result = self.parse_one_config(config, dotconfig_lines, > @@ -585,7 +598,7 @@ class Slot: > for faster processing. > """ > > - def __init__(self, configs, options, progress, devnull, make_cmd): > + def __init__(self, configs, options, progress, devnull, make_cmd, defconfig_src_dir): > """Create a new process slot. > > Arguments: In this v2 approach, defconfig occurs twice; first against the cwd tree, 2nd against the cloned-tree. So, "defconfig_src_dir" does not best-describe the behavior, I think. Could you rename "defconfig_src_dir" to something more suitable? Maybe, "ref_src_dir" or something? Also, when you add a new argument, please add a comment to "Arguments:" > @@ -600,8 +613,11 @@ class Slot: > self.build_dir = tempfile.mkdtemp() > self.devnull = devnull > self.make_cmd = (make_cmd, 'O=' + self.build_dir) > + self.defconfig_src_dir = defconfig_src_dir Please consider renaming it. > self.parser = KconfigParser(configs, options, self.build_dir) > self.state = STATE_IDLE > + if self.options.git_ref: > + self.use_git_ref = True This is not needed because it is always initialized in the add() method. > self.failed_boards = [] > > def __del__(self): > @@ -609,7 +625,7 @@ class Slot: > > This function makes sure the temporary directory is cleaned away > even if Python suddenly dies due to error. It should be done in here > - because it is guranteed the destructor is always invoked when the > + because it is guaranteed the destructor is always invoked when the > instance of the class gets unreferenced. > > If the subprocess is still running, wait until it finishes. > @@ -638,6 +654,7 @@ class Slot: > self.defconfig = defconfig > self.state = STATE_INIT > self.log = '' > + self.use_git_ref = True Setting always use_git_ref to True seems odd. Maybe, can you change it like this? self.use_git_ref = True if self.options.git_ref else False > return True > > def poll(self): > @@ -662,6 +679,9 @@ class Slot: > if self.state == STATE_INIT: > cmd = list(self.make_cmd) > cmd.append(self.defconfig) > + if self.options.git_ref and self.use_git_ref: With my suggestion above, checking self.use_git_ref should be enough. if self.use_git_ref: > + cmd.append('-C') > + cmd.append(self.defconfig_src_dir) > self.ps = subprocess.Popen(cmd, stdout=self.devnull, > stderr=subprocess.PIPE) > self.state = STATE_DEFCONFIG > @@ -692,12 +712,22 @@ class Slot: > cmd.append('CROSS_COMPILE=%s' % self.cross_compile) > cmd.append('KCONFIG_IGNORE_DUPLICATES=1') > cmd.append('include/config/auto.conf') > + if self.options.git_ref and self.use_git_ref: Ditto. if self.use_git_ref: > + cmd.append('-C') > + cmd.append(self.defconfig_src_dir) > self.ps = subprocess.Popen(cmd, stdout=self.devnull, > stderr=subprocess.PIPE) > self.state = STATE_AUTOCONF > return False > > if self.state == STATE_AUTOCONF: > + if self.options.git_ref and self.use_git_ref: > + shutil.move(os.path.join(self.build_dir, 'include/autoconf.mk'), > + os.path.join(self.build_dir, 'include/autoconf.orig.mk')) > + self.state = STATE_INIT > + self.use_git_ref = False > + return False > + > (updated, log) = self.parser.update_dotconfig() > self.log += log As far as I understood, if -r options is specified, the state moves like this. (1) STATE_IDLE (2) STATE_INIT (3) STATE_DEFCONFIG (4) STATE_AUTOCONF (5) STATE_INIT (2nd) (6) STATE_DEFCONFIG (2nd) (7) STATE_AUTOCONF (2nd) (8) STATE_SAVEDEFCONFIG But, is the 2nd autoconf necessary? We only want to use autoconf.mk from the first autoconf. The second one is just harmful; it overwrites autoconf.mk we want to parse. If the 2nd one is skipped, we do not need to copy it to include/autoconf.orig.mk I understand why you did so. The state transition is getting very complicated. After pondering on it for a while, I decided splitting code into helper methods might get the situation better. Please see my this follow-up patch. http://patchwork.ozlabs.org/patch/631921/ With the patch applied, the poll() method is like this, if self.ps.poll() != 0: self.handle_error() elif self.state == STATE_DEFCONFIG: self.do_autoconf() elif self.state == STATE_AUTOCONF: self.do_savedefconfig() elif self.state == STATE_SAVEDEFCONFIG: self.update_defconfig() else: sys.exit("Internal Error. This should not happen.") -r option might be implemented like this: if self.ps.poll() != 0: self.handle_error() elif self.state == STATE_DEFCONFIG: if self.options.git_ref and not self.use_git_ref: self.do_savedefconfig() else: self.do_autoconf() elif self.state == STATE_AUTOCONF: if self.use_git_ref: self.use_git_ref = False self.do_defconfig else: self.do_savedefconfig() elif self.state == STATE_SAVEDEFCONFIG: self.update_defconfig() else: sys.exit("Internal Error. This should not happen.") > @@ -724,7 +754,7 @@ class Slot: > updated = not filecmp.cmp(orig_defconfig, new_defconfig) > > if updated: > - self.log += color_text(self.options.color, COLOR_LIGHT_GREEN, > + self.log += color_text(self.options.color, COLOR_LIGHT_BLUE, > "defconfig was updated.\n") Unrelated change. You should send a separate patch if you want to change it. > @@ -853,11 +901,28 @@ def move_config(configs, options): > if options.force_sync: > print 'No CONFIG is specified. You are probably syncing defconfigs.', > else: > - print 'Neigher CONFIG nor --force-sync is specified. Nothing will happen.', > + print 'Neither CONFIG nor --force-sync is specified. Nothing will happen.', > else: > print 'Move ' + ', '.join(configs), > print '(jobs: %d)\n' % options.jobs I will fix this typo in my patch. Please drop this hunk when you send a new patch. > + defconfig_src_dir = '' > + > + if options.git_ref: > + work_dir = WorkDir() > + defconfig_src_dir = work_dir.get() > + cwd = os.getcwd() > + print "Cloning git repo for 'make *_defconfig' step..." > + subprocess.check_output(['git', 'clone', cwd, '.'], \ > + cwd=defconfig_src_dir) > + print "Checkout '%s' to find original configs." % \ > + subprocess.check_output(['git', 'rev-parse', '--short', \ > + options.git_ref]).strip() > + os.chdir(defconfig_src_dir) > + subprocess.check_output(['git', 'checkout', options.git_ref], > + stderr=subprocess.STDOUT) > + os.chdir(cwd) > + Please use cmd= option instead of os.chdir() subprocess.check_output(['git', 'checkout', options.git_ref], stderr=subprocess.STDOUT, cwd=defconfig_src_dir) To sum up, Apply my series without 11/21, then apply http://patchwork.ozlabs.org/patch/631921/, then could you consider rebasing your 2/2 on top of that? Thanks,
diff --git a/tools/moveconfig.py b/tools/moveconfig.py index 01350ce..7d9d31e 100755 --- a/tools/moveconfig.py +++ b/tools/moveconfig.py @@ -143,6 +143,10 @@ Available options Specify the number of threads to run simultaneously. If not specified, the number of threads is the same as the number of CPU cores. + -r, --git-ref + Specify the git ref to clone for the make *_defconfig step. If unspecified + use the CWD. + -v, --verbose Show any build errors as boards are built @@ -412,6 +416,9 @@ class KconfigParser: self.options = options self.dotconfig = os.path.join(build_dir, '.config') self.autoconf = os.path.join(build_dir, 'include', 'autoconf.mk') + if options.git_ref: + self.autoconf_orig = os.path.join(build_dir, 'include', + 'autoconf.orig.mk') self.config_autoconf = os.path.join(build_dir, 'include', 'config', 'auto.conf') self.defconfig = os.path.join(build_dir, 'defconfig') @@ -464,14 +471,6 @@ class KconfigParser: """ not_set = '# %s is not set' % config - for line in dotconfig_lines: - line = line.rstrip() - if line.startswith(config + '=') or line == not_set: - old_val = line - break - else: - return (ACTION_NO_ENTRY, config) - for line in autoconf_lines: line = line.rstrip() if line.startswith(config + '='): @@ -480,6 +479,14 @@ class KconfigParser: else: new_val = not_set + for line in dotconfig_lines: + line = line.rstrip() + if line.startswith(config + '=') or line == not_set: + old_val = line + break + else: + return (ACTION_NO_ENTRY, config) + if old_val == new_val: return (ACTION_NO_CHANGE, new_val) @@ -515,8 +522,14 @@ class KconfigParser: with open(self.dotconfig) as f: dotconfig_lines = f.readlines() - with open(self.autoconf) as f: - autoconf_lines = f.readlines() + + if self.options.git_ref: + # Pull the target value from the original autoconf.mk + with open(self.autoconf_orig) as f: + autoconf_lines = f.readlines() + else: + with open(self.autoconf) as f: + autoconf_lines = f.readlines() for config in self.configs: result = self.parse_one_config(config, dotconfig_lines, @@ -585,7 +598,7 @@ class Slot: for faster processing. """ - def __init__(self, configs, options, progress, devnull, make_cmd): + def __init__(self, configs, options, progress, devnull, make_cmd, defconfig_src_dir): """Create a new process slot. Arguments: @@ -600,8 +613,11 @@ class Slot: self.build_dir = tempfile.mkdtemp() self.devnull = devnull self.make_cmd = (make_cmd, 'O=' + self.build_dir) + self.defconfig_src_dir = defconfig_src_dir self.parser = KconfigParser(configs, options, self.build_dir) self.state = STATE_IDLE + if self.options.git_ref: + self.use_git_ref = True self.failed_boards = [] def __del__(self): @@ -609,7 +625,7 @@ class Slot: This function makes sure the temporary directory is cleaned away even if Python suddenly dies due to error. It should be done in here - because it is guranteed the destructor is always invoked when the + because it is guaranteed the destructor is always invoked when the instance of the class gets unreferenced. If the subprocess is still running, wait until it finishes. @@ -638,6 +654,7 @@ class Slot: self.defconfig = defconfig self.state = STATE_INIT self.log = '' + self.use_git_ref = True return True def poll(self): @@ -662,6 +679,9 @@ class Slot: if self.state == STATE_INIT: cmd = list(self.make_cmd) cmd.append(self.defconfig) + if self.options.git_ref and self.use_git_ref: + cmd.append('-C') + cmd.append(self.defconfig_src_dir) self.ps = subprocess.Popen(cmd, stdout=self.devnull, stderr=subprocess.PIPE) self.state = STATE_DEFCONFIG @@ -692,12 +712,22 @@ class Slot: cmd.append('CROSS_COMPILE=%s' % self.cross_compile) cmd.append('KCONFIG_IGNORE_DUPLICATES=1') cmd.append('include/config/auto.conf') + if self.options.git_ref and self.use_git_ref: + cmd.append('-C') + cmd.append(self.defconfig_src_dir) self.ps = subprocess.Popen(cmd, stdout=self.devnull, stderr=subprocess.PIPE) self.state = STATE_AUTOCONF return False if self.state == STATE_AUTOCONF: + if self.options.git_ref and self.use_git_ref: + shutil.move(os.path.join(self.build_dir, 'include/autoconf.mk'), + os.path.join(self.build_dir, 'include/autoconf.orig.mk')) + self.state = STATE_INIT + self.use_git_ref = False + return False + (updated, log) = self.parser.update_dotconfig() self.log += log @@ -724,7 +754,7 @@ class Slot: updated = not filecmp.cmp(orig_defconfig, new_defconfig) if updated: - self.log += color_text(self.options.color, COLOR_LIGHT_GREEN, + self.log += color_text(self.options.color, COLOR_LIGHT_BLUE, "defconfig was updated.\n") if not self.options.dry_run and updated: @@ -771,7 +801,7 @@ class Slots: """Controller of the array of subprocess slots.""" - def __init__(self, configs, options, progress): + def __init__(self, configs, options, progress, defconfig_src_dir): """Create a new slots controller. Arguments: @@ -785,7 +815,7 @@ class Slots: make_cmd = get_make_cmd() for i in range(options.jobs): self.slots.append(Slot(configs, options, progress, devnull, - make_cmd)) + make_cmd, defconfig_src_dir)) def add(self, defconfig): """Add a new subprocess if a vacant slot is found. @@ -842,6 +872,24 @@ class Slots: for board in failed_boards: f.write(board + '\n') +class WorkDir: + def __init__(self): + """Create a new working directory.""" + self.work_dir = tempfile.mkdtemp() + + def __del__(self): + """Delete the working directory + + This function makes sure the temporary directory is cleaned away + even if Python suddenly dies due to error. It should be done in here + because it is guaranteed the destructor is always invoked when the + instance of the class gets unreferenced. + """ + shutil.rmtree(self.work_dir) + + def get(self): + return self.work_dir + def move_config(configs, options): """Move config options to defconfig files. @@ -853,11 +901,28 @@ def move_config(configs, options): if options.force_sync: print 'No CONFIG is specified. You are probably syncing defconfigs.', else: - print 'Neigher CONFIG nor --force-sync is specified. Nothing will happen.', + print 'Neither CONFIG nor --force-sync is specified. Nothing will happen.', else: print 'Move ' + ', '.join(configs), print '(jobs: %d)\n' % options.jobs + defconfig_src_dir = '' + + if options.git_ref: + work_dir = WorkDir() + defconfig_src_dir = work_dir.get() + cwd = os.getcwd() + print "Cloning git repo for 'make *_defconfig' step..." + subprocess.check_output(['git', 'clone', cwd, '.'], \ + cwd=defconfig_src_dir) + print "Checkout '%s' to find original configs." % \ + subprocess.check_output(['git', 'rev-parse', '--short', \ + options.git_ref]).strip() + os.chdir(defconfig_src_dir) + subprocess.check_output(['git', 'checkout', options.git_ref], + stderr=subprocess.STDOUT) + os.chdir(cwd) + if options.defconfigs: defconfigs = [line.strip() for line in open(options.defconfigs)] for i, defconfig in enumerate(defconfigs): @@ -875,7 +940,7 @@ def move_config(configs, options): defconfigs.append(os.path.join(dirpath, filename)) progress = Progress(len(defconfigs)) - slots = Slots(configs, options, progress) + slots = Slots(configs, options, progress, defconfig_src_dir) # Main loop to process defconfig files: # Add a new subprocess into a vacant slot. @@ -917,6 +982,8 @@ def main(): help='only cleanup the headers') parser.add_option('-j', '--jobs', type='int', default=cpu_count, help='the number of jobs to run simultaneously') + parser.add_option('-r', '--git-ref', type='string', + help='the git ref to clone for the make *_defconfig step') parser.add_option('-v', '--verbose', action='store_true', default=False, help='show any build errors as boards are built') parser.usage += ' CONFIG ...'
This option allows the 'make *_defconfig' step to run against a former repo state, while the savedefconfig step runs against the current repo state. This is convenient for the case where something in the Kconfig has changed such that the defconfig is no longer complete with the new Kconfigs. This feature allows the .config to be built assuming those old Kconfigs, but then savedefconfig based on the new state of the Kconfigs. Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> --- Changes in v2: - Stop reusing functions from patman - Rebase on top of the latest moveconfig series tools/moveconfig.py | 101 +++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 84 insertions(+), 17 deletions(-)