[U-Boot] [PATCH 0/4] tools: moveconfig: some more improvements

[U-Boot] [PATCH 0/4] tools: moveconfig: some more improvements

[Masahiro Yamada]

Masahiro Yamada (4):
  tools: moveconfig: remove document about deprecated error message
  tools: moveconfig: use sets instead of lists for failed/suspicious
    boards
  tools: moveconfig: warn loudly if moved option has no entry in Kconfig
  tools: moveconfig: add --spl option to move options for SPL build

 tools/moveconfig.py | 102 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 71 insertions(+), 31 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH 1/4] tools: moveconfig: remove document about deprecated error message

[Masahiro Yamada]

Since commit cc008299f852 ("tools: moveconfig: do not rely on type
and default value given by users"), we do not have this error case.

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

 tools/moveconfig.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/tools/moveconfig.py b/tools/moveconfig.py
index 5283689..c188235 100755
--- a/tools/moveconfig.py
+++ b/tools/moveconfig.py
@@ -58,10 +58,6 @@ It looks like one of the following:
    The define in the config header matched the one in Kconfig.
    We do not need to touch it.
 
- - Undefined.  Do nothing.
-   This config option was not found in the config header.
-   Nothing to do.
-
  - Compiler is missing.  Do nothing.
    The compiler specified for this architecture was not found
    in your PATH environment.
-- 
1.9.1

[U-Boot] [PATCH 2/4] tools: moveconfig: use sets instead of lists for failed/suspicious boards

[Masahiro Yamada]

The sets feature is handier for adding unique elements.

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

 tools/moveconfig.py | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/moveconfig.py b/tools/moveconfig.py
index c188235..93f3781 100755
--- a/tools/moveconfig.py
+++ b/tools/moveconfig.py
@@ -798,8 +798,8 @@ class Slot:
         self.reference_src_dir = reference_src_dir
         self.parser = KconfigParser(configs, options, self.build_dir)
         self.state = STATE_IDLE
-        self.failed_boards = []
-        self.suspicious_boards = []
+        self.failed_boards = set()
+        self.suspicious_boards = set()
 
     def __del__(self):
         """Delete the working directory
@@ -946,7 +946,7 @@ class Slot:
 
         log = self.parser.check_defconfig()
         if log:
-            self.suspicious_boards.append(self.defconfig)
+            self.suspicious_boards.add(self.defconfig)
             self.log += log
         orig_defconfig = os.path.join('configs', self.defconfig)
         new_defconfig = os.path.join(self.build_dir, 'defconfig')
@@ -980,19 +980,19 @@ class Slot:
                 sys.exit("Exit on error.")
             # If --exit-on-error flag is not set, skip this board and continue.
             # Record the failed board.
-            self.failed_boards.append(self.defconfig)
+            self.failed_boards.add(self.defconfig)
 
         self.progress.inc()
         self.progress.show()
         self.state = STATE_IDLE
 
     def get_failed_boards(self):
-        """Returns a list of failed boards (defconfigs) in this slot.
+        """Returns a set of failed boards (defconfigs) in this slot.
         """
         return self.failed_boards
 
     def get_suspicious_boards(self):
-        """Returns a list of boards (defconfigs) with possible misconversion.
+        """Returns a set of boards (defconfigs) with possible misconversion.
         """
         return self.suspicious_boards
 
@@ -1057,11 +1057,11 @@ class Slots:
 
     def show_failed_boards(self):
         """Display all of the failed boards (defconfigs)."""
-        boards = []
+        boards = set()
         output_file = 'moveconfig.failed'
 
         for slot in self.slots:
-            boards += slot.get_failed_boards()
+            boards |= slot.get_failed_boards()
 
         if boards:
             boards = '\n'.join(boards) + '\n'
@@ -1076,11 +1076,11 @@ class Slots:
 
     def show_suspicious_boards(self):
         """Display all boards (defconfigs) with possible misconversion."""
-        boards = []
+        boards = set()
         output_file = 'moveconfig.suspicious'
 
         for slot in self.slots:
-            boards += slot.get_suspicious_boards()
+            boards |= slot.get_suspicious_boards()
 
         if boards:
             boards = '\n'.join(boards) + '\n'
-- 
1.9.1

[U-Boot] [PATCH 3/4] tools: moveconfig: warn loudly if moved option has no entry in Kconfig

[Masahiro Yamada]

Currently, the tool gives up moving an option quietly if its entry
was not found in Kconfig.

If the option is not defined in the config header in the first
place, it is no problem (as the Kconfig entry may have been hidden
by reasonable "depends on").

However, if the option is defined in the config header, the missing
Kconfig entry is a sign of possible behavior change.  It is highly
recommended to manually check if the option has been moved as
expected.  In this case, let's add "suspicious" in the log and
change the log color (if --color option is given) to make it stand
out.

This was suggested by Tom in [1].

[1] http://lists.denx.de/pipermail/u-boot/2016-July/261988.html

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Suggested-by: Tom Rini <trini@konsulko.com>
---

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

diff --git a/tools/moveconfig.py b/tools/moveconfig.py
index 93f3781..98e8608 100755
--- a/tools/moveconfig.py
+++ b/tools/moveconfig.py
@@ -47,12 +47,18 @@ It looks like one of the following:
    This config option was moved to the defconfig
 
  - CONFIG_... is not defined in Kconfig.  Do nothing.
-   The entry for this CONFIG was not found in Kconfig.
+   The entry for this CONFIG was not found in Kconfig.  The option is not
+   defined in the config header, either.  So, this case can be just skipped.
+
+ - CONFIG_... is not defined in Kconfig (suspicious).  Do nothing.
+   This option is defined in the config header, but its entry was not found
+   in Kconfig.
    There are two common cases:
      - You forgot to create an entry for the CONFIG before running
        this tool, or made a typo in a CONFIG passed to this tool.
      - The entry was hidden due to unmet 'depends on'.
-       This is correct behavior.
+   The tool does not know if the result is reasonable, so please check it
+   manually.
 
  - 'CONFIG_...' is the same as the define in Kconfig.  Do nothing.
    The define in the config header matched the one in Kconfig.
@@ -210,7 +216,8 @@ STATE_SAVEDEFCONFIG = 3
 
 ACTION_MOVE = 0
 ACTION_NO_ENTRY = 1
-ACTION_NO_CHANGE = 2
+ACTION_NO_ENTRY_WARN = 2
+ACTION_NO_CHANGE = 3
 
 COLOR_BLACK        = '0;30'
 COLOR_RED          = '0;31'
@@ -659,14 +666,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 + '='):
@@ -675,6 +674,17 @@ 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:
+            if new_val == not_set:
+                return (ACTION_NO_ENTRY, config)
+            else:
+                return (ACTION_NO_ENTRY_WARN, config)
+
         # 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'.
@@ -704,6 +714,7 @@ class KconfigParser:
 
         results = []
         updated = False
+        suspicious = False
 
         with open(self.dotconfig) as f:
             dotconfig_lines = f.readlines()
@@ -725,6 +736,10 @@ class KconfigParser:
             elif action == ACTION_NO_ENTRY:
                 actlog = "%s is not defined in Kconfig.  Do nothing." % value
                 log_color = COLOR_LIGHT_BLUE
+            elif action == ACTION_NO_ENTRY_WARN:
+                actlog = "%s is not defined in Kconfig (suspicious).  Do nothing." % value
+                log_color = COLOR_YELLOW
+                suspicious = True
             elif action == ACTION_NO_CHANGE:
                 actlog = "'%s' is the same as the define in Kconfig.  Do nothing." \
                          % value
@@ -744,7 +759,7 @@ class KconfigParser:
         os.remove(self.config_autoconf)
         os.remove(self.autoconf)
 
-        return (updated, log)
+        return (updated, suspicious, log)
 
     def check_defconfig(self):
         """Check the defconfig after savedefconfig
@@ -923,7 +938,9 @@ class Slot:
     def do_savedefconfig(self):
         """Update the .config and run 'make savedefconfig'."""
 
-        (updated, log) = self.parser.update_dotconfig()
+        (updated, suspicious, log) = self.parser.update_dotconfig()
+        if suspicious:
+            self.suspicious_boards.add(self.defconfig)
         self.log += log
 
         if not self.options.force_sync and not updated:
@@ -994,7 +1011,7 @@ class Slot:
     def get_suspicious_boards(self):
         """Returns a set of boards (defconfigs) with possible misconversion.
         """
-        return self.suspicious_boards
+        return self.suspicious_boards - self.failed_boards
 
 class Slots:
 
-- 
1.9.1

[U-Boot] [PATCH 4/4] tools: moveconfig: add --spl option to move options for SPL build

[Masahiro Yamada]

Prior to this commit, the tool could not move options guarded by
CONFIG_SPL_BUILD ifdef conditionals because they do not show up in
include/autoconf.mk.  This new option, if given, makes the tool
parse spl/include/autoconf.mk instead of include/autoconf.mk,
which is probably preferred behavior when moving options for SPL.

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

 tools/moveconfig.py | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/tools/moveconfig.py b/tools/moveconfig.py
index 98e8608..5576b57 100755
--- a/tools/moveconfig.py
+++ b/tools/moveconfig.py
@@ -138,6 +138,12 @@ Available options
    If not specified, "make savedefconfig" only occurs for cases
    where at least one CONFIG was moved.
 
+ -S, --spl
+   Look for moved config options in spl/include/autoconf.mk instead of
+   include/autoconf.mk.  This is useful for moving options for SPL build
+   because SPL related options (mostly prefixed with CONFIG_SPL_) are
+   sometimes blocked by CONFIG_SPL_BUILD ifdef conditionals.
+
  -H, --headers-only
    Only cleanup the headers; skip the defconfig processing
 
@@ -614,6 +620,8 @@ class KconfigParser:
         self.options = options
         self.dotconfig = os.path.join(build_dir, '.config')
         self.autoconf = os.path.join(build_dir, 'include', 'autoconf.mk')
+        self.spl_autoconf = os.path.join(build_dir, 'spl', 'include',
+                                         'autoconf.mk')
         self.config_autoconf = os.path.join(build_dir, 'include', 'config',
                                             'auto.conf')
         self.defconfig = os.path.join(build_dir, 'defconfig')
@@ -715,11 +723,25 @@ class KconfigParser:
         results = []
         updated = False
         suspicious = False
+        rm_files = [self.config_autoconf, self.autoconf]
+
+        if self.options.spl:
+            if os.path.exists(self.spl_autoconf):
+                autoconf_path = self.spl_autoconf
+                rm_files.append(self.spl_autoconf)
+            else:
+                for f in rm_files:
+                    os.remove(f)
+                return (updated, suspicious,
+                        color_text(self.options.color, COLOR_BROWN,
+                                   "SPL is not enabled.  Skipped.") + '\n')
+        else:
+            autoconf_path = self.autoconf
 
         with open(self.dotconfig) as f:
             dotconfig_lines = f.readlines()
 
-        with open(self.autoconf) as f:
+        with open(autoconf_path) as f:
             autoconf_lines = f.readlines()
 
         for config in self.configs:
@@ -744,6 +766,9 @@ class KconfigParser:
                 actlog = "'%s' is the same as the define in Kconfig.  Do nothing." \
                          % value
                 log_color = COLOR_LIGHT_PURPLE
+            elif action == ACTION_SPL_NOT_EXIST:
+                actlog = "SPL is not enabled for this defconfig.  Skip."
+                log_color = COLOR_PURPLE
             else:
                 sys.exit("Internal Error. This should not happen.")
 
@@ -756,8 +781,8 @@ class KconfigParser:
                     updated = True
 
         self.results = results
-        os.remove(self.config_autoconf)
-        os.remove(self.autoconf)
+        for f in rm_files:
+            os.remove(f)
 
         return (updated, suspicious, log)
 
@@ -1217,6 +1242,8 @@ def main():
                       help='exit immediately on any error')
     parser.add_option('-s', '--force-sync', action='store_true', default=False,
                       help='force sync by savedefconfig')
+    parser.add_option('-S', '--spl', action='store_true', default=False,
+                      help='parse config options defined for SPL build')
     parser.add_option('-H', '--headers-only', dest='cleanup_headers_only',
                       action='store_true', default=False,
                       help='only cleanup the headers')
-- 
1.9.1

[U-Boot] [PATCH 3/4] tools: moveconfig: warn loudly if moved option has no entry in Kconfig

[Tom Rini]

On Mon, Aug 22, 2016 at 10:18:21PM +0900, Masahiro Yamada wrote:

> Currently, the tool gives up moving an option quietly if its entry
> was not found in Kconfig.
> 
> If the option is not defined in the config header in the first
> place, it is no problem (as the Kconfig entry may have been hidden
> by reasonable "depends on").
> 
> However, if the option is defined in the config header, the missing
> Kconfig entry is a sign of possible behavior change.  It is highly
> recommended to manually check if the option has been moved as
> expected.  In this case, let's add "suspicious" in the log and
> change the log color (if --color option is given) to make it stand
> out.
> 
> This was suggested by Tom in [1].
> 
> [1] http://lists.denx.de/pipermail/u-boot/2016-July/261988.html
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Suggested-by: Tom Rini <trini@konsulko.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160822/c9267d6e/attachment.sig>

[U-Boot] [PATCH 4/4] tools: moveconfig: add --spl option to move options for SPL build

[Tom Rini]

On Mon, Aug 22, 2016 at 10:18:22PM +0900, Masahiro Yamada wrote:

> Prior to this commit, the tool could not move options guarded by
> CONFIG_SPL_BUILD ifdef conditionals because they do not show up in
> include/autoconf.mk.  This new option, if given, makes the tool
> parse spl/include/autoconf.mk instead of include/autoconf.mk,
> which is probably preferred behavior when moving options for SPL.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160822/c4b49f26/attachment.sig>

[U-Boot] [PATCH 4/4] tools: moveconfig: add --spl option to move options for SPL build

[Andrew F. Davis]

On 08/22/2016 09:43 AM, Tom Rini wrote:
> On Mon, Aug 22, 2016 at 10:18:22PM +0900, Masahiro Yamada wrote:
> 
>> Prior to this commit, the tool could not move options guarded by
>> CONFIG_SPL_BUILD ifdef conditionals because they do not show up in
>> include/autoconf.mk.  This new option, if given, makes the tool
>> parse spl/include/autoconf.mk instead of include/autoconf.mk,
>> which is probably preferred behavior when moving options for SPL.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> 
> Reviewed-by: Tom Rini <trini@konsulko.com>
> 

I think we are still missing the case of some option being defined one
way in the SPL build case but another the regular case, which one should
be added to the defconfig?

