Hi Joe,
2016-06-02 12:30 GMT+09:00 Joe Hershberger <joe.hershber...@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.hershber...@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, -- Best Regards Masahiro Yamada _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot