[U-Boot] [PATCH 0/5] tools: moveconfig: some more fix, cleanups, improvement

[U-Boot] [PATCH 0/5] tools: moveconfig: some more fix, cleanups, improvement

[Masahiro Yamada]

I sent a pull request for moveconfig recently,
but this tool can be improved more.

(This series has no impact on the on-going BOOTDELAY fix-up.)



Masahiro Yamada (5):
  tools: moveconfig: fix needless move for config with default 1
  tools: moveconfig: change class WorkDir to class ReferenceSource
  tools: moveconfig: simplify source tree switching
  tools: moveconfig: simplify show_failed_boards() and show more info
  tools: moveconfig: show suspicious boards with possible misconversion

 tools/moveconfig.py | 130 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 81 insertions(+), 49 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH 1/5] tools: moveconfig: fix needless move for config with default 1

[Masahiro Yamada]

When moving an integer type option with default value 1, the tool
moves configs with the same value as the default (, and then removed
by the later savedefconfig).  This is a needless operation.

The KconfigParser.parse_one_config() should compare the config after
the "=y -> =1" fixup.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 tools/moveconfig.py | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/moveconfig.py b/tools/moveconfig.py
index 238e961..06ff4d9 100755
--- a/tools/moveconfig.py
+++ b/tools/moveconfig.py
@@ -487,9 +487,6 @@ class KconfigParser:
         else:
             new_val = not_set
 
-        if old_val == new_val:
-            return (ACTION_NO_CHANGE, new_val)
-
         # If this CONFIG is neither bool nor trisate
         if old_val[-2:] != '=y' and old_val[-2:] != '=m' and old_val != not_set:
             # tools/scripts/define2mk.sed changes '1' to 'y'.
@@ -498,7 +495,8 @@ class KconfigParser:
             if new_val[-2:] == '=y':
                 new_val = new_val[:-1] + '1'
 
-        return (ACTION_MOVE, new_val)
+        return (ACTION_NO_CHANGE if old_val == new_val else ACTION_MOVE,
+                new_val)
 
     def update_dotconfig(self):
         """Parse files for the config options and update the .config.
-- 
1.9.1

[U-Boot] [PATCH 2/5] tools: moveconfig: change class WorkDir to class ReferenceSource

[Masahiro Yamada]

The class WorkDir can be used in a very generic way, but currently
it is only used for containing a reference source directory.

This commit changes it for a more dedicated use.  The move_config
function can be more readable by enclosing the git-clone and git-
checkout in the class constructor.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 tools/moveconfig.py | 49 ++++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/tools/moveconfig.py b/tools/moveconfig.py
index 06ff4d9..f4e2580 100755
--- a/tools/moveconfig.py
+++ b/tools/moveconfig.py
@@ -880,23 +880,39 @@ 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()
+class ReferenceSource:
+
+    """Reference source against which original configs should be parsed."""
+
+    def __init__(self, commit):
+        """Create a reference source directory based on a specified commit.
+
+        Arguments:
+          commit: commit to git-clone
+        """
+        self.src_dir = tempfile.mkdtemp()
+        print "Cloning git repo to a separate work directory..."
+        subprocess.check_output(['git', 'clone', os.getcwd(), '.'],
+                                cwd=self.src_dir)
+        print "Checkout '%s' to build the original autoconf.mk." % \
+            subprocess.check_output(['git', 'rev-parse', '--short', commit]).strip()
+        subprocess.check_output(['git', 'checkout', commit],
+                                stderr=subprocess.STDOUT, cwd=self.src_dir)
 
     def __del__(self):
-        """Delete the working directory
+        """Delete the reference source 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)
+        shutil.rmtree(self.src_dir)
+
+    def get_dir(self):
+        """Return the absolute path to the reference source directory."""
 
-    def get(self):
-        return self.work_dir
+        return self.src_dir
 
 def move_config(configs, options):
     """Move config options to defconfig files.
@@ -914,20 +930,11 @@ def move_config(configs, options):
         print 'Move ' + ', '.join(configs),
     print '(jobs: %d)\n' % options.jobs
 
-    reference_src_dir = ''
-
     if options.git_ref:
-        work_dir = WorkDir()
-        reference_src_dir = work_dir.get()
-        print "Cloning git repo to a separate work directory..."
-        subprocess.check_output(['git', 'clone', os.getcwd(), '.'],
-                                cwd=reference_src_dir)
-        print "Checkout '%s' to build the original autoconf.mk." % \
-            subprocess.check_output(['git', 'rev-parse', '--short',
-                                    options.git_ref]).strip()
-        subprocess.check_output(['git', 'checkout', options.git_ref],
-                                stderr=subprocess.STDOUT,
-                                cwd=reference_src_dir)
+        reference_src = ReferenceSource(options.git_ref)
+        reference_src_dir = reference_src.get_dir()
+    else:
+        reference_src_dir = ''
 
     if options.defconfigs:
         defconfigs = [line.strip() for line in open(options.defconfigs)]
-- 
1.9.1

[U-Boot] [PATCH 3/5] tools: moveconfig: simplify source tree switching

[Masahiro Yamada]

The subprocess.Popen() does not change the child process's working
directory if cwd=None is given.  Let's exploit this fact to refactor
the source directory handling.

We no longer have to pass "-C <reference_src_dir>" to the sub-process
because self.current_src_dir tracks the source tree against which we
want to run defconfig/autoconf.

The flag self.use_git_ref is not necessary either because we can know
the current state by checking whether the self.current_src_dir is a
valid string or None.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 tools/moveconfig.py | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/tools/moveconfig.py b/tools/moveconfig.py
index f4e2580..0e03751 100755
--- a/tools/moveconfig.py
+++ b/tools/moveconfig.py
@@ -645,7 +645,7 @@ class Slot:
 
         self.defconfig = defconfig
         self.log = ''
-        self.use_git_ref = True if self.options.git_ref else False
+        self.current_src_dir = self.reference_src_dir
         self.do_defconfig()
         return True
 
@@ -674,13 +674,13 @@ class Slot:
         if self.ps.poll() != 0:
             self.handle_error()
         elif self.state == STATE_DEFCONFIG:
-            if self.options.git_ref and not self.use_git_ref:
+            if self.reference_src_dir and not self.current_src_dir:
                 self.do_savedefconfig()
             else:
                 self.do_autoconf()
         elif self.state == STATE_AUTOCONF:
-            if self.use_git_ref:
-                self.use_git_ref = False
+            if self.current_src_dir:
+                self.current_src_dir = None
                 self.do_defconfig()
             else:
                 self.do_savedefconfig()
@@ -706,11 +706,9 @@ class Slot:
 
         cmd = list(self.make_cmd)
         cmd.append(self.defconfig)
-        if self.use_git_ref:
-            cmd.append('-C')
-            cmd.append(self.reference_src_dir)
         self.ps = subprocess.Popen(cmd, stdout=self.devnull,
-                                   stderr=subprocess.PIPE)
+                                   stderr=subprocess.PIPE,
+                                   cwd=self.current_src_dir)
         self.state = STATE_DEFCONFIG
 
     def do_autoconf(self):
@@ -728,11 +726,9 @@ class Slot:
             cmd.append('CROSS_COMPILE=%s' % self.cross_compile)
         cmd.append('KCONFIG_IGNORE_DUPLICATES=1')
         cmd.append('include/config/auto.conf')
-        if self.use_git_ref:
-            cmd.append('-C')
-            cmd.append(self.reference_src_dir)
         self.ps = subprocess.Popen(cmd, stdout=self.devnull,
-                                   stderr=subprocess.PIPE)
+                                   stderr=subprocess.PIPE,
+                                   cwd=self.current_src_dir)
         self.state = STATE_AUTOCONF
 
     def do_savedefconfig(self):
@@ -934,7 +930,7 @@ def move_config(configs, options):
         reference_src = ReferenceSource(options.git_ref)
         reference_src_dir = reference_src.get_dir()
     else:
-        reference_src_dir = ''
+        reference_src_dir = None
 
     if options.defconfigs:
         defconfigs = [line.strip() for line in open(options.defconfigs)]
-- 
1.9.1

[U-Boot] [PATCH 4/5] tools: moveconfig: simplify show_failed_boards() and show more info

[Masahiro Yamada]