In a set of slides[0] I found on the subject it looks like there was
going to be a system where we could conditionally define options in
defconfig based on whether we were building SPL or not. So we could run
moveconfig in multiple passes and find what kind of tag we need.

S:CONFIG_FOO=200
T:CONFIG_FOO=300
ST:CONFIG_BAR=y
+S:CONFIG_BAZ=y
+T:CONFIG_QUX=y
+ST:CONFIG_QUUX=y
etc..

Did this ever get implemented?

[0]
http://www.denx.de/wiki/pub/U-Boot/MiniSummitELCE2014/uboot2014_kconfig.pdf

[U-Boot] [PATCH 1/4] tools: moveconfig: remove document about deprecated error message

[Joe Hershberger]

On Mon, Aug 22, 2016 at 8:18 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Since commit cc008299f852 ("tools: moveconfig: do not rely on type
> and default value given by users"), we do not have this error case.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

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

[U-Boot] [PATCH 2/4] tools: moveconfig: use sets instead of lists for failed/suspicious boards

[Joe Hershberger]

On Mon, Aug 22, 2016 at 8:18 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> The sets feature is handier for adding unique elements.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

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

[U-Boot] [PATCH 4/4] tools: moveconfig: add --spl option to move options for SPL build

[Masahiro Yamada]

Hi Andrew,


2016-08-22 23:57 GMT+09:00 Andrew F. Davis <afd@ti.com>:

> I think we are still missing the case of some option being defined one
> way in the SPL build case but another the regular case, which one should
> be added to the defconfig?


  #ifdef CONFIG_SPL_BUILD
     #define CONFIG_FOO   100
  #else
     #define CONFIG_FOO   200
  #endif

is a case where we can not migrate as is.



Generally, it will be changed as follows:

config  FOO
        int "foo"

config  SPL_FOO
        int "foo for SPL"


It is too much to have regular/SPL variants
for all cases, so we need to draw a line somewhere, though.




> In a set of slides[0] I found on the subject it looks like there was
> going to be a system where we could conditionally define options in
> defconfig based on whether we were building SPL or not. So we could run
> moveconfig in multiple passes and find what kind of tag we need.
>
> S:CONFIG_FOO=200
> T:CONFIG_FOO=300
> ST:CONFIG_BAR=y
> +S:CONFIG_BAZ=y
> +T:CONFIG_QUX=y
> +ST:CONFIG_QUUX=y
> etc..
>
> Did this ever get implemented?
>
> [0]
> http://www.denx.de/wiki/pub/U-Boot/MiniSummitELCE2014/uboot2014_kconfig.pdf
>

At first, I implemented like that,
but I realized it was tedious to use.

So, commit e02ee2548 switched to single .config
like Linux has.  Instead, this requires
separate CONFIGs for U-Boot proper and SPL
as mentioned above.

-- 
Best Regards
Masahiro Yamada

[U-Boot] [U-Boot, 1/4] tools: moveconfig: remove document about deprecated error message

[Tom Rini]

On Mon, Aug 22, 2016 at 10:18:19PM +0900, Masahiro Yamada wrote:

> Since commit cc008299f852 ("tools: moveconfig: do not rely on type
> and default value given by users"), we do not have this error case.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160907/ac943f82/attachment.sig>

[U-Boot] [U-Boot, 2/4] tools: moveconfig: use sets instead of lists for failed/suspicious boards

[Tom Rini]

On Mon, Aug 22, 2016 at 10:18:20PM +0900, Masahiro Yamada wrote:

> The sets feature is handier for adding unique elements.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160907/3237ec46/attachment.sig>

[U-Boot] [U-Boot, 3/4] tools: moveconfig: warn loudly if moved option has no entry in Kconfig

[Tom Rini]

On Mon, Aug 22, 2016 at 10:18:21PM +0900, Masahiro Yamada wrote:

> Currently, the tool gives up moving an option quietly if its entry
> was not found in Kconfig.
> 
> If the option is not defined in the config header in the first
> place, it is no problem (as the Kconfig entry may have been hidden
> by reasonable "depends on").
> 
> However, if the option is defined in the config header, the missing
> Kconfig entry is a sign of possible behavior change.  It is highly
> recommended to manually check if the option has been moved as
> expected.  In this case, let's add "suspicious" in the log and
> change the log color (if --color option is given) to make it stand
> out.
> 
> This was suggested by Tom in [1].
> 
> [1] http://lists.denx.de/pipermail/u-boot/2016-July/261988.html
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Suggested-by: Tom Rini <trini@konsulko.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160907/b1200012/attachment.sig>

[U-Boot] [U-Boot, 4/4] tools: moveconfig: add --spl option to move options for SPL build

[Tom Rini]

On Mon, Aug 22, 2016 at 10:18:22PM +0900, Masahiro Yamada wrote:

> Prior to this commit, the tool could not move options guarded by
> CONFIG_SPL_BUILD ifdef conditionals because they do not show up in
> include/autoconf.mk.  This new option, if given, makes the tool
> parse spl/include/autoconf.mk instead of include/autoconf.mk,
> which is probably preferred behavior when moving options for SPL.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160907/9fae9f5b/attachment.sig>