[U-Boot] moveconfig: Add a new --git-ref option

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>
---

 tools/moveconfig.py     | 60 +++++++++++++++++++++++++++++++++++++++++++++----
 tools/patman/gitutil.py | 15 +++++++++++++
 2 files changed, 71 insertions(+), 4 deletions(-)

Hey guys,

On Fri, May 29, 2015 at 4:23 PM, Joe Hershberger <joe.hershberger@ni.com> wrote:
> 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>
> ---

I forgot the Cc's on this...

>  tools/moveconfig.py     | 60 +++++++++++++++++++++++++++++++++++++++++++++----
>  tools/patman/gitutil.py | 15 +++++++++++++
>  2 files changed, 71 insertions(+), 4 deletions(-)
>
> diff --git a/tools/moveconfig.py b/tools/moveconfig.py
> index 496c90a..eeb9c0e 100755
> --- a/tools/moveconfig.py
> +++ b/tools/moveconfig.py
> @@ -153,6 +153,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
>
> @@ -173,6 +177,12 @@ import sys
>  import tempfile
>  import time
>
> +# Bring in the patman libraries
> +our_path = os.path.dirname(os.path.realpath(__file__))
> +sys.path.append(os.path.join(our_path, 'patman'))
> +
> +import gitutil
> +
>  SHOW_GNU_MAKE = 'scripts/show-gnu-make'
>  SLEEP_TIME=0.03
>
> @@ -526,7 +536,7 @@ class Slot:
>      for faster processing.
>      """
>
> -    def __init__(self, config_attrs, options, devnull, make_cmd):
> +    def __init__(self, config_attrs, options, devnull, make_cmd, defconfig_src_dir):
>          """Create a new process slot.
>
>          Arguments:
> @@ -540,6 +550,7 @@ 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(config_attrs, options, self.build_dir)
>          self.state = STATE_IDLE
>          self.failed_boards = []
> @@ -576,6 +587,9 @@ class Slot:
>              return False
>          cmd = list(self.make_cmd)
>          cmd.append(defconfig)
> +        if self.options.git_ref:
> +            cmd.append('-C')
> +            cmd.append(self.defconfig_src_dir)
>          self.ps = subprocess.Popen(cmd, stdout=self.devnull,
>                                     stderr=subprocess.PIPE)
>          self.defconfig = defconfig
> @@ -658,6 +672,9 @@ class Slot:
>          cmd.append('include/config/auto.conf')
>          """This will be screen-scraped, so be sure the expected text will be
>          returned consistently on every machine by setting LANG=C"""
> +        if self.options.git_ref:
> +            cmd.append('-C')
> +            cmd.append(self.defconfig_src_dir)
>          self.ps = subprocess.Popen(cmd, stdout=self.devnull,
>                                     env=dict(os.environ, LANG='C'),
>                                     stderr=subprocess.PIPE)
> @@ -673,7 +690,7 @@ class Slots:
>
>      """Controller of the array of subprocess slots."""
>
> -    def __init__(self, config_attrs, options):
> +    def __init__(self, config_attrs, options, defconfig_src_dir):
>          """Create a new slots controller.
>
>          Arguments:
> @@ -686,7 +703,8 @@ class Slots:
>          devnull = get_devnull()
>          make_cmd = get_make_cmd()
>          for i in range(options.jobs):
> -            self.slots.append(Slot(config_attrs, options, devnull, make_cmd))
> +            self.slots.append(Slot(config_attrs, options, devnull, make_cmd,
> +                                   defconfig_src_dir))
>
>      def add(self, defconfig, num, total):
>          """Add a new subprocess if a vacant slot is found.
> @@ -743,6 +761,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(config_attrs, options):
>      """Move config options to defconfig files.
>
> @@ -755,6 +791,20 @@ def move_config(config_attrs, options):
>          print 'Nothing to do. exit.'
>          sys.exit(0)
>
> +    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...'
> +        gitutil.Clone(cwd, defconfig_src_dir)
> +        print 'Checkout \'%s\' to find original configs.' % \
> +            gitutil.CommitHash(options.git_ref)
> +        os.chdir(defconfig_src_dir)
> +        gitutil.Checkout(options.git_ref)
> +        os.chdir(cwd)
> +
>      print 'Move the following CONFIG options (jobs: %d)' % options.jobs
>      for config_attr in config_attrs:
>          print '  %s (type: %s, default: %s)' % (config_attr['config'],
> @@ -777,7 +827,7 @@ def move_config(config_attrs, options):
>              for filename in fnmatch.filter(filenames, '*_defconfig'):
>                  defconfigs.append(os.path.join(dirpath, filename))
>
> -    slots = Slots(config_attrs, options)
> +    slots = Slots(config_attrs, options, defconfig_src_dir)
>
>      # Main loop to process defconfig files:
>      #  Add a new subprocess into a vacant slot.
> @@ -887,6 +937,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 += ' recipe_file\n\n' + \
> diff --git a/tools/patman/gitutil.py b/tools/patman/gitutil.py
> index 9e739d8..138f989 100644
> --- a/tools/patman/gitutil.py
> +++ b/tools/patman/gitutil.py
> @@ -61,6 +61,21 @@ def CountCommitsToBranch():
>      patch_count = int(stdout)
>      return patch_count
>
> +def CommitHash(commit_ref):
> +    """Gets the hash for a commit
> +
> +    Args:
> +        commit_ref: Commit ref to look up
> +
> +    Return:
> +        Hash of revision, if any, else None
> +    """
> +    pipe = ['git', 'rev-parse', '--short', commit_ref]
> +    stdout = command.RunPipe([pipe], capture=True, oneline=True).stdout
> +
> +    hash = stdout.strip()
> +    return hash
> +
>  def NameRevision(commit_hash):
>      """Gets the revision name for a commit
>
> --
> 1.7.11.5
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Hi Joe,