Since commit 1d085568b3de ("tools: moveconfig: display log atomically
in more readable format"), the function color_text() is clever enough
to exclude LF from escape sequences.  Exploit it for removing the
"for" loops from Slots.show_failed_boards().

Also, display "(the list has been saved in moveconfig.failed)" if
there are failed boards.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 tools/moveconfig.py | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/tools/moveconfig.py b/tools/moveconfig.py
index 0e03751..f795a7f 100755
--- a/tools/moveconfig.py
+++ b/tools/moveconfig.py
@@ -860,21 +860,22 @@ class Slots:
 
     def show_failed_boards(self):
         """Display all of the failed boards (defconfigs)."""
-        failed_boards = []
+        boards = []
+        output_file = 'moveconfig.failed'
 
         for slot in self.slots:
-            failed_boards += slot.get_failed_boards()
-
-        if len(failed_boards) > 0:
-            msg = [ "The following boards were not processed due to error:" ]
-            msg += failed_boards
-            for line in msg:
-                print >> sys.stderr, color_text(self.options.color,
-                                                COLOR_LIGHT_RED, line)
-
-            with open('moveconfig.failed', 'w') as f:
-                for board in failed_boards:
-                    f.write(board + '\n')
+            boards += slot.get_failed_boards()
+
+        if boards:
+            boards = '\n'.join(boards) + '\n'
+            msg = "The following boards were not processed due to error:\n"
+            msg += boards
+            msg += "(the list has been saved in %s)\n" % output_file
+            print >> sys.stderr, color_text(self.options.color, COLOR_LIGHT_RED,
+                                            msg)
+
+            with open(output_file, 'w') as f:
+                f.write(boards)
 
 class ReferenceSource:
 
-- 
1.9.1

[U-Boot] [PATCH 5/5] tools: moveconfig: show suspicious boards with possible misconversion

[Masahiro Yamada]

There are some cases where config options are moved, but they are
ripped off at the final savedefconfig stage:

  - The moved option is not user-configurable, for example, due to
    a missing prompt in the Kconfig entry

  - The config was not defined in the original config header despite
    the Kconfig specifies it as non-bool type

  - The config define in the header contains reference to another
    macro, for example:
        #define CONFIG_CONS_INDEX     (CONFIG_SYS_LPC32XX_UART - 2)
    The current moveconfig does not support recursive macro expansion.

In these cases, the conversion is very likely to be an unexpected
result.  That is why I decided to display the log in yellow color
in commit 5da4f857beac ("tools: moveconfig: report when CONFIGs are
removed by savedefconfig").

It would be nice to display the list of suspicious boards when the
tool finishes processing.  It is highly recommended to check the
defconfigs once again when this message is displayed.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 tools/moveconfig.py | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/tools/moveconfig.py b/tools/moveconfig.py
index f795a7f..bf60dbc 100755
--- a/tools/moveconfig.py
+++ b/tools/moveconfig.py
@@ -611,6 +611,7 @@ class Slot:
         self.parser = KconfigParser(configs, options, self.build_dir)
         self.state = STATE_IDLE
         self.failed_boards = []
+        self.suspicious_boards = []
 
     def __del__(self):
         """Delete the working directory
@@ -755,7 +756,10 @@ class Slot:
     def update_defconfig(self):
         """Update the input defconfig and go back to the idle state."""
 
-        self.log += self.parser.check_defconfig()
+        log = self.parser.check_defconfig()
+        if log:
+            self.suspicious_boards.append(self.defconfig)
+            self.log += log
         orig_defconfig = os.path.join('configs', self.defconfig)
         new_defconfig = os.path.join(self.build_dir, 'defconfig')
         updated = not filecmp.cmp(orig_defconfig, new_defconfig)
@@ -799,6 +803,11 @@ class Slot:
         """
         return self.failed_boards
 
+    def get_suspicious_boards(self):
+        """Returns a list of boards (defconfigs) with possible misconversion.
+        """
+        return self.suspicious_boards
+
 class Slots:
 
     """Controller of the array of subprocess slots."""
@@ -877,6 +886,26 @@ class Slots:
             with open(output_file, 'w') as f:
                 f.write(boards)
 
+    def show_suspicious_boards(self):
+        """Display all boards (defconfigs) with possible misconversion."""
+        boards = []
+        output_file = 'moveconfig.suspicious'
+
+        for slot in self.slots:
+            boards += slot.get_suspicious_boards()
+
+        if boards:
+            boards = '\n'.join(boards) + '\n'
+            msg = "The following boards might have been converted incorrectly.\n"
+            msg += "It is highly recommended to check them manually:\n"
+            msg += boards
+            msg += "(the list has been saved in %s)\n" % output_file
+            print >> sys.stderr, color_text(self.options.color, COLOR_YELLOW,
+                                            msg)
+
+            with open(output_file, 'w') as f:
+                f.write(boards)
+
 class ReferenceSource:
 
     """Reference source against which original configs should be parsed."""
@@ -967,6 +996,7 @@ def move_config(configs, options):
 
     print ''
     slots.show_failed_boards()
+    slots.show_suspicious_boards()
 
 def main():
     try:
-- 
1.9.1

[U-Boot] [PATCH 1/5] tools: moveconfig: fix needless move for config with default 1

[Joe Hershberger]

On Wed, Jun 15, 2016 at 12:33 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> When moving an integer type option with default value 1, the tool
> moves configs with the same value as the default (, and then removed
> by the later savedefconfig).  This is a needless operation.
>
> The KconfigParser.parse_one_config() should compare the config after
> the "=y -> =1" fixup.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 2/5] tools: moveconfig: change class WorkDir to class ReferenceSource

[Joe Hershberger]

On Wed, Jun 15, 2016 at 12:33 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> The class WorkDir can be used in a very generic way, but currently
> it is only used for containing a reference source directory.
>
> This commit changes it for a more dedicated use.  The move_config
> function can be more readable by enclosing the git-clone and git-
> checkout in the class constructor.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 3/5] tools: moveconfig: simplify source tree switching

[Joe Hershberger]

On Wed, Jun 15, 2016 at 12:33 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> The subprocess.Popen() does not change the child process's working
> directory if cwd=None is given.  Let's exploit this fact to refactor
> the source directory handling.
>
> We no longer have to pass "-C <reference_src_dir>" to the sub-process
> because self.current_src_dir tracks the source tree against which we
> want to run defconfig/autoconf.
>
> The flag self.use_git_ref is not necessary either because we can know
> the current state by checking whether the self.current_src_dir is a
> valid string or None.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  tools/moveconfig.py | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/tools/moveconfig.py b/tools/moveconfig.py
> index f4e2580..0e03751 100755
> --- a/tools/moveconfig.py
> +++ b/tools/moveconfig.py
> @@ -645,7 +645,7 @@ class Slot:
>
>          self.defconfig = defconfig
>          self.log = ''
> -        self.use_git_ref = True if self.options.git_ref else False
> +        self.current_src_dir = self.reference_src_dir
>          self.do_defconfig()
>          return True
>
> @@ -674,13 +674,13 @@ class Slot:
>          if self.ps.poll() != 0:
>              self.handle_error()
>          elif self.state == STATE_DEFCONFIG:
> -            if self.options.git_ref and not self.use_git_ref:
> +            if self.reference_src_dir and not self.current_src_dir:
>                  self.do_savedefconfig()
>              else:
>                  self.do_autoconf()
>          elif self.state == STATE_AUTOCONF:
> -            if self.use_git_ref:
> -                self.use_git_ref = False
> +            if self.current_src_dir:
> +                self.current_src_dir = None

This seems less clear to read. There is no current source dir? I think
you need a different name.

>                  self.do_defconfig()
>              else:
>                  self.do_savedefconfig()
> @@ -706,11 +706,9 @@ class Slot:
>
>          cmd = list(self.make_cmd)
>          cmd.append(self.defconfig)
> -        if self.use_git_ref:
> -            cmd.append('-C')
> -            cmd.append(self.reference_src_dir)
>          self.ps = subprocess.Popen(cmd, stdout=self.devnull,
> -                                   stderr=subprocess.PIPE)
> +                                   stderr=subprocess.PIPE,
> +                                   cwd=self.current_src_dir)
>          self.state = STATE_DEFCONFIG
>
>      def do_autoconf(self):
> @@ -728,11 +726,9 @@ class Slot:
>              cmd.append('CROSS_COMPILE=%s' % self.cross_compile)
>          cmd.append('KCONFIG_IGNORE_DUPLICATES=1')
>          cmd.append('include/config/auto.conf')
> -        if self.use_git_ref:
> -            cmd.append('-C')
> -            cmd.append(self.reference_src_dir)
>          self.ps = subprocess.Popen(cmd, stdout=self.devnull,
> -                                   stderr=subprocess.PIPE)
> +                                   stderr=subprocess.PIPE,
> +                                   cwd=self.current_src_dir)
>          self.state = STATE_AUTOCONF
>
>      def do_savedefconfig(self):
> @@ -934,7 +930,7 @@ def move_config(configs, options):
>          reference_src = ReferenceSource(options.git_ref)
>          reference_src_dir = reference_src.get_dir()
>      else:
> -        reference_src_dir = ''
> +        reference_src_dir = None
>
>      if options.defconfigs:
>          defconfigs = [line.strip() for line in open(options.defconfigs)]
> --
> 1.9.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 4/5] tools: moveconfig: simplify show_failed_boards() and show more info

