Re: [Qemu-devel] [PATCH] minikconf: do not include variables from MINIKCONF_ARGS in config-all-devices.mak

[Christophe de Dinechin]

> On 25 Jun 2019, at 10:13, Paolo Bonzini <address@hidden> wrote:
> 
> When minikconf writes config-devices.mak, it includes all variables including
> those from MINIKCONF_ARGS.  This causes values from config-host.mak to 
> "stick" to
> the ones used in generating config-devices.mak, because config-devices.mak is
> included after config-host.mak.  Avoid this by omitting assignments coming
> from the command line in the output of minikconf.
> 
> Reported-by: Christophe de Dinechin <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
> scripts/minikconf.py | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/minikconf.py b/scripts/minikconf.py
> index 0ffc6c38da..3109a81db7 100644
> --- a/scripts/minikconf.py
> +++ b/scripts/minikconf.py
> @@ -688,11 +688,13 @@ if __name__ == '__main__':
> 
>     data = KconfigData(mode)
>     parser = KconfigParser(data)
> +    external_vars = set()
>     for arg in argv[3:]:
>         m = re.match(r'^(CONFIG_[A-Z0-9_]+)=([yn]?)$', arg)
>         if m is not None:
>             name, value = m.groups()
>             parser.do_assignment(name, value == 'y')
> +            external_vars.add(name[7:])
>         else:
>             fp = open(arg, 'r')
>             parser.parse_file(fp)
> @@ -700,7 +702,8 @@ if __name__ == '__main__':
> 
>     config = data.compute_config()
>     for key in sorted(config.keys()):
> -        print ('CONFIG_%s=%s' % (key, ('y' if config[key] else 'n')))
> +        if key not in external_vars:
> +            print ('CONFIG_%s=%s' % (key, ('y' if config[key] else 'n')))

The approach is interesting, and shorter than the fix
I had in mind, which was to add a target to generate
all the config*.mak, then call that from the configure script,
forcing external variables on the command-line to
override config*.mak values

> 
>     deps = open(argv[2], 'w')
>     for fname in data.previously_included:
> -- 
> 2.21.0

This seems to address the leftover CONFIG_SPICE=y, but
I ran into what looks like new compilation errors with this.
For example, with a configure that strips quite a lot away:

monitor/hmp-cmds.c: In function ‘hmp_change’:
monitor/hmp-cmds.c:1946:17: error: unused variable ‘hmp_mon’ 
[-Werror=unused-variable]
 1946 |     MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
      |                 ^~~~~~~

This is apparently a side effect of that variable being used
only under CONFIG_VNC. Patch upcoming. There may be
other similar side effects.


Tested-by: Christophe de Dinechin <address@hidden>
Reviewed-by: Christophe de DInechin <address@hidden>