2015-05-30 6:23 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>


This idea seems nice, but I have some comments about the implementation.



> +    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...'

You can use signle quotes without escape sequences inside "...", and vice versa.





> diff --git a/tools/patman/gitutil.py b/tools/patman/gitutil.py
> index 9e739d8..138f989 100644
> --- a/tools/patman/gitutil.py
> +++ b/tools/patman/gitutil.py
> @@ -61,6 +61,21 @@ def CountCommitsToBranch():
>      patch_count = int(stdout)
>      return patch_count
>
> +def CommitHash(commit_ref):
> +    """Gets the hash for a commit
> +
> +    Args:
> +        commit_ref: Commit ref to look up
> +
> +    Return:
> +        Hash of revision, if any, else None
> +    """
> +    pipe = ['git', 'rev-parse', '--short', commit_ref]
> +    stdout = command.RunPipe([pipe], capture=True, oneline=True).stdout
> +
> +    hash = stdout.strip()
> +    return hash
> +
>  def NameRevision(commit_hash):
>      """Gets the revision name for a commit



Finally, this tool is going to depend on patman.  I am afraid this
tool is getting messy.

gitutils.py depends on command.py, and then command.py depends on
cros_subprocess.py.

Do you really need to invoke such a chain of libraries
to run a sub-process?


For example, you can get a hash in a single line like this:

subprocess.check_output(['git', 'rev-parse', '--short', 'HEAD'])

Hi Masahiro-san,

On Thu, Jun 4, 2015 at 11:54 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Hi Joe,
>
>
> 2015-05-30 6:23 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>
>
>
> This idea seems nice, but I have some comments about the implementation.
>
>
>
>> +    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...'
>
> You can use signle quotes without escape sequences inside "...", and vice versa.

OK

>> diff --git a/tools/patman/gitutil.py b/tools/patman/gitutil.py
>> index 9e739d8..138f989 100644
>> --- a/tools/patman/gitutil.py
>> +++ b/tools/patman/gitutil.py
>> @@ -61,6 +61,21 @@ def CountCommitsToBranch():
>>      patch_count = int(stdout)
>>      return patch_count
>>
>> +def CommitHash(commit_ref):
>> +    """Gets the hash for a commit
>> +
>> +    Args:
>> +        commit_ref: Commit ref to look up
>> +
>> +    Return:
>> +        Hash of revision, if any, else None
>> +    """
>> +    pipe = ['git', 'rev-parse', '--short', commit_ref]
>> +    stdout = command.RunPipe([pipe], capture=True, oneline=True).stdout
>> +
>> +    hash = stdout.strip()
>> +    return hash
>> +
>>  def NameRevision(commit_hash):
>>      """Gets the revision name for a commit
>
>
>
> Finally, this tool is going to depend on patman.  I am afraid this
> tool is getting messy.
>
> gitutils.py depends on command.py, and then command.py depends on
> cros_subprocess.py.
>
> Do you really need to invoke such a chain of libraries
> to run a sub-process?

Why does that matter?

> For example, you can get a hash in a single line like this:
>
> subprocess.check_output(['git', 'rev-parse', '--short', 'HEAD'])

I certainly can do that, but it seems better to reuse the common
functionality. This tool doesn't seem so useful elsewhere that a
person would want it to work easily outside of u-boot. In u-boot,
patman exists. Simon did the same thing for buildman. I can change it
to call the git commands directly sine what I'm doing is simple, but I
don't see any value in doing it.

Simon, what do you think?

Thanks,
-Joe