[Joe Hershberger]

On Wed, Jun 15, 2016 at 12:33 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Since commit 1d085568b3de ("tools: moveconfig: display log atomically
> in more readable format"), the function color_text() is clever enough
> to exclude LF from escape sequences.  Exploit it for removing the
> "for" loops from Slots.show_failed_boards().
>
> Also, display "(the list has been saved in moveconfig.failed)" if
> there are failed boards.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 5/5] tools: moveconfig: show suspicious boards with possible misconversion

[Joe Hershberger]

On Wed, Jun 15, 2016 at 12:33 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> There are some cases where config options are moved, but they are
> ripped off at the final savedefconfig stage:
>
>   - The moved option is not user-configurable, for example, due to
>     a missing prompt in the Kconfig entry
>
>   - The config was not defined in the original config header despite
>     the Kconfig specifies it as non-bool type
>
>   - The config define in the header contains reference to another
>     macro, for example:
>         #define CONFIG_CONS_INDEX     (CONFIG_SYS_LPC32XX_UART - 2)
>     The current moveconfig does not support recursive macro expansion.
>
> In these cases, the conversion is very likely to be an unexpected
> result.  That is why I decided to display the log in yellow color
> in commit 5da4f857beac ("tools: moveconfig: report when CONFIGs are
> removed by savedefconfig").
>
> It would be nice to display the list of suspicious boards when the
> tool finishes processing.  It is highly recommended to check the
> defconfigs once again when this message is displayed.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 3/5] tools: moveconfig: simplify source tree switching

[Masahiro Yamada]

2016-06-21 6:33 GMT+09:00 Joe Hershberger <joe.hershberger@gmail.com>:
> On Wed, Jun 15, 2016 at 12:33 AM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> The subprocess.Popen() does not change the child process's working
>> directory if cwd=None is given.  Let's exploit this fact to refactor
>> the source directory handling.
>>
>> We no longer have to pass "-C <reference_src_dir>" to the sub-process
>> because self.current_src_dir tracks the source tree against which we
>> want to run defconfig/autoconf.
>>
>> The flag self.use_git_ref is not necessary either because we can know
>> the current state by checking whether the self.current_src_dir is a
>> valid string or None.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  tools/moveconfig.py | 22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/moveconfig.py b/tools/moveconfig.py
>> index f4e2580..0e03751 100755
>> --- a/tools/moveconfig.py
>> +++ b/tools/moveconfig.py
>> @@ -645,7 +645,7 @@ class Slot:
>>
>>          self.defconfig = defconfig
>>          self.log = ''
>> -        self.use_git_ref = True if self.options.git_ref else False
>> +        self.current_src_dir = self.reference_src_dir
>>          self.do_defconfig()
>>          return True
>>
>> @@ -674,13 +674,13 @@ class Slot:
>>          if self.ps.poll() != 0:
>>              self.handle_error()
>>          elif self.state == STATE_DEFCONFIG:
>> -            if self.options.git_ref and not self.use_git_ref:
>> +            if self.reference_src_dir and not self.current_src_dir:
>>                  self.do_savedefconfig()
>>              else:
>>                  self.do_autoconf()
>>          elif self.state == STATE_AUTOCONF:
>> -            if self.use_git_ref:
>> -                self.use_git_ref = False
>> +            if self.current_src_dir:
>> +                self.current_src_dir = None
>
> This seems less clear to read. There is no current source dir? I think
> you need a different name.


Maybe,  self.subprocess_dir or something?




-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 3/5] tools: moveconfig: simplify source tree switching

[Joe Hershberger]

