Hi Doug,
On Wed, 6 Jul 2022 at 18:08, Doug Anderson dianders@chromium.org wrote:
Hi,
On Tue, Jul 5, 2022 at 11:16 AM Sean Anderson sean.anderson@seco.com wrote:
Hi Doug,
On 7/1/22 4:23 PM, Douglas Anderson wrote:
Ever since commit 4600767d294d ("patman: Refactor how the default subcommand works"), when I use patman on the Linux tree I get grumbles about unknown tags. This is because the Linux default making process_tags be False wasn't working anymore.
It appears that the comment claiming that the defaults propagates through all subparsers no longer works for some reason.
We're already looking at all the subparsers anyway. Let's just update each one.
Fixes: 4600767d294d ("patman: Refactor how the default subcommand works") Signed-off-by: Douglas Anderson dianders@chromium.org Tested-by: Brian Norris briannorris@chromium.org Reviewed-by: Brian Norris briannorris@chromium.org
(no changes since v1)
tools/patman/settings.py | 41 +++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-)
diff --git a/tools/patman/settings.py b/tools/patman/settings.py index 7c2b5c196c06..5eefe3d1f55e 100644 --- a/tools/patman/settings.py +++ b/tools/patman/settings.py @@ -244,28 +244,31 @@ def _UpdateDefaults(main_parser, config): if isinstance(action, argparse._SubParsersAction) for _, subparser in action.choices.items()]
- unknown_settings = set(name for name, val in config.items('settings'))
- # Collect the defaults from each parser
- defaults = {} for parser in parsers: pdefs = parser.parse_known_args()[0]
defaults.update(vars(pdefs))- # Go through the settings and collect defaults
- for name, val in config.items('settings'):
if name in defaults:default_val = defaults[name]if isinstance(default_val, bool):val = config.getboolean('settings', name)elif isinstance(default_val, int):val = config.getint('settings', name)elif isinstance(default_val, str):val = config.get('settings', name)defaults[name] = valelse:print("WARNING: Unknown setting %s" % name)- # Set all the defaults (this propagates through all subparsers)
- main_parser.set_defaults(**defaults)
defaults = dict(vars(pdefs))# Go through the settings and collect defaultsfor name, val in config.items('settings'):if name in defaults:default_val = defaults[name]if isinstance(default_val, bool):val = config.getboolean('settings', name)elif isinstance(default_val, int):val = config.getint('settings', name)elif isinstance(default_val, str):val = config.get('settings', name)defaults[name] = valunknown_settings.discard(name)# Set all the defaultsparser.set_defaults(**defaults)- for name in sorted(unknown_settings):
print("WARNING: Unknown setting %s" % name)Can you see if 4780f7d8a6b ("patman: Fix defaults not propagating to subparsers") [1] addresses this problem? The implementation is different, but I believe these should have the same effect.
To my mind the logic of your patch is a bit harder to follow, but I believe you're correct that it accomplishes the same thing. ...and my quick test also seems to confirm that yours works fine. Too bad it wasn't already in "-next" or it would have saved me a bit of time...
+Tom Rini
It's been languishing for two months due to me taking a break. The PR itself was sent a week ago but is waiting on one discussion.
I'm curious whether you agree that the logic in my patch is a little simpler. Should I re-post it as a squashed revert of yours and then apply mine and call it a "simplify" instead of a bugfix? ...or just leave yours alone? If we leave yours alone, I guess my patch #2 needs a trivial rebase to fix a merge conflict.
Regards, Simon