[U-Boot] [PATCH v2 2/2] tools: moveconfig: Add a new --git-ref option

[Masahiro Yamada <yamada.masahiro@socionext.com>]

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,

-- 
Best Regards
Masahiro Yamada