On Mon, Jun 20, 2016 at 8:53 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 2016-06-21 6:33 GMT+09:00 Joe Hershberger <joe.hershberger@gmail.com>:
>> On Wed, Jun 15, 2016 at 12:33 AM, Masahiro Yamada
>> <yamada.masahiro@socionext.com> wrote:
>>> The subprocess.Popen() does not change the child process's working
>>> directory if cwd=None is given.  Let's exploit this fact to refactor
>>> the source directory handling.
>>>
>>> We no longer have to pass "-C <reference_src_dir>" to the sub-process
>>> because self.current_src_dir tracks the source tree against which we
>>> want to run defconfig/autoconf.
>>>
>>> The flag self.use_git_ref is not necessary either because we can know
>>> the current state by checking whether the self.current_src_dir is a
>>> valid string or None.
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>>
>>>  tools/moveconfig.py | 22 +++++++++-------------
>>>  1 file changed, 9 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/tools/moveconfig.py b/tools/moveconfig.py
>>> index f4e2580..0e03751 100755
>>> --- a/tools/moveconfig.py
>>> +++ b/tools/moveconfig.py
>>> @@ -645,7 +645,7 @@ class Slot:
>>>
>>>          self.defconfig = defconfig
>>>          self.log = ''
>>> -        self.use_git_ref = True if self.options.git_ref else False
>>> +        self.current_src_dir = self.reference_src_dir
>>>          self.do_defconfig()
>>>          return True
>>>
>>> @@ -674,13 +674,13 @@ class Slot:
>>>          if self.ps.poll() != 0:
>>>              self.handle_error()
>>>          elif self.state == STATE_DEFCONFIG:
>>> -            if self.options.git_ref and not self.use_git_ref:
>>> +            if self.reference_src_dir and not self.current_src_dir:
>>>                  self.do_savedefconfig()
>>>              else:
>>>                  self.do_autoconf()
>>>          elif self.state == STATE_AUTOCONF:
>>> -            if self.use_git_ref:
>>> -                self.use_git_ref = False
>>> +            if self.current_src_dir:
>>> +                self.current_src_dir = None
>>
>> This seems less clear to read. There is no current source dir? I think
>> you need a different name.
>
>
> Maybe,  self.subprocess_dir or something?

How about something like self.alternate_src_dir?

-Joe

[U-Boot] [PATCH 3/5] tools: moveconfig: simplify source tree switching

[Masahiro Yamada]

2016-06-22 1:25 GMT+09:00 Joe Hershberger <joe.hershberger@gmail.com>:
> On Mon, Jun 20, 2016 at 8:53 PM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> 2016-06-21 6:33 GMT+09:00 Joe Hershberger <joe.hershberger@gmail.com>:
>>> On Wed, Jun 15, 2016 at 12:33 AM, Masahiro Yamada
>>> <yamada.masahiro@socionext.com> wrote:
>>>> The subprocess.Popen() does not change the child process's working
>>>> directory if cwd=None is given.  Let's exploit this fact to refactor
>>>> the source directory handling.
>>>>
>>>> We no longer have to pass "-C <reference_src_dir>" to the sub-process
>>>> because self.current_src_dir tracks the source tree against which we
>>>> want to run defconfig/autoconf.
>>>>
>>>> The flag self.use_git_ref is not necessary either because we can know
>>>> the current state by checking whether the self.current_src_dir is a
>>>> valid string or None.
>>>>
>>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>> ---
>>>>
>>>>  tools/moveconfig.py | 22 +++++++++-------------
>>>>  1 file changed, 9 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/tools/moveconfig.py b/tools/moveconfig.py
>>>> index f4e2580..0e03751 100755
>>>> --- a/tools/moveconfig.py
>>>> +++ b/tools/moveconfig.py
>>>> @@ -645,7 +645,7 @@ class Slot:
>>>>
>>>>          self.defconfig = defconfig
>>>>          self.log = ''
>>>> -        self.use_git_ref = True if self.options.git_ref else False
>>>> +        self.current_src_dir = self.reference_src_dir
>>>>          self.do_defconfig()
>>>>          return True
>>>>
>>>> @@ -674,13 +674,13 @@ class Slot:
>>>>          if self.ps.poll() != 0:
>>>>              self.handle_error()
>>>>          elif self.state == STATE_DEFCONFIG:
>>>> -            if self.options.git_ref and not self.use_git_ref:
>>>> +            if self.reference_src_dir and not self.current_src_dir:
>>>>                  self.do_savedefconfig()
>>>>              else:
>>>>                  self.do_autoconf()
>>>>          elif self.state == STATE_AUTOCONF:
>>>> -            if self.use_git_ref:
>>>> -                self.use_git_ref = False
>>>> +            if self.current_src_dir:
>>>> +                self.current_src_dir = None
>>>
>>> This seems less clear to read. There is no current source dir? I think
>>> you need a different name.
>>
>>
>> Maybe,  self.subprocess_dir or something?
>
> How about something like self.alternate_src_dir?
>

