[U-Boot] [PATCH v2 2/2] tools: moveconfig: Add a new --git-ref option
Masahiro Yamada
yamada.masahiro at socionext.com
Wed Jun 8 04:56:58 CEST 2016
Hi Joe,
2016-06-02 12:30 GMT+09:00 Joe Hershberger <joe.hershberger at 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 at 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
More information about the U-Boot
mailing list