So,
reference_src_dir is still
alternate_src_dir moves


This is not clear to me from the variable names.


My first choice "current" means
it is a moving directory.

I can live with subprocess_dir, though.



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 3/5] tools: moveconfig: simplify source tree switching

[Joe Hershberger]

On Tue, Jun 21, 2016 at 4:13 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 2016-06-22 1:25 GMT+09:00 Joe Hershberger <joe.hershberger@gmail.com>:
>> On Mon, Jun 20, 2016 at 8:53 PM, Masahiro Yamada
>> <yamada.masahiro@socionext.com> wrote:
>>> 2016-06-21 6:33 GMT+09:00 Joe Hershberger <joe.hershberger@gmail.com>:
>>>> On Wed, Jun 15, 2016 at 12:33 AM, Masahiro Yamada
>>>> <yamada.masahiro@socionext.com> wrote:
>>>>> The subprocess.Popen() does not change the child process's working
>>>>> directory if cwd=None is given.  Let's exploit this fact to refactor
>>>>> the source directory handling.
>>>>>
>>>>> We no longer have to pass "-C <reference_src_dir>" to the sub-process
>>>>> because self.current_src_dir tracks the source tree against which we
>>>>> want to run defconfig/autoconf.
>>>>>
>>>>> The flag self.use_git_ref is not necessary either because we can know
>>>>> the current state by checking whether the self.current_src_dir is a
>>>>> valid string or None.
>>>>>
>>>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>> ---
>>>>>
>>>>>  tools/moveconfig.py | 22 +++++++++-------------
>>>>>  1 file changed, 9 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/tools/moveconfig.py b/tools/moveconfig.py
>>>>> index f4e2580..0e03751 100755
>>>>> --- a/tools/moveconfig.py
>>>>> +++ b/tools/moveconfig.py
>>>>> @@ -645,7 +645,7 @@ class Slot:
>>>>>
>>>>>          self.defconfig = defconfig
>>>>>          self.log = ''
>>>>> -        self.use_git_ref = True if self.options.git_ref else False
>>>>> +        self.current_src_dir = self.reference_src_dir
>>>>>          self.do_defconfig()
>>>>>          return True
>>>>>
>>>>> @@ -674,13 +674,13 @@ class Slot:
>>>>>          if self.ps.poll() != 0:
>>>>>              self.handle_error()
>>>>>          elif self.state == STATE_DEFCONFIG:
>>>>> -            if self.options.git_ref and not self.use_git_ref:
>>>>> +            if self.reference_src_dir and not self.current_src_dir:
>>>>>                  self.do_savedefconfig()
>>>>>              else:
>>>>>                  self.do_autoconf()
>>>>>          elif self.state == STATE_AUTOCONF:
>>>>> -            if self.use_git_ref:
>>>>> -                self.use_git_ref = False
>>>>> +            if self.current_src_dir:
>>>>> +                self.current_src_dir = None
>>>>
>>>> This seems less clear to read. There is no current source dir? I think
>>>> you need a different name.
>>>
>>>
>>> Maybe,  self.subprocess_dir or something?
>>
>> How about something like self.alternate_src_dir?
>>
>
> So,
> reference_src_dir is still
> alternate_src_dir moves
>
>
> This is not clear to me from the variable names.
>
>
> My first choice "current" means
> it is a moving directory.
>
> I can live with subprocess_dir, though.

OK, do that then.

-Joe

[U-Boot] [PATCH 0/5] tools: moveconfig: some more fix, cleanups, improvement

[Masahiro Yamada]

2016-06-15 14:33 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
>
> I sent a pull request for moveconfig recently,
> but this tool can be improved more.
>
> (This series has no impact on the on-going BOOTDELAY fix-up.)
>
>
>
> Masahiro Yamada (5):
>   tools: moveconfig: fix needless move for config with default 1
>   tools: moveconfig: change class WorkDir to class ReferenceSource
>   tools: moveconfig: simplify source tree switching
>   tools: moveconfig: simplify show_failed_boards() and show more info
>   tools: moveconfig: show suspicious boards with possible misconversion
>
>  tools/moveconfig.py | 130 ++++++++++++++++++++++++++++++++-

Applied to u-boot-uniphier/master.




-- 
Best Regards
Masahiro